Skip to content

stream: improve read() performance#29337

Merged
mscdex merged 1 commit intonodejs:masterfrom
mscdex:stream-read-perf
Aug 31, 2019
Merged

stream: improve read() performance#29337
mscdex merged 1 commit intonodejs:masterfrom
mscdex:stream-read-perf

Conversation

@mscdex
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 27, 2019

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

Fishrock123
Fishrock123 reviewed Aug 28, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread lib/internal/streams/buffer_list.js Outdated
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 29, 2019

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

@Trott
Copy link
Copy Markdown
Member

Trott commented Aug 29, 2019

@nodejs/streams

mcollina
mcollina approved these changes Aug 29, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

mcollina commented Aug 30, 2019

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
Copy Markdown
Member

mcollina commented Aug 30, 2019

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 <[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
@mscdex @BridgeAR
stream: improve read() performance
d82ecb9 
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
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 <[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

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

+2 more reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@devnexen devnexen devnexen approved these changes

Reviewers whose approvals may not affect merge requirements

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