streams: performance improvements for clearBuffer in Writable #3751
Conversation
|
LGTM |
|
cc @nodejs/streams |
| var entry = state.corkedCbs; | ||
| var cbs = entry.cbs; | ||
|
|
||
| state.corkedCbs = entry.next; |
There was a problem hiding this comment.
Has this property already been set on the object? If not I would suggest placing it, even just this.corkedCbs = null; in the constructor. This will help prevent the object map from changing, and aide the optimizing compiler.
There was a problem hiding this comment.
It is done some line before: https://github.com/nodejs/node/pull/3751/files#diff-1915a7b992a3012b177dd855fa3477e0R116
|
👍 from me |
|
👍 LGTM |
| }); | ||
|
|
||
| if (state.corkedCbs) { | ||
| state.corkedCbs.next = new CorkCbs(cbs) |
There was a problem hiding this comment.
Can this only ever be two corkedCbs long? If we already have corkedCbs, and it has .next assigned, will we replace it with a new set of callbacks?
There was a problem hiding this comment.
@chrisdickinson good spot! I planned to write more about this! My understanding is that it is only 2 corkedCbs long. Maybe I am wrong, see these lines, the callback can be deferred on the nextTick: https://github.com/mcollina/node/blob/perf-clearBuffer/lib/_stream_writable.js#L360-L364.
There might also be a different way to implement that, maybe having an array with two objects, and a even/odd counter.
|
I like the direction this is going! I might be misunderstanding this bit though — is this behavior intended? |
| if (state.corkedCbs) { | ||
| state.corkedCbs.next = new CorkCbs(cbs) | ||
| } else { | ||
| state.corkedCbs = new CorkCbs(cbs) |
|
I've renamed the field, changed the little class into @chrisdickinson are you happy with my explanation on the double element? Do you see another (possibly faster) way to implement this? |
|
LGTM |
I think so, though I might suggest adding a comment (or an |
mcollina
force-pushed
the
perf-clearBuffer
branch
from
d867f55 to
a23723d
Compare
|
@chrisdickinson I have added an assert and a comment. It should be ok now. Let me know if it is ok and I'll squash my commits. |
|
CI Looks good. Couple of related failures. |
|
@jasnell anything I should look into/fix? It seems there are windows-specific and arm-specific failures. On a window configuration, test-child-process-fork-regr-gh-2847.js is failing. On ARM, I can't access https://ci.nodejs.org/job/node-test-commit-arm/1188/nodes=armv7-wheezy/tapTestReport/ (I get a Gateway Timeout) - so I can't really know. On ARM fanned (what does that mean?), I'm getting a timeout/failure on test-crypto-dh.js. |
|
There's nothing you need to do. The issues are with the CI environment
|
|
LGTM — I don't have access to the CI server at the moment, perhaps @jasnell can restart CI? |
Do you mean technically or you're on your phone or something that makes it impractical? If you technically can't access it then we should sort that out! |
|
@rvagg: ah! I take it back. Last I checked I was in a weird state where I could kind of log in using the old pw auth, but it seems to have resolved. False alarm! |
This commits removes a function allocation inside the clearBuffer function. Moreover, it adds counting of buffered chunks to instantiate a fixed-sized Array. The performance improvements are in the range 5-50%, depending on the usecase.
mcollina
force-pushed
the
perf-clearBuffer
branch
from
a23723d to
1819b6f
Compare
|
I've run this on the CI, and I'm getting a possibly unrelated failure on node-test-binary-windows: https://ci.nodejs.org/job/node-test-binary-windows/301/. (I've rebased this to master, and squashed the commits) @chrisdickinson @jasnell if you folks are good with this, I would like to get this merged at some point this week. |
|
Those are known flakey tests, so LGTM. |
|
Yep, LGTM — Feel free to merge at will! |
|
I've been working on this for the last two days :D. Turn out V8 is improved in the latest update, and this is not needed anymore in this current form. I am so happy to be proven wrong, because it means all the node apps will be really faster soon, for free. However there is still the original use case I discovered this for (100+ chunks for a single I will submit a separate PR for that, referencing this one. |
This commits removes a function allocation inside the clearBuffer function. Moreover, it adds counting of buffered chunks to instantiate a fixed-sized Array. The performance improvements are in the range
5-50%, depending on the usecase.
Here is gist of the results of the http benchmarks in node on my MacBook Pro Late 2014 (i7, 16GB of RAM): https://gist.github.com/mcollina/5c59c057cb5b138fcd70.
cc @trevnorris @lucamaraschi @nodejs/benchmarking