stream: refactor Writable buffering #31046

ronag · 2019-12-20T22:13:34Z

Another try on this, now without the reduced performance:

 streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000           ***      8.49 %       ±1.42% ±1.88% ±2.42%
 streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000                   0.28 %       ±1.38% ±1.84% ±2.40%
 streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000          ***     36.63 %       ±3.10% ±4.09% ±5.28%
 streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000                 -0.72 %       ±2.57% ±3.42% ±4.46%
 streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000          ***      6.75 %       ±1.16% ±1.54% ±1.99%
 streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000                 -0.67 %       ±1.86% ±2.48% ±3.23%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000         ***     10.91 %       ±1.75% ±2.33% ±3.02%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000                -1.70 %       ±2.71% ±3.61% ±4.70%

Benchmarks from #31066

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

lib/_stream_writable.js

mscdex · 2019-12-21T06:28:00Z

Might be a good idea to check CITGM to check for dependencies on the old internal state properties?

ronag · 2019-12-21T08:22:37Z

@mscdex: I noticed that using symbol accessors seems slower than regular properties. Is this something we are aware of or am I just unlucky in my benchmarks?

benchmark/streams/writable-manywrites.js

lib/_stream_writable.js

nodejs-github-bot · 2019-12-25T09:41:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27931/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2158/

ronag · 2020-01-01T12:05:00Z

rebased to fix conflicts

BridgeAR

This looks very promising!

lib/_stream_writable.js

BridgeAR

LGTM with a green CITGM.

lib/_stream_writable.js

mafintosh · 2020-04-23T01:15:29Z

LGTM on green canary

ronag · 2020-04-23T06:38:28Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2357/

ronag · 2020-04-23T14:07:30Z

CITGM OK. ws failure is expected and not related to this PR.

Refactors buffering in Writable to use an array instead of a linked list. PR-URL: #31046 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ronag · 2020-04-23T17:52:53Z

Landed in d8c57cb

mscdex · 2020-04-23T18:29:57Z

There seems to have been some significant performance regressions with the latest changes:

                                                                                confidence improvement accuracy (*)   (**)  (***)
streams/creation.js kind='duplex' n=50000000                                             *      2.44 %       ±1.87% ±2.49% ±3.25%
streams/creation.js kind='readable' n=50000000                                                  0.57 %       ±3.10% ±4.12% ±5.36%
streams/creation.js kind='transform' n=50000000                                          *      3.44 %       ±2.81% ±3.73% ±4.86%
streams/creation.js kind='writable' n=50000000                                         ***      8.08 %       ±3.53% ±4.70% ±6.12%
streams/pipe.js n=5000000                                                                       1.95 %       ±3.32% ±4.43% ±5.81%
streams/pipe-object-mode.js n=5000000                                                   **      4.76 %       ±3.29% ±4.40% ±5.78%
streams/readable-bigread.js n=1000                                                             -0.19 %       ±3.43% ±4.57% ±5.97%
streams/readable-bigunevenread.js n=1000                                                        0.49 %       ±2.05% ±2.73% ±3.56%
streams/readable-boundaryread.js type='buffer' n=2000                                           0.98 %       ±2.30% ±3.06% ±3.99%
streams/readable-boundaryread.js type='string' n=2000                                           0.32 %       ±1.20% ±1.60% ±2.08%
streams/readable-readall.js n=5000                                                              1.62 %       ±2.92% ±3.89% ±5.09%
streams/readable-unevenread.js n=1000                                                          -1.05 %       ±2.03% ±2.70% ±3.53%
streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000           ***     11.18 %       ±1.51% ±2.01% ±2.62%
streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000          ***    -18.20 %       ±4.62% ±6.15% ±8.01%
streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000          ***     24.39 %       ±2.88% ±3.85% ±5.05%
streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000                  0.52 %       ±5.40% ±7.23% ±9.49%
streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000          ***      8.15 %       ±1.76% ±2.35% ±3.05%
streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000         ***    -22.02 %       ±3.14% ±4.20% ±5.50%
streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000         ***      6.31 %       ±2.80% ±3.74% ±4.87%
streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000                -1.93 %       ±3.50% ±4.66% ±6.07%

jasnell · 2020-04-23T19:03:00Z

@ronag .... it would appear that maybe one of the handful of streams related PRs you just landed may be having some CI issues: https://ci.nodejs.org/job/node-test-commit-linux/34504/

test-stream-pipeline consistently failing on Linux...

ronag · 2020-04-23T19:10:44Z

@ronag .... it would appear that maybe one of the handful of streams related PRs you just landed may be having some CI issues: ci.nodejs.org/job/node-test-commit-linux/34504

That's strange. I'm on it.

ronag · 2020-04-23T19:15:22Z

All of them have green CI, so it must be that one of them made the other fail.

jasnell · 2020-04-23T19:22:05Z

