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 read() performance #29337

Merged
merged 1 commit into from
Aug 31, 2019
Merged

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

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

@Trott
Copy link
Member

Trott commented Aug 29, 2019

@nodejs/streams

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
PR-URL: nodejs#29337
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@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
PR-URL: #29337
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
PR-URL: #29337
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
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. 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.

7 participants