stream: simplify Writable.write by ronag · Pull Request #31146 · nodejs/node

ronag · 2019-12-31T11:45:15Z

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

mcollina

lgtm with green CITGM

nodejs-github-bot · 2020-01-01T12:31:02Z

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

lpinca · 2020-01-01T13:58:02Z

@nodejs/streams

Trott · 2020-01-01T14:39:56Z

lgtm with green CITGM

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

ronag · 2020-01-01T19:24:23Z

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

mscdex · 2020-01-02T02:37:44Z

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

Performance regressions should be resolved first

ronag · 2020-01-02T11:03:12Z

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.jscallback='no'writev='no'sync='no'n=2000000**2.84%±1.80%±2.38%±3.08%streams/writable-manywrites.jscallback='no'writev='no'sync='yes'n=2000000-1.17%±1.80%±2.39%±3.12%streams/writable-manywrites.jscallback='no'writev='yes'sync='no'n=2000000**2.03%±1.23%±1.62%±2.09%streams/writable-manywrites.jscallback='no'writev='yes'sync='yes'n=2000000-1.34%±2.10%±2.82%±3.71%streams/writable-manywrites.jscallback='yes'writev='no'sync='no'n=20000001.26%±1.47%±1.95%±2.51%streams/writable-manywrites.jscallback='yes'writev='no'sync='yes'n=2000000-0.73%±2.41%±3.21%±4.21%streams/writable-manywrites.jscallback='yes'writev='yes'sync='no'n=2000000**2.30%±1.39%±1.84%±2.39%streams/writable-manywrites.jscallback='yes'writev='yes'sync='yes'n=20000001.45%±2.27%±3.04%±4.01%

BridgeAR · 2020-01-02T16:28:30Z

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

ronag · 2020-01-02T16:37:03Z

@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 · 2020-01-02T16:46:15Z

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

BridgeAR · 2020-01-02T23:01:07Z

@mscdex the performance issue is resolved. PTAL.

ronag · 2020-02-09T09:30:03Z

rebased

nodejs-github-bot · 2020-02-09T09:32:46Z

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 · 2020-02-09T11:03:16Z

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

nodejs-github-bot · 2020-02-09T13:39:36Z

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

ronag · 2020-02-09T15:36:28Z

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

ronag · 2020-02-14T09:37:15Z

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

mcollina

lgtm

nodejs-github-bot · 2020-02-14T12:11:35Z

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

ronag · 2020-02-14T13:29:33Z

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

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

ronag · 2020-02-14T13:30:50Z

Landed in 194f2e20db11

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 · 2020-02-29T23:15:09Z

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

ronag · 2020-03-10T17:27:18Z

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

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>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Dec 31, 2019

ronag commented Dec 31, 2019
View reviewed changes

Comment threadlib/_stream_writable.js Outdated

ronag force-pushed the stream-write-simplify branch 4 times, most recently from e5ca143 to 4741a70Compare January 1, 2020 11:38

ronag changed the title ~~stream: simplify Writable.write~~stream: write with 'buffer'Jan 1, 2020

mcollina approved these changes Jan 1, 2020
View reviewed changes

BridgeAR reviewed Jan 1, 2020
View reviewed changes

Comment threadlib/_stream_writable.js Outdated
Comment threadbenchmark/streams/writable-manywrites.js Outdated

BridgeAR approved these changes Jan 1, 2020
View reviewed changes

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

lpinca approved these changes Jan 1, 2020
View reviewed changes

Trott approved these changes Jan 1, 2020
View reviewed changes

mscdex previously requested changes Jan 2, 2020
View reviewed changes

ronag changed the title ~~stream: write with 'buffer'~~stream: simplify Writable.writeJan 2, 2020

ronag force-pushed the stream-write-simplify branch 2 times, most recently from 4eb510d to 7e59633Compare January 2, 2020 11:06

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 reviewed Jan 2, 2020
View reviewed changes

Comment threadlib/_stream_writable.js Outdated

ronag force-pushed the stream-write-simplify branch from e19ef3a to bf34c46Compare February 9, 2020 09:30

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

stream: simplify Writable.write
b3e3180

ronag force-pushed the stream-write-simplify branch from bf34c46 to b3e3180Compare February 9, 2020 15:34

mcollina approved these changes Feb 14, 2020
View reviewed changes

ronag closed this Feb 14, 2020

codebytere added dont-land-on-v13.x and removed dont-land-on-v13.x labels Feb 28, 2020

MylesBorins mentioned this pull request Mar 9, 2020
Multiple backports for Streams needed on v13.x #32159
Closed
7 tasks

ronag mentioned this pull request Mar 9, 2020
[v13.x backport] stream: simplify Writable.write #32162
Closed
4 tasks

ronag removed the backport-requested-v13.x label Mar 10, 2020

MylesBorins mentioned this pull request Mar 10, 2020
v13.11.0 proposal #32185
Merged

targos added the backport-requested-v12.x label Apr 20, 2020

addaleax added dont-land-on-v12.x and removed backport-requested-v12.x labels Sep 23, 2020

Uh oh!

Uh oh!

Conversation

ronag commented Dec 31, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 1, 2020

Uh oh!

lpinca commented Jan 1, 2020

Uh oh!

Trott commented Jan 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jan 1, 2020

Uh oh!

mscdex commented Jan 2, 2020

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jan 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jan 2, 2020

Uh oh!

ronag commented Jan 2, 2020

Uh oh!

Uh oh!

BridgeAR commented Jan 2, 2020

Uh oh!

BridgeAR commented Jan 2, 2020

Uh oh!

ronag commented Feb 9, 2020

Uh oh!

nodejs-github-bot commented Feb 9, 2020• edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 9, 2020

Uh oh!

nodejs-github-bot commented Feb 9, 2020

Uh oh!

ronag commented Feb 9, 2020

Uh oh!

ronag commented Feb 14, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 14, 2020

Uh oh!

ronag commented Feb 14, 2020

Uh oh!

ronag commented Feb 14, 2020

Uh oh!

codebytere commented Feb 29, 2020

Uh oh!

ronag commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

ronag commented Dec 31, 2019•
edited
Loading

Trott commented Jan 1, 2020•
edited
Loading

ronag commented Jan 2, 2020•
edited
Loading

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