Skip to content

stream: fix sync write perf regression #33032

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
Closed
wants to merge 1 commit into from
Closed

stream: fix sync write perf regression #33032

wants to merge 1 commit into from
+14 −17

Conversation

@ronag
Copy link
Member

@ronag ronag commented Apr 23, 2020 •
edited
Loading

While #31046 did make async writes faster it at the same time made sync writes slower.

This PR corrects this while maintaining performance improvements.

 streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000                    2.55 %       ±3.61% ±4.81%  ±6.26%
 streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000          ***      7.46 %       ±3.52% ±4.69%  ±6.10%
 streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000                   4.13 %       ±4.74% ±6.34%  ±8.32%
 streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000                  3.91 %       ±4.16% ±5.53%  ±7.20%
 streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000            *      3.91 %       ±3.09% ±4.11%  ±5.36%
 streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000         ***     24.84 %       ±3.67% ±4.89%  ±6.36%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000                  2.69 %       ±6.09% ±8.13% ±10.64%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000        ***     12.66 %       ±3.47% ±4.62%  ±6.01%
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

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Apr 23, 2020
@ronag ronag requested a review from mscdex April 23, 2020 19:59
@ronag
stream: fix sync write perf regression
bc93484 
While nodejs#31046 did make async
writes faster it at the same time made sync writes slower.

This PR corrects this while maintaining performance improvements.
@ronag ronag force-pushed the stream-opt-write branch from 7fd2050 to bc93484 Compare April 23, 2020 20:00
ronag
ronag commented Apr 23, 2020
View reviewed changes
lib/_stream_writable.js
}
} else {
if (!state.destroyed) {
if (state.buffered.length > state.bufferedIndex) {
Copy link
Member Author

@ronag ronag Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

since clearBuffer is not inlineable this condition removes a lot of the overhead of calling into clearBuffer

ronag
ronag commented Apr 23, 2020
View reviewed changes
lib/_stream_writable.js
if (state.buffered.length > state.bufferedIndex) {
clearBuffer(stream, state);
}
if (state.needDrain || cb !== nop || state.ending || state.destroyed) {
Copy link
Member Author

@ronag ronag Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This condition was an optimization which did not add much if any value and made the code more complex.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 23, 2020

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

@jasnell
Copy link
Member

jasnell commented Apr 23, 2020

@ronag ... can you run a benchmark comparison between this and the code before #31046 landed?

@ronag
Copy link
Member Author

ronag commented Apr 23, 2020

@ronag ... can you run a benchmark comparison between this and the code before #31046 landed?

 streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000           ***     12.92 %       ±0.79% ±1.05% ±1.37%
 streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000           **     -2.58 %       ±1.56% ±2.08% ±2.72%
 streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000          ***     35.82 %       ±2.93% ±3.90% ±5.08%
 streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000                  0.48 %       ±2.18% ±2.90% ±3.78%
 streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000          ***     13.58 %       ±2.63% ±3.50% ±4.56%
 streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000         ***     -3.50 %       ±1.81% ±2.41% ±3.14%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000         ***     15.47 %       ±2.17% ±2.89% ±3.76%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000                 1.47 %       ±2.02% ±2.69% ±3.50%

BridgeAR
BridgeAR approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

RSLGTM

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2020 •
edited by ronag
Loading

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

ronag added a commit that referenced this pull request Apr 25, 2020
@ronag
stream: fix sync write perf regression
ab7d9db 
While #31046 did make async
writes faster it at the same time made sync writes slower.

This PR corrects this while maintaining performance improvements.

PR-URL: #33032
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@ronag
Copy link
Member Author

ronag commented Apr 25, 2020

Landed in ab7d9db

@ronag ronag closed this Apr 25, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@ronag @BethGriggs
stream: fix sync write perf regression
7b2382b 
While #31046 did make async
writes faster it at the same time made sync writes slower.

This PR corrects this while maintaining performance improvements.

PR-URL: #33032
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
@ronag @BethGriggs
stream: fix sync write perf regression
e07c4ff 
While #31046 did make async
writes faster it at the same time made sync writes slower.

This PR corrects this while maintaining performance improvements.

PR-URL: #33032
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@himself65 himself65 himself65 approved these changes

+1 more reviewer

@mscdex mscdex mscdex 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. 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.

8 participants

@ronag @nodejs-github-bot @jasnell @mscdex @BridgeAR @himself65 @addaleax @targos