stream: readable cosmetics

[ronag]

Some minor cleanup of readable.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[Trott]

@nodejs/streams

[mcollina]

LGTM

This needs a benchmark run (stream, http) to check if using let slow us down anyhow. I don’t expect any surprises here anyway.

[ronag]

@Trott can I get a benchmark CI on this? (stream, http)

[Trott]

@Trott can I get a benchmark CI on this? (stream, http)

stream: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/436/
http: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/437/

[Trott]

Let's try again. Here's the streams benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/438/

If that passes, maybe I'll try the http benchmark again....

[Trott]

19:18:21 error: patch failed: lib/_stream_readable.js:330

Seems like there's some kind of odd git issue on the benchmark machine? @nodejs/build

[rvagg]

I don't think that's a git problem. I've blown away the workspace on server so you can try running it again. If it fails again then maybe start by looking at https://github.com/nodejs/benchmarking/blob/master/experimental/benchmarks/community-benchmark/run.sh because that's what's being run and the patch that's failing is occurring in there.

[Trott]

Streams benchmark (if it works): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/439/

[ronag]

@Trott: Is this missing anything?

[nodejs-github-bot]

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

[Trott]

CI Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/452/

[Trott]

Our CI does not handle merge commits. Can you force-push it out?

[ronag]

@Trott Sorry about that. Fixed.

[Trott]

@Trott Sorry about that. Fixed.

I'm sorry about the fact that we don't handle merge commits. 😆

[nodejs-github-bot]

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

[Trott]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/

[mcollina]

02:21:53                                                        confidence improvement accuracy (*)    (**)   (***)
02:21:53  streams/creation.js kind='duplex' n=50000000                   *     -1.96 %       ±1.67%  ±2.23%  ±2.90%
02:21:53  streams/creation.js kind='readable' n=50000000                        0.62 %       ±2.18%  ±2.91%  ±3.79%
02:21:53  streams/creation.js kind='transform' n=50000000                      -0.75 %       ±2.18%  ±2.91%  ±3.78%
02:21:53  streams/creation.js kind='writable' n=50000000                       -0.33 %       ±1.80%  ±2.39%  ±3.12%
02:21:53  streams/pipe.js n=5000000                                            -1.24 %       ±5.65%  ±7.52%  ±9.79%
02:21:53  streams/pipe-object-mode.js n=5000000                                -4.77 %       ±6.32%  ±8.45% ±11.06%
02:21:53  streams/readable-bigread.js n=1000                                   -0.34 %       ±2.54%  ±3.38%  ±4.40%
02:21:53  streams/readable-bigunevenread.js n=1000                             -3.14 %      ±14.72% ±19.59% ±25.49%
02:21:53  streams/readable-boundaryread.js type='buffer' n=2000                 0.64 %       ±2.44%  ±3.25%  ±4.24%
02:21:53  streams/readable-boundaryread.js type='string' n=2000                 0.84 %       ±1.79%  ±2.39%  ±3.11%
02:21:53  streams/readable-readall.js n=5000                                    0.04 %       ±2.83%  ±3.77%  ±4.91%
02:21:53  streams/readable-unevenread.js n=1000                                 0.39 %       ±2.32%  ±3.10%  ±4.04%
02:21:53  streams/writable-manywrites.js n=2000000                              2.44 %       ±4.11%  ±5.50%  ±7.22%

I think this can land, but I would rather not use let in tight loop.

[Trott]

I think this can land, but I would rather not use let in tight loop.

@mcollina Is that for the sake of older versions of Node.js running the readable-stream module? Or is the concern relevant to the current version of Node.js/V8? Just being extra cautious? Or even though the performance difference is very small in current V8, might as well stick with what is more performant, even if it is only very minimally so?

Asking because I'm genuinely curious what your concern is (and so that I can maybe watch out for similar stuff elsewhere).

[mcollina]

@mcollina Is that for the sake of older versions of Node.js running the readable-stream module? Or is the concern relevant to the current version of Node.js/V8? Just being extra cautious?

Essentially all of this. let used to be slower in for loops, and I just want to avoid unexpected regressions because we had too many in the last while.

[ronag]

@Trott: Land or Kill? This isn't really worth more energy.