lib: fix readable state awaitDrain increase incorrectly in recursion

[abbshr]

fix #27571

Checklist

• make -j4 test (UNIX)
• tests are included
• commit message follows commit guidelines

[abbshr]

Ok，I'll do that Rich Trott <notifications@github.com> 于 2019年5月6日周一 06:31写道：

…

***@***.**** commented on this pull request. ------------------------------ In lib/_stream_readable.js <#27572 (comment)>: > (state.pipesCount > 1 && state.pipes.includes(dest))) && !cleanedUp) { debug('false write response, pause', state.awaitDrain); state.awaitDrain++; + state.awaitDrainDestSet.add(dest) Lint error: missing a semicolon on this line. Run make lint-js (or vcbuild lint-js on Windows) to get lint results. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27572 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXJWS7FUSLUMDT35XRVYFDPT5N5DANCNFSM4HK3WU6Q> .

[abbshr]

You are right about using `Set`. I'll add some test cases and run lint Anna Henningsen <notifications@github.com> 于 2019年5月6日周一 01:23写道：

…

***@***.**** commented on this pull request. ------------------------------ In lib/_stream_readable.js <#27572 (comment)>: > @@ -134,6 +134,8 @@ function ReadableState(options, stream, isDuplex) { // The number of writers that are awaiting a drain event in .pipe()s this.awaitDrain = 0; + // Use a WeakSet to ref the piped dest which has already been counted on `this.awaitDrain` + this.awaitDrainDestSet = new WeakSet(); Why is this a WeakSet and not a Set? The pipe should keep the destination alive from the source anyway, to the two should be functionally equivalent, with Set having the added niceness of having a deterministic size that could replace awaitDrain. ------------------------------ In lib/_stream_readable.js <#27572 (comment)>: > @@ -753,12 +758,14 @@ Readable.prototype.pipe = function(dest, pipeOpts) { return dest; }; -function pipeOnDrain(src) { +function pipeOnDrain(src, dest) { You shouldn’t need the extra parameter, the destination is available as this inside the event listener. ------------------------------ In lib/_stream_readable.js <#27572 (comment)>: > @@ -700,11 +702,14 @@ Readable.prototype.pipe = function(dest, pipeOpts) { // to get stuck in a permanently paused state if that write // also returned false. // => Check whether `dest` is still a piping destination. - if (((state.pipesCount === 1 && state.pipes === dest) || + + // To avoid increasing the `state.awaitDrain++` incorrectly when in a sync recursion `dest.write(chunk)` call. Here we check if the dest has already been counted on `state.awaitDrain` + if (!state.awaitDrainDestSet.has(dest) && ((state.pipesCount === 1 && state.pipes === dest) || Can you add a test, and make sure that make test passes locally? I think at least the linter should fail for these lines. (You probably also want to either run benchmarks yourself or have us do it, because this is likely to have a non-trivial effect.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27572 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXJWS7DNFV3JDZJFJYCTOTPT4JZRANCNFSM4HK3WU6Q> .

[abbshr]

Code fixed and local build & test ok. But CI failed:

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22983/

[addaleax]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/365/

If the results are okay then this looks good to me, but somehow I’m a bit afraid they won’t be…

[addaleax]

Here’s a fresh benchmark CI run: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/366/

That being said, I kind of like the original approach better… maybe we could optimize it similar to what we do for EventEmitter? There, we store the list of listeners in a somewhat optimized fashion; undefined if there are no listeners, the function itself if there is 1 listener, and an array of functions if there are more listeners.

99.99 % of streams are either going to have 0 or 1 pipe destinations, so the code could be very fast for those cases, not requiring any extra objects, and falling back to a Set for more than 1 destination would also be okay imo, even if it might impact performance a bit. The downside would likely be even more complex code here.

[abbshr]

I kind of like the original approach better…

Why you say so? Emm... I think the optimization made some improvement, even it still has some penalty on performance. It seems there is no obvious way to fix this problem without impacting the perf.

Before:

