Repository navigation
Conversation
|
Ok，I'll do that
Rich Trott <[email protected]> 于 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>
.
|
|
You are right about using `Set`. I'll add some test cases and run lint
Anna Henningsen <[email protected]> 于 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>
.
|
|
Code fixed and local build & test ok. But CI failed:
|
|
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… |
|
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 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 |
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: After: |
|
👍 |
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. |
|
Understood, thanks for the explanation. |
|
Yeah, this looks better to me :) Fresh benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/372/ |
|
The benchmark looks fine now 🎉. |
There was a problem hiding this comment.
Should we call src.pause() in case it's cleaned up? If so: that prevents this from happening.
There was a problem hiding this comment.
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.
|
@abbshr Sorry that we kind of dropped the ball on this, could you rebase the PR? |
|
@addaleax Ok, I have rebased the commits |
addaleax
added
the
author ready
PR-URL: #27572 Reviewed-By: Anna Henningsen <[email protected]>
|
Landed in 698a294, thanks for the PR and sorry again that this didn’t land earlier! |
|
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 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 |
|
quick ping re: backport |
PR-URL: nodejs#27572 Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#27572 Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #27572 Backport-PR-URL: #33056 Reviewed-By: Anna Henningsen <[email protected]>
8 participants
fix #27571
Checklist
make -j4 test(UNIX)