Entirely possible. Just ran the tests a second time (with a different unrelated PR) and seeing the same failures. https://ci.nodejs.org/job/node-test-commit-linux/34506/

ronag · 2020-04-23T19:24:43Z

Entirely possible. Just ran the tests a second time (with a different unrelated PR) and seeing the same failures. https://ci.nodejs.org/job/node-test-commit-linux/34506/

Yep, Working towards a PR to resolve it.

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.

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>

Refactors buffering in Writable to use an array instead of a linked list. PR-URL: #31046 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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>

Refactors buffering in Writable to use an array instead of a linked list. PR-URL: #31046 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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>

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

ronag force-pushed the stream-writable-buffered2 branch 4 times, most recently from 24f8b15 to 0c9b7e7 Compare December 20, 2019 22:20

addaleax reviewed Dec 21, 2019

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

ronag mentioned this pull request Dec 21, 2019

stream: improve write performance #30736

Closed

4 tasks

ronag force-pushed the stream-writable-buffered2 branch from e6c3b63 to 76eb9ec Compare December 21, 2019 08:21

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-writable-buffered2 branch 5 times, most recently from bd5e468 to 3df92f4 Compare December 23, 2019 15:27

lundibundi reviewed Dec 23, 2019

View reviewed changes

ronag force-pushed the stream-writable-buffered2 branch from 5a4a300 to fd9c408 Compare December 23, 2019 19:45

ronag commented Dec 23, 2019

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

wa-Nadoo reviewed Dec 23, 2019

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-writable-buffered2 branch from a7d1723 to 393c31f Compare December 23, 2019 23:20

ronag force-pushed the stream-writable-buffered2 branch from 709439b to 393c31f Compare December 25, 2019 10:28

ronag force-pushed the stream-writable-buffered2 branch from 393c31f to 8165483 Compare January 1, 2020 12:04

ronag force-pushed the stream-writable-buffered2 branch 3 times, most recently from 5b9a81c to b161293 Compare January 1, 2020 14:23

BridgeAR reviewed Jan 1, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

lib/_stream_writable.js Show resolved Hide resolved

BridgeAR approved these changes Jan 1, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

mafintosh reviewed Apr 22, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 22, 2020

mafintosh reviewed Apr 22, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 23, 2020

This comment has been minimized.

Sign in to view

ronag closed this Apr 23, 2020

ronag mentioned this pull request Apr 23, 2020

stream: fix sync write perf regression #33032

Closed

4 tasks

ronag added a commit to nxtedition/node that referenced this pull request Apr 23, 2020

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.

BethGriggs mentioned this pull request Apr 27, 2020

v14.1.0 proposal #33103

Merged

BridgeAR added the backport-requested-v13.x label Apr 28, 2020

targos added the backport-blocked-v12.x label Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: refactor Writable buffering #31046

stream: refactor Writable buffering #31046

ronag commented Dec 20, 2019 •

edited

Loading

mscdex commented Dec 21, 2019

ronag commented Dec 21, 2019

This comment has been minimized.

nodejs-github-bot commented Dec 25, 2019 •

edited by BridgeAR

Loading

ronag commented Jan 1, 2020

BridgeAR left a comment

BridgeAR left a comment

mafintosh commented Apr 23, 2020

ronag commented Apr 23, 2020

ronag commented Apr 23, 2020

This comment has been minimized.

ronag commented Apr 23, 2020

mscdex commented Apr 23, 2020 •

edited

Loading

jasnell commented Apr 23, 2020 •

edited

Loading

ronag commented Apr 23, 2020

ronag commented Apr 23, 2020

jasnell commented Apr 23, 2020

ronag commented Apr 23, 2020 •

edited

Loading

stream: refactor Writable buffering #31046

stream: refactor Writable buffering #31046

Conversation

ronag commented Dec 20, 2019 • edited Loading

Checklist

mscdex commented Dec 21, 2019

ronag commented Dec 21, 2019

This comment has been minimized.

nodejs-github-bot commented Dec 25, 2019 • edited by BridgeAR Loading

ronag commented Jan 1, 2020

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

mafintosh commented Apr 23, 2020

ronag commented Apr 23, 2020

ronag commented Apr 23, 2020

This comment has been minimized.

ronag commented Apr 23, 2020

mscdex commented Apr 23, 2020 • edited Loading

jasnell commented Apr 23, 2020 • edited Loading

ronag commented Apr 23, 2020

ronag commented Apr 23, 2020

jasnell commented Apr 23, 2020

ronag commented Apr 23, 2020 • edited Loading

ronag commented Dec 20, 2019 •

edited

Loading

nodejs-github-bot commented Dec 25, 2019 •

edited by BridgeAR

Loading

mscdex commented Apr 23, 2020 •

edited

Loading

jasnell commented Apr 23, 2020 •

edited

Loading

ronag commented Apr 23, 2020 •

edited

Loading