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 <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

Refactors buffering in Writable to use an array instead of a linked list. PR-URL: #31046 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

Refactors buffering in Writable to use an array instead of a linked list. PR-URL: #31046 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

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 0c9b7e7Compare December 20, 2019 22:20

addaleax reviewed Dec 21, 2019
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide 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 76eb9ecCompare 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 3df92f4Compare December 23, 2019 15:27

lundibundi reviewed Dec 23, 2019
View reviewed changes

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

ronag commented Dec 23, 2019
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved

wa-Nadoo reviewed Dec 23, 2019
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved

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

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

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

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

BridgeAR reviewed Jan 1, 2020
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved
lib/_stream_writable.js Outdated Show resolvedHide resolved

BridgeAR approved these changes Jan 1, 2020
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved

mafintosh reviewed Apr 22, 2020
View reviewed changes

lib/_stream_writable.jsShow resolvedHide 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.jsShow resolvedHide 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

Uh oh!

stream: refactor Writable buffering#31046

stream: refactor Writable buffering #31046

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

Uh oh!

mscdex commented Dec 21, 2019

Uh oh!

ronag commented Dec 21, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 25, 2019• edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jan 1, 2020

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafintosh commented Apr 23, 2020

Uh oh!

ronag commented Apr 23, 2020

Uh oh!

ronag commented Apr 23, 2020

Uh oh!

This comment has been minimized.

ronag commented Apr 23, 2020

Uh oh!

mscdex commented Apr 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Apr 23, 2020

Uh oh!

ronag commented Apr 23, 2020

Uh oh!

jasnell commented Apr 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

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