stream: refactor clearBuffer and writableState

[anonrig]

This pull request refactors clearBuffer function and removes the unnecessary typeof isDuplex check on WritableState class.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[mscdex]

Doesn't seem to be reproducible. A re-run of the benchmarks shows:

streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=2000000              *     -1.43 %       ±1.12% ±1.49% ±1.95%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=2000000                   -1.73 %       ±3.29% ±4.38% ±5.71%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=2000000                    1.52 %       ±2.90% ±3.86% ±5.02%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=2000000                   2.19 %       ±3.46% ±4.60% ±6.00%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=2000000             *     -2.91 %       ±2.53% ±3.38% ±4.44%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=2000000                  -0.08 %       ±3.22% ±4.29% ±5.58%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=2000000                   2.97 %       ±5.28% ±7.08% ±9.34%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=2000000                 -2.02 %       ±4.66% ±6.21% ±8.11%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=2000000                   -0.63 %       ±1.29% ±1.71% ±2.23%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=2000000                   2.60 %       ±3.30% ±4.44% ±5.88%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=2000000                   1.84 %       ±2.69% ±3.61% ±4.76%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=2000000                  1.35 %       ±4.10% ±5.50% ±7.26%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=2000000                   0.15 %       ±1.46% ±1.94% ±2.53%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=2000000                  0.55 %       ±1.83% ±2.44% ±3.18%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=2000000                 -0.10 %       ±2.19% ±2.91% ±3.79%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=2000000                 2.10 %       ±4.64% ±6.22% ±8.20%

[anonrig]

Doesn't seem to be reproducible. A re-run of the benchmarks shows:

Thanks for the update @mscdex. Do you have any particular reason for this flakiness? I've added a new commit and re-run the benchmark CI. If it's still the same, I'll update the description and remove the references to the benchmark results.

[mscdex]

Do you have any particular reason for this flakiness?

No, but it's always best to re-run the benchmarks several times to ensure consistent results.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ronag]

This is unnecessary change

[ronag]

I don't understand why this is better?

[anonrig]

Unnecessary length operation was done before state.bufferProcessing assignment is done. This change eliminates that.

[ronag]

I still don't see it. This is actually slower for the case where bufferedLength is 0... since it unnecessarily sets and unsets bufferProcessing.