Skip to content

stream: simplify Writable.write#31146

Closed
ronag wants to merge 1 commit intonodejs:masterfrom
nxtedition:stream-write-simplify
Closed

stream: simplify Writable.write#31146
ronag wants to merge 1 commit intonodejs:masterfrom
nxtedition:stream-write-simplify

Conversation

@ronag
Copy link
Member

@ronag ronag commented Dec 31, 2019 •
edited
Loading

Simplifies Writable.write and adds an optimization where if you pass 'buffer' as the encoding parameter to write(chunk, encoding, cb) it will bypass a slow path where we check whether chunk is a Buffer or not.

#31146 (comment)

Details ```js streams/writable-manywrites.js sync='no' n=2000000 0.98 % ±2.18% ±2.88% ±3.72% streams/writable-manywrites.js sync='yes' n=2000000 *** 30.92 % ±4.45% ±5.93% ±7.76% ````
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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Dec 31, 2019
ronag
ronag commented Dec 31, 2019
View reviewed changes
@ronag ronag force-pushed the stream-write-simplify branch 4 times, most recently from e5ca143 to 4741a70 Compare January 1, 2020 11:38
@ronag ronag changed the title stream: simplify Writable.write stream: write with 'buffer' Jan 1, 2020
mcollina
mcollina approved these changes Jan 1, 2020
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 with green CITGM

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

nodejs-github-bot commented Jan 1, 2020

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

@lpinca
Copy link
Member

lpinca commented Jan 1, 2020

@nodejs/streams

@Trott
Copy link
Member

Trott commented Jan 1, 2020 •
edited
Loading

lgtm with green CITGM

CITGM: https://ci.nodejs.org/job/citgm-smoker/2176/ ✔️

@ronag
Copy link
Member Author

ronag commented Jan 1, 2020

@BridgeAR: Applied your comment regarding typeof vs instanceof here as well

@mscdex
Copy link
Contributor

mscdex commented Jan 2, 2020

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/499/console

streams/writable-manywrites.js callback='no' writev='no' encoding='buffer' sync='no' n=1000000             *     -2.77 %       ±2.12% ±2.80%  ±3.63%
streams/writable-manywrites.js callback='no' writev='no' encoding='buffer' sync='yes' n=1000000          ***    -23.16 %       ±4.65% ±6.21%  ±8.14%
streams/writable-manywrites.js callback='no' writev='no' encoding='' sync='no' n=1000000                          0.00 %       ±1.27% ±1.69%  ±2.17%
streams/writable-manywrites.js callback='no' writev='no' encoding='' sync='yes' n=1000000                         1.67 %       ±5.11% ±6.80%  ±8.86%
streams/writable-manywrites.js callback='no' writev='yes' encoding='buffer' sync='no' n=1000000          ***    -13.38 %       ±1.96% ±2.60%  ±3.36%
streams/writable-manywrites.js callback='no' writev='yes' encoding='buffer' sync='yes' n=1000000         ***    -19.33 %       ±4.21% ±5.62%  ±7.35%
streams/writable-manywrites.js callback='no' writev='yes' encoding='' sync='no' n=1000000                        -0.51 %       ±2.27% ±3.01%  ±3.88%
streams/writable-manywrites.js callback='no' writev='yes' encoding='' sync='yes' n=1000000                        3.47 %       ±4.99% ±6.65%  ±8.68%
streams/writable-manywrites.js callback='yes' writev='no' encoding='buffer' sync='no' n=1000000            *     -1.28 %       ±1.11% ±1.47%  ±1.90%
streams/writable-manywrites.js callback='yes' writev='no' encoding='buffer' sync='yes' n=1000000         ***    -20.16 %       ±5.16% ±6.88%  ±8.99%
streams/writable-manywrites.js callback='yes' writev='no' encoding='' sync='no' n=1000000                        -0.28 %       ±2.24% ±2.96%  ±3.83%
streams/writable-manywrites.js callback='yes' writev='no' encoding='' sync='yes' n=1000000                       -3.40 %       ±5.56% ±7.40%  ±9.63%
streams/writable-manywrites.js callback='yes' writev='yes' encoding='buffer' sync='no' n=1000000         ***     -9.50 %       ±2.43% ±3.22%  ±4.15%
streams/writable-manywrites.js callback='yes' writev='yes' encoding='buffer' sync='yes' n=1000000        ***    -15.69 %       ±5.18% ±6.89%  ±8.98%
streams/writable-manywrites.js callback='yes' writev='yes' encoding='' sync='no' n=1000000                        0.27 %       ±2.86% ±3.78%  ±4.88%
streams/writable-manywrites.js callback='yes' writev='yes' encoding='' sync='yes' n=1000000              ***     10.14 %       ±5.79% ±7.72% ±10.09%

Seems like a lot of bad regressions...

mscdex
mscdex previously requested changes Jan 2, 2020
View reviewed changes
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

Performance regressions should be resolved first

@ronag
Copy link
Member Author

ronag commented Jan 2, 2020 •
edited
Loading

I'm a little confused what happened here. Passing 'buffer' now seems to have a significant negative instead of positive performance impact.

I've slightly changed this to not include the encoding !== 'buffer' "fast/slow" path.

 streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000            **      2.84 %       ±1.80% ±2.38% ±3.08%
 streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000                  -1.17 %       ±1.80% ±2.39% ±3.12%
 streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000           **      2.03 %       ±1.23% ±1.62% ±2.09%
 streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000                 -1.34 %       ±2.10% ±2.82% ±3.71%
 streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000                   1.26 %       ±1.47% ±1.95% ±2.51%
 streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000                 -0.73 %       ±2.41% ±3.21% ±4.21%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000          **      2.30 %       ±1.39% ±1.84% ±2.39%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000                 1.45 %       ±2.27% ±3.04% ±4.01%

@ronag ronag changed the title stream: write with 'buffer' stream: simplify Writable.write Jan 2, 2020
@ronag ronag force-pushed the stream-write-simplify branch 2 times, most recently from 4eb510d to 7e59633 Compare January 2, 2020 11:06
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2020
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

@ronag I think it makes sense to keep the encoding part in the benchmark. That way it's easier to identify regressions.

@ronag
Copy link
Member Author

ronag commented Jan 2, 2020

@ronag I think it makes sense to keep the encoding part in the benchmark. That way it's easier to identify regressions.

But right now the encoding part doesn't do anything with 'buffer'? It would just unnecessarily make the benchmark run slower.

BridgeAR
BridgeAR reviewed Jan 2, 2020
View reviewed changes
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/501/

@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

@mscdex the performance issue is resolved. PTAL.

@ronag
Copy link
Member Author

ronag commented Feb 9, 2020

rebased

@ronag ronag force-pushed the stream-write-simplify branch from e19ef3a to bf34c46 Compare February 9, 2020 09:30
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 9, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 9, 2020 •
edited by ronag
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/29035/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/519/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 9, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 9, 2020

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

@ronag ronag force-pushed the stream-write-simplify branch from bf34c46 to b3e3180 Compare February 9, 2020 15:34
@ronag
Copy link
Member Author

ronag commented Feb 9, 2020

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/520/

@ronag
Copy link
Member Author

ronag commented Feb 14, 2020

@mcollina: Just a quick post rebase LGTM before landing?

mcollina
mcollina approved these changes Feb 14, 2020
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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 14, 2020

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

@ronag
Copy link
Member Author

ronag commented Feb 14, 2020

Whatever is causing #31146 (comment) needs to be fixed....

I believe this was resolved in a different commit related to http2.

@ronag
Copy link
Member Author

ronag commented Feb 14, 2020

Landed in 194f2e20db11

@ronag ronag closed this Feb 14, 2020
ronag added a commit that referenced this pull request Feb 14, 2020
@ronag
stream: simplify Writable.write
0c7ff7f 
Slightly refactors Writable.write for minor perf
and readability improvements.

PR-URL: #31146
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere
Copy link
Member

codebytere commented Feb 29, 2020

@ronag this'll need manual bp it looks like, but feel free to swap out the label with dont-land if we shouldn't land it.

@MylesBorins MylesBorins mentioned this pull request Mar 9, 2020
Closed
7 tasks
@ronag ronag mentioned this pull request Mar 9, 2020
Closed
4 tasks
@ronag
Copy link
Member Author

ronag commented Mar 10, 2020

@MylesBorins this will now land cleanly on 13.x-staging

MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
@ronag @MylesBorins
stream: simplify Writable.write
9d1b1a3 
Slightly refactors Writable.write for minor perf
and readability improvements.

PR-URL: #31146
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@Trott Trott Trott requested changes

@mcollina mcollina mcollina approved these changes

@lpinca lpinca lpinca approved these changes

+2 more reviewers

@devnexen devnexen devnexen approved these changes

@mscdex mscdex mscdex left review comments

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.