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 label Dec 20, 2019

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

addaleax reviewed Dec 21, 2019

View 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 Dec 21, 2019

This comment has been hidden.

Sign in to view

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

lundibundi reviewed Dec 23, 2019

View changes

ronag force-pushed the stream-writable-buffered2 branch from 5a4a300 to fd9c408 Compare Dec 23, 2019

ronag reviewed Dec 23, 2019

View changes

lib/_stream_writable.js Show resolved Hide resolved

wa-Nadoo reviewed Dec 23, 2019

View changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

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

ronag force-pushed the stream-writable-buffered2 branch from 709439b to 393c31f Compare Dec 25, 2019

ronag force-pushed the stream-writable-buffered2 branch from 393c31f to 8165483 Compare Jan 1, 2020

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

BridgeAR reviewed Jan 1, 2020

View 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 changes

lib/_stream_writable.js Show resolved Hide resolved

mafintosh reviewed Apr 22, 2020

View changes

lib/_stream_writable.js Show resolved Hide resolved

ronag removed the author ready label Apr 22, 2020

mafintosh reviewed Apr 22, 2020

View changes

lib/_stream_writable.js Show resolved Hide resolved

ronag added the author ready label Apr 23, 2020

This comment has been hidden.

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 issue 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

nodejs / node Public

stream: refactor Writable buffering #31046

stream: refactor Writable buffering #31046

ronag commented Dec 20, 2019 •

edited

mscdex commented Dec 21, 2019

ronag commented Dec 21, 2019

This comment has been hidden.

nodejs-github-bot commented Dec 25, 2019 •

edited by BridgeAR

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

ronag commented Apr 23, 2020

mscdex commented Apr 23, 2020 •

edited

jasnell commented Apr 23, 2020 •

edited

ronag commented Apr 23, 2020

ronag commented Apr 23, 2020

jasnell commented Apr 23, 2020

ronag commented Apr 23, 2020 •

edited

nodejs / node Public

stream: refactor Writable buffering #31046

stream: refactor Writable buffering #31046

Conversation

ronag commented Dec 20, 2019 • edited

Checklist

mscdex commented Dec 21, 2019

ronag commented Dec 21, 2019

This comment has been hidden.

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

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

ronag commented Apr 23, 2020

mscdex commented Apr 23, 2020 • edited

jasnell commented Apr 23, 2020 • edited

ronag commented Apr 23, 2020

ronag commented Apr 23, 2020

jasnell commented Apr 23, 2020

ronag commented Apr 23, 2020 • edited

ronag commented Dec 20, 2019 •

edited

nodejs-github-bot commented Dec 25, 2019 •

edited by BridgeAR

mscdex commented Apr 23, 2020 •

edited

jasnell commented Apr 23, 2020 •

edited

ronag commented Apr 23, 2020 •

edited