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: improve readable push performance #13113

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented May 19, 2017

This is mostly about improving performance when pushing strings, but pushing Buffers seems to be just a tad faster too. Specifically this PR makes two types of changes: avoid unnecessary chunk validation and reducing duplicated conditionals.

Here are results from one of the benchmark files I modified to be able to test strings:

                                                        improvement confidence      p.value
 streams/readable-boundaryread.js type="buffer" n=2000      3.04 %          * 3.055942e-02
 streams/readable-boundaryread.js type="string" n=2000     19.08 %        *** 2.927688e-11

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • stream

@mscdex mscdex added performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem. labels May 19, 2017
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM with CI green.

@mcollina
Copy link
Member

When CITGM is restored, a CITGM run would be highly welcomed.

@mscdex mscdex force-pushed the stream-readable-push-perf branch from 22eade4 to 89aa63b Compare May 20, 2017 03:54
@mscdex
Copy link
Contributor Author

mscdex commented May 20, 2017

@mscdex
Copy link
Contributor Author

mscdex commented May 23, 2017

There does not seem to be any CITGM failures caused by this particular PR.

@mcollina
Copy link
Member

@mscdex great news, LGTM!

@mscdex mscdex force-pushed the stream-readable-push-perf branch from 89aa63b to b94a7e0 Compare May 23, 2017 12:32
@addaleax
Copy link
Member

Landed in 359ea2a

@addaleax addaleax closed this May 23, 2017
addaleax pushed a commit that referenced this pull request May 23, 2017
PR-URL: #13113
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
jasnell pushed a commit that referenced this pull request May 24, 2017
PR-URL: #13113
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@mscdex mscdex deleted the stream-readable-push-perf branch May 26, 2017 08:35
jasnell pushed a commit that referenced this pull request May 28, 2017
PR-URL: #13113
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Jul 17, 2017
@MylesBorins
Copy link
Contributor

Should this be backported? Letting it bake for a little bit if so

@mcollina
Copy link
Member

I'm 👍 on waiting, but it can be backported.

@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants