stream: refactor Writable buffering

[ronag]

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

[mscdex]

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

[ronag]

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

[nodejs-github-bot]

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]

rebased to fix conflicts

[BridgeAR]

This looks very promising!

[BridgeAR]

LGTM with a green CITGM.

[mafintosh]

LGTM on green canary

[ronag]

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

[ronag]

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

[ronag]

Landed in d8c57cb

[mscdex]

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]

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

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

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

[jasnell]

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]

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.