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

stream: pipe lazy drain #29095

Closed
wants to merge 1 commit into from

Conversation

@ronag
Copy link
Contributor

commented Aug 12, 2019

Slight optimization. Don't allocate and register the drain function for fast consumers.

Related #29087

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

NOTE TO SELF: After this has been merged, look into removing unnecessary onclose.

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch from 99c0615 to 9bf7643 Aug 12, 2019

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch 2 times, most recently from 3355012 to 10fe46a Aug 12, 2019

@ronag ronag changed the title Stream pipe lazy drain stream: pipe lazy drain Aug 12, 2019

@ronag ronag referenced this pull request Aug 12, 2019
0 of 7 tasks complete

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch 4 times, most recently from b3be9d3 to e81f6c2 Aug 12, 2019

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Mixing pipe with 'readable' is a nono since registering readable messes with state.flowing.

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch from e81f6c2 to c8da80a Aug 12, 2019

test/parallel/test-stream-pipe-flow.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch from c8da80a to 45bac8d Aug 12, 2019

@jasnell jasnell requested a review from mcollina Aug 13, 2019

@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Why do you think this is a optimization?

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

It skips the extra allocation and lookup.

@mcollina
Copy link
Member

left a comment

LGTM with green CITGM

// pipe() cleanup function, we'll still function properly.
r.on('readable', function() {
w.emit('drain');
});

This comment has been minimized.

Copy link
@mcollina

mcollina Aug 13, 2019

Member

Why was this removed?

This comment has been minimized.

Copy link
@ronag

ronag Aug 13, 2019

Author Contributor

Mixing pipe with 'readable' is a nono since registering readable messes with state.flowing.

This comment has been minimized.

Copy link
@ronag

ronag Aug 13, 2019

Author Contributor

I'm actually not sure what this is supposed to test and whether is even valid?

lib/_stream_readable.js Show resolved Hide resolved

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch 2 times, most recently from 836880a to 60067c6 Aug 13, 2019

@ronag ronag force-pushed the nxtedition:stream-pipe-lazy-drain branch from 60067c6 to 59eb36c Aug 13, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

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

Results are decidedly "no significant change in performance detected". None of them came in with a p-value lower than 0.05.

13:47:22                                                        confidence improvement accuracy (*)    (**)   (***)
13:47:22  streams/creation.js kind='duplex' n=50000000                          0.42 %       ±2.59%  ±3.44%  ±4.48%
13:47:22  streams/creation.js kind='readable' n=50000000                        0.61 %       ±4.21%  ±5.65%  ±7.45%
13:47:22  streams/creation.js kind='transform' n=50000000                      -0.14 %       ±2.46%  ±3.29%  ±4.31%
13:47:22  streams/creation.js kind='writable' n=50000000                       -2.13 %       ±3.29%  ±4.37%  ±5.69%
13:47:22  streams/pipe.js n=5000000                                             1.55 %       ±3.43%  ±4.56%  ±5.95%
13:47:22  streams/pipe-object-mode.js n=5000000                                 1.41 %       ±3.66%  ±4.87%  ±6.34%
13:47:22  streams/readable-bigread.js n=1000                                   -1.95 %       ±2.44%  ±3.24%  ±4.22%
13:47:22  streams/readable-bigunevenread.js n=1000                              6.89 %      ±14.51% ±19.31% ±25.15%
13:47:22  streams/readable-boundaryread.js type='buffer' n=2000                -0.89 %       ±2.11%  ±2.81%  ±3.66%
13:47:22  streams/readable-boundaryread.js type='string' n=2000                -0.15 %       ±1.98%  ±2.64%  ±3.43%
13:47:22  streams/readable-readall.js n=5000                                   -0.29 %       ±1.98%  ±2.64%  ±3.43%
13:47:22  streams/readable-unevenread.js n=1000                                -1.07 %       ±1.85%  ±2.48%  ±3.25%
13:47:22  streams/writable-manywrites.js n=2000000                              0.54 %       ±5.16%  ±6.87%  ±8.94%
13:47:22 
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

(Of course, maybe the benchmark isn't significantly exercising the optimization here?)

@Trott

This comment has been minimized.

@Trott
Trott approved these changes Aug 16, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Landed in 7195cd6

@Trott Trott closed this Aug 16, 2019

Trott added a commit that referenced this pull request Aug 16, 2019
stream: use lazy registration for drain for fast destinations
PR-URL: #29095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
stream: use lazy registration for drain for fast destinations
PR-URL: #29095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
stream: use lazy registration for drain for fast destinations
PR-URL: #29095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos referenced this pull request Aug 19, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: use lazy registration for drain for fast destinations
PR-URL: nodejs#29095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: use lazy registration for drain for fast destinations
PR-URL: nodejs#29095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.