stream: improve performance for sync write finishes#30710

addaleax · 2019-11-29T01:07:44Z

Improve performance and reduce memory usage when a writable stream
is written to with the same callback (which is the most common case)
and when the write operation finishes synchronously (which is also
often the case).

 confidence improvement accuracy (*) (**) (***) streams/writable-manywrites.js sync='no' n=2000000 0.99 % ±3.20% ±4.28% ±5.61% streams/writable-manywrites.js sync='yes' n=2000000 *** 710.69 % ±19.65% ±26.47% ±35.09%

Refs: #18013
Refs: #18367

@nodejs/streams @ronag

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Improve performance and reduce memory usage when a writable stream is written to with the same callback (which is the most common case) and when the write operation finishes synchronously (which is also often the case). confidence improvement accuracy (*) (**) (***) streams/writable-manywrites.js sync='no' n=2000000 0.99 % ±3.20% ±4.28% ±5.61% streams/writable-manywrites.js sync='yes' n=2000000 *** 710.69 % ±19.65% ±26.47% ±35.09% Refs: nodejs#18013 Refs: nodejs#18367

mscdex · 2019-11-29T03:49:50Z

lib/_stream_writable.js

+state.afterWriteTickInfo.count++;
+}else{
+state.afterWriteTickInfo={count: 1, cb, stream, state };
+process.nextTick(afterWriteTick,state.afterWriteTickInfo);


Is there any difference in just using afterWrite directly here (process.nextTick(afterWrite, stream, ...))?

@mscdex We need to allocate an object anyway so that we can modify count later, so that’s why it’s not just spreading the arguments right now

mcollina

lgtm

how did you find out about this problem? Was this tied with a specific use case? I'm almost never pass a callback to write.

addaleax · 2019-11-29T10:16:52Z

how did you find out about this problem? Was this tied with a specific use case? I'm almost never pass a callback to write.

Well … Somebody asked for help privately because they were running a Node.js program, which consisted of a single synchronous loop and printed output using console.log() on each iteration, which lead to memory exhaustion after a few hours because the process.nextTick() queue was filling up 🙃

ronag · 2019-11-29T11:20:34Z

LGTM

Using the callback is very uncommon in my experience. I'm not sure the extra complexity is worth it? I'm a little worried about the maintenance cost (in general) of streams.

I would maybe instead consider looking into why console.log uses a callback and whether it's strictly necessary.

ronag · 2019-11-29T11:38:18Z

Actually looking into this further this is not an optimization just for the callback case, but in general when doing multiple write calls in the same tick.

lib/_stream_writable.js

addaleax · 2019-11-29T12:20:33Z

Using the callback is very uncommon in my experience. I'm not sure the extra complexity is worth it? I'm a little worried about the maintenance cost (in general) of streams.

Yeah, this also applies when no callback is passed -- that being said, I would understand if people were concerned about this working only for streams that potentially call write callbacks synchronously (although a number of builtin streams do that).

nodejs-github-bot · 2019-11-29T15:37:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27173/ (:white_check_mark:)

ronag · 2019-11-29T20:20:06Z

lib/_stream_writable.js

 }