02:34:41                                                        confidence improvement accuracy (*)   (**)   (***)
07:50:57  streams/pipe.js n=5000000                                    ***    -25.22 %       ±3.93% ±5.23%  ±6.80%
07:50:57  streams/pipe-object-mode.js n=5000000                        ***    -29.62 %       ±4.03% ±5.37%  ±7.01%
07:50:57  streams/readable-bigread.js n=1000                           ***     -6.35 %       ±2.46% ±3.29%  ±4.32%
07:50:57  streams/readable-bigunevenread.js n=1000                     ***     -8.46 %       ±1.58% ±2.11%  ±2.74%
07:50:57  streams/readable-boundaryread.js type='buffer' n=2000        ***    -36.27 %       ±1.68% ±2.24%  ±2.92%
07:50:57  streams/readable-boundaryread.js type='string' n=2000        ***    -20.20 %       ±1.80% ±2.40%  ±3.14%
07:50:57  streams/readable-readall.js n=5000                                    1.83 %       ±2.28% ±3.04%  ±3.98%
07:50:57  streams/readable-unevenread.js n=1000                        ***    -20.60 %       ±1.12% ±1.49%  ±1.95%
07:50:57  streams/writable-manywrites.js n=2000000                       *     -7.03 %       ±6.40% ±8.52% ±11.10%

After:

02:34:41                                                        confidence improvement accuracy (*)   (**)   (***)
02:34:41  streams/pipe.js n=5000000                                            -0.69 %       ±3.90% ±5.19%  ±6.76%
02:34:41  streams/pipe-object-mode.js n=5000000                                -4.37 %       ±4.43% ±5.92%  ±7.77%
02:34:41  streams/readable-bigread.js n=1000                           ***     -3.63 %       ±1.92% ±2.56%  ±3.33%
02:34:41  streams/readable-bigunevenread.js n=1000                              0.15 %       ±2.98% ±3.98%  ±5.21%
02:34:41  streams/readable-boundaryread.js type='buffer' n=2000        ***    -13.16 %       ±1.58% ±2.10%  ±2.74%
02:34:41  streams/readable-boundaryread.js type='string' n=2000        ***     -4.69 %       ±2.11% ±2.81%  ±3.67%
02:34:41  streams/readable-readall.js n=5000                                   -0.60 %       ±2.20% ±2.94%  ±3.84%
02:34:41  streams/readable-unevenread.js n=1000                        ***     -7.30 %       ±1.84% ±2.45%  ±3.20%
02:34:41  streams/writable-manywrites.js n=2000000                              1.76 %       ±7.13% ±9.49% ±12.36%

[christineRR]

👍

[addaleax]

I kind of like the original approach better…

Why you say so? Emm... I think the optimization made some improvement, even it still has some penalty on performance. It seems there is no obvious way to fix this problem without impacting the perf.

Because I think the original approach could be optimized in such a way that, for the common cases, there is no real performance impact, as mentioned above.

I think a 13 % regression is still a somewhat significant one, so I would prefer to look into alternatives to the current approach before we merge this.

[abbshr]

Understood, thanks for the explanation.

[addaleax]

Yeah, this looks better to me :)

Fresh benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/372/

[BridgeAR]

The benchmark looks fine now 🎉.

[BridgeAR]

Should we call src.pause() in case it's cleaned up? If so: that prevents this from happening.

[abbshr]

Should we call src.pause() in case it's cleaned up? If so: that prevents this from happening.

src.pause() is related to ret === false and has nothing to do with cleanup.

[addaleax]

@abbshr Sorry that we kind of dropped the ball on this, could you rebase the PR?

[abbshr]

@addaleax Ok, I have rebased the commits

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25247/

[addaleax]

Landed in 698a294, thanks for the PR and sorry again that this didn’t land earlier!

[BridgeAR]

Should this be backported to at least v12.x? If so, this requires a manual backport due to multiple conflicts. Here is a guide how to create manual backports.

[abbshr]

@BridgeAR 👌

[BridgeAR]

@abbshr I guess you pushed some commits to your local repository? Would you be so kind and open a pull request with the backport specific for Node.js v12? The branch it should correspond with is v12.x-staging.

[MylesBorins]

quick ping re: backport