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.

Sign up for GitHub

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

Jump to bottom

stream: improve read() performance #29337

Merged
merged 1 commit into from
Aug 31, 2019
Merged

stream: improve read() performance #29337

merged 1 commit into from
Aug 31, 2019

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 27, 2019

Benchmark results:

                                                        confidence improvement accuracy (*)    (**)   (***)
 streams/readable-bigread.js n=1000                           ***      9.17 %       ±0.96%  ±1.28%  ±1.66%
 streams/readable-unevenread.js n=1000                        ***      5.02 %       ±1.06%  ±1.41%  ±1.84%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex mscdex added stream Issues and PRs related to the stream subsystem. performance Issues and PRs related to the performance of Node.js. labels Aug 27, 2019
@nodejs-github-bot
Copy link
Collaborator

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

Fishrock123
Fishrock123 reviewed Aug 28, 2019
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

This code is frankly quite difficult to understand, but I think this is correct.

lib/internal/streams/buffer_list.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 29, 2019

@nodejs/streams

mcollina
mcollina approved these changes Aug 29, 2019
View reviewed changes
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, good work! A CITGM run would be helpful to assess any breakage.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 29, 2019

@mcollina What breakage? These changes consist of:

  • Replacing src.copy() with dest.set() (like was done in one of my recent streams perf PRs)

  • Folding the copy of the first list item into the loop

  • Avoiding unnecessary calculations once we've reached the last buffer in the list used to fill ret

@mcollina
Copy link
Member

I'm skeptical of any side effect a change in streams can have at this point, so a run would not be a waste of time.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1987/

@mcollina
Copy link
Member

I don't see anything suspicious on CITGM, LGTM from me.

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 30, 2019
@mscdex
stream: improve read() performance
98b7185 
PR-URL: nodejs#29337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@mscdex mscdex merged commit 98b7185 into nodejs:master Aug 31, 2019
@mscdex mscdex deleted the stream-read-perf branch August 31, 2019 03:42
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@mscdex @BridgeAR
stream: improve read() performance
d82ecb9 
PR-URL: #29337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
@mscdex @BridgeAR
stream: improve read() performance
2efd72f 
PR-URL: #29337
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mscdex @nodejs-github-bot @Trott @mcollina @jasnell @Fishrock123 @devnexen