FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: fix sync write perf regression by ronag · Pull Request #33032 · nodejs/node · GitHub

/ node Public

stream: fix sync write perf regression - #33032

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-opt-write
ConversationCommits (1)ChecksFiles changed
Closed

stream: fix sync write perf regression#33032
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-opt-write

Conversation

ronag commented Apr 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 added the stream Issues and PRs related to the stream subsystem. label Apr 23, 2020
ronag requested a review from mscdex April 23, 2020 19:59
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 force-pushed the stream-opt-write branch from 7fd2050 to bc93484 Compare April 23, 2020 20:00
ronag commented Apr 23, 2020
View reviewed changes
Comment thread lib/_stream_writable.js
}
} else {
if (!state.destroyed) {
if (state.buffered.length > state.bufferedIndex) {

ronag Apr 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

ronag Apr 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

nodejs-github-bot commented Apr 23, 2020

Copy link
Copy Markdown
Collaborator

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

jasnell commented Apr 23, 2020

Copy link
Copy Markdown
Member

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

ronag commented Apr 23, 2020

Copy link
Copy Markdown
Member Author

@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 approved these changes Apr 23, 2020
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 24, 2020

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

Copy link
Copy Markdown
Collaborator

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

ronag added a commit that referenced this pull request Apr 25, 2020
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 <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>

ronag commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

Landed in ab7d9db

ronag closed this Apr 25, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
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 <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
BethGriggs mentioned this pull request Apr 27, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
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 <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
BridgeAR BridgeAR approved these changes
himself65 himself65 approved these changes
+1 more reviewer
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 outstanding review comments, 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

Back | FazBrowse Home | New Git URL