stream: improve performance for sync write finishes by addaleax · Pull Request #30710 · nodejs/node

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

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

+        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

ghost

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.

ghost · 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 🙃

ghost · 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.

ghost · 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.

ghost · 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).

ghost · 2019-11-29T15:37:26Z

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

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

 }

-function afterWrite(stream, state, cb) {
+function afterWriteTick({ 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?

function afterWriteTick(info) { const { stream, state, count, cb } = info; info.cb = null; return afterWrite(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 a write('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.

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

Landed in 2205f85

nodejs-github-bot added the stream Issues and PRs related to Node.js streams. 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

Comment thread lib/_stream_writable.js Outdated

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 with CI started, the required approvals, and no outstanding review comments. 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!

Conversation

addaleax commented Nov 29, 2019

Checklist

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 29, 2019

Uh oh!

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

Uh oh!

Uh oh!

ghost commented Nov 29, 2019

Uh oh!

Uh oh!

ghost commented Nov 29, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Nov 30, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 30, 2019

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ghost Nov 29, 2019 •

edited by mscdex

Loading

ghost commented Nov 29, 2019 •

edited by ronag

Loading

ghost commented Nov 29, 2019 •

edited by addaleax

Loading

ghost Nov 29, 2019 •

edited by ronag

Loading

ghost Nov 30, 2019 •

edited by ronag

Loading