Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: fix readable state awaitDrain increase incorrectly in recursion #27572

Closed
wants to merge 1 commit into from

Conversation

abbshr
Copy link
Contributor

@abbshr abbshr commented May 5, 2019

fix #27571

Checklist

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label May 5, 2019
lib/_stream_readable.js Outdated Show resolved Hide resolved
lib/_stream_readable.js Show resolved Hide resolved
lib/_stream_readable.js Outdated Show resolved Hide resolved
lib/_stream_readable.js Outdated Show resolved Hide resolved
@abbshr
Copy link
Contributor Author

abbshr commented May 6, 2019 via email

@abbshr
Copy link
Contributor Author

abbshr commented May 6, 2019 via email

@abbshr
Copy link
Contributor Author

abbshr commented May 6, 2019

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

2019-05-06 21 10 49

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

addaleax commented May 8, 2019

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
Copy link
Member

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
Copy link
Contributor Author

abbshr commented May 11, 2019

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
Copy link

👍

@addaleax
Copy link
Member

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
Copy link
Contributor Author

abbshr commented May 11, 2019

Understood, thanks for the explanation.

@addaleax
Copy link
Member

Yeah, this looks better to me :)

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

lib/_stream_writable.js Outdated Show resolved Hide resolved
test/parallel/test-stream2-basic.js Outdated Show resolved Hide resolved
lib/_stream_readable.js Outdated Show resolved Hide resolved
lib/_stream_readable.js Show resolved Hide resolved
@BridgeAR
Copy link
Member

The benchmark looks fine now 🎉.

@@ -695,17 +713,21 @@ Readable.prototype.pipe = function(dest, pipeOpts) {
debug('ondata');
const ret = dest.write(chunk);
debug('dest.write', ret);
if (ret === false) {
if (ret === false && !cleanedUp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@abbshr abbshr May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@abbshr
Copy link
Contributor Author

abbshr commented Aug 26, 2019

@addaleax Ok, I have rebased the commits

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 26, 2019
addaleax pushed a commit that referenced this pull request Aug 26, 2019
PR-URL: #27572
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

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

@BridgeAR
Copy link
Member

BridgeAR commented Sep 3, 2019

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
Copy link
Contributor Author

abbshr commented Sep 18, 2019

@BridgeAR 👌

@BridgeAR
Copy link
Member

@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
Copy link
Member

quick ping re: backport

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#27572
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#27572
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #27572
Backport-PR-URL: #33056
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readable stream's awaitDrain increased incorrectly
8 participants