-functionafterWrite(stream,state,cb){
+functionafterWriteTick({ stream, state, count, cb }){


This might clear the wrong object. I think clearing the count and cb of the passed object is safer then modifying state?
functionafterWriteTick(info){const{ stream, state, count, cb }=info;info.cb=null;returnafterWrite(stream,state,count,cb);
This would also allow reusing the object and avoiding allocations:
if(!state.afterWriteTickInfo||state.afterWriteTickInfo.cb){state.afterWriteTickInfo={ stream, state, cb,count: 1};}else{state.afterWriteTickInfo.cb=cb;state.afterWriteTickInfo.count=1;}

Not sure if it matter though

This comment is for the row below.

@ronag So … the effect of setting afterWriteTickInfo to null is that the next time the code above is reached, a new process.nextTick() call with a new afterWriteTickInfo object is made. That’s always safe, right?
I think setting .cb to null would have the same effect, and .count is cleared anyway. I can do that instead, if you prefer, although it might screw with the map/hidden class of afterWriteTickInfo, as .cb is always a function right now.

I was more thinking of the case where you have two different cbs, e.g.
write('a',cba)// schedule tick awrite('b',cbb)// clear info a, schedule tick b// ...// tick a// clear info b// tick b// clear nothing
The a tick will actually clear the info for the b tick.
Probably not a problem, but maybe a little weird... I don't have a strong opinion if you think it's fine.

it might screw with the map/hidden class
Oh, I didn't know that null could cause a problems with that once it's been a function type.

The a tick will actually clear the info for the b tick.
Probably not a problem, but maybe a little weird... I don't have a strong opinion if you think it's fine.
Yeah, I think that’s fine, because it would only make a difference if there’s a write('b', cbb) inside cba(), and that seems like a somewhat unlikely scenario, and even then it would only affect performance, not behaviour.

Improve performance and reduce memory usage when a writable stream is written to with the same callback (which is the most common case) and when the write operation finishes synchronously (which is also often the case). confidence improvement accuracy (*) (**) (***) streams/writable-manywrites.js sync='no' n=2000000 0.99 % ±3.20% ±4.28% ±5.61% streams/writable-manywrites.js sync='yes' n=2000000 *** 710.69 % ±19.65% ±26.47% ±35.09% Refs: #18013 Refs: #18367 PR-URL: #30710 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax · 2019-12-01T01:14:24Z

Landed in 2205f85

Improve performance and reduce memory usage when a writable stream is written to with the same callback (which is the most common case) and when the write operation finishes synchronously (which is also often the case). confidence improvement accuracy (*) (**) (***) streams/writable-manywrites.js sync='no' n=2000000 0.99 % ±3.20% ±4.28% ±5.61% streams/writable-manywrites.js sync='yes' n=2000000 *** 710.69 % ±19.65% ±26.47% ±35.09% Refs: #18013 Refs: #18367 PR-URL: #30710 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Nov 29, 2019

mscdex reviewed Nov 29, 2019
View reviewed changes

mcollina approved these changes Nov 29, 2019
View reviewed changes

ronag approved these changes Nov 29, 2019
View reviewed changes

ronag reviewed Nov 29, 2019
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved

fixup! stream: improve performance for sync write finishes
302ec92

jasnell approved these changes Nov 29, 2019
View reviewed changes

ronag reviewed Nov 29, 2019
View reviewed changes

ronag mentioned this pull request Nov 30, 2019
stream: improve write performance #30736
Closed
4 tasks

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019

addaleax closed this Dec 1, 2019

addaleax deleted the writable-nocb branch December 1, 2019 01:14

BridgeAR mentioned this pull request Dec 3, 2019
v13.3.0 proposal #30774
Merged

BethGriggs mentioned this pull request Dec 9, 2019
v12.13.2 proposal #30865
Closed

BethGriggs mentioned this pull request Dec 23, 2019
v12.14.1 proposal #31069
Merged

This was referenced Jan 5, 2020
Allocation failure scavenge might not succeed nodejs/help#2388
Closed
stream: hide afterWriteTickInfo property #31287
Closed

Uh oh!

stream: improve performance for sync write finishes#30710

stream: improve performance for sync write finishes #30710

Uh oh!

Conversation

addaleax commented Nov 29, 2019

Checklist

Uh oh!

mscdexNov 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 29, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 29, 2019

Uh oh!

ronag commented Nov 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Nov 29, 2019

Uh oh!

Uh oh!

addaleax commented Nov 29, 2019

Uh oh!

nodejs-github-bot commented Nov 29, 2019• edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronagNov 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronagNov 29, 2019

Choose a reason for hiding this comment

Uh oh!

ronagNov 29, 2019

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 29, 2019

Choose a reason for hiding this comment

Uh oh!

ronagNov 30, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronagNov 30, 2019

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 30, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mscdexNov 29, 2019•
edited
Loading

ronag commented Nov 29, 2019•
edited
Loading

nodejs-github-bot commented Nov 29, 2019•
edited by addaleax
Loading

ronagNov 29, 2019•
edited
Loading

ronagNov 30, 2019•
edited
Loading