http2: refactor to avoid unsafe array iteration

[aduh95]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[rchougule]

for..of loop has higher performance as compared to forEach (correct me if I am wrong)...
So are we still fine going with forEach for this specific case here?

P.S. Just started with open-source reviews and was trying to learn from other PRs.

[Trott]

Yes, for..of is more performant which is why it was chosen to begin with. It's also why this pull request will need a benchmark run before it can be merged. The problem with for..of is that it is vulnerable to prototype pollution attacks. If some third party module deep in your dependencies decides to inject some code in Array.prototype[Symbol.iterator], then for..of will happily run that code. That may or may not be a problem for the end user's code--there are legitimate use cases (like profiling) for this sort of so-called "monkeypatching", but we don't want Node.js core to run the altered code.

[Trott]

LGTM if benchmarks are OK

[tniessen]

Isn't this also "vulnerable" to prototype pollution "attacks," if we really want to consider that "unsafe?" I mean, one could define a getter on the Array.prototype that returns something "malicious."

I'm really not sure if all this complexity and illegibility are worth the minimal benefit.

[aduh95]

Defining a 0 property on the Array.prototype would have effect only on empty arrays, right? args here is created by the spread operator, so I think this code would actually be safe even in case of Array.prototype pollution.

Object.defineProperty(Array.prototype, '0', {get(){return 9}, set(v){/* no op */}})
((t, ...args)=>args[0])(1, 2, 3) === 2 // true

I don't think we are concerned about code being "malicious" anyway, what we are trying to achieve with the move to primordials is to allow users to monkey-patch the built-in objects (for debugging purposes, for tinkering, etc.) without having errors thrown by Node.js internals.

[tniessen]

so I think this code would actually be safe even in case of Array.prototype pollution.

Might be, I am not sure.

without having errors thrown by Node.js internals.

I understand the motivation, but I am not sure it's worth the cost to maintainability, readability etc.

[aduh95]

To address the maintainability concern, would you prefer if we added a linter rule to forbid array destructuring assignment?

[aduh95]

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

[ZYSzys]

Benchmark CI: ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/887 (queued)

I'm a bit of -1 since benchmarks showed some regression.

21:22:36  http2/headers.js nheaders=0 n=1000                                                                              **     -3.60 %       ±2.48%  ±3.30%  ±4.30%
21:22:36  http2/headers.js nheaders=1000 n=1000                                                                            *     -2.02 %       ±1.59%  ±2.12%  ±2.76%
21:22:36  http2/headers.js nheaders=100 n=1000                                                                             *      4.27 %       ±3.56%  ±4.74%  ±6.18%
21:22:36  http2/headers.js nheaders=10 n=1000                                                                            ***     -5.31 %       ±2.11%  ±2.81%  ±3.66%

[aduh95]

Another benchmark run confirmed there's a perf regression for low values of nheaders, investigating…

                                       confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js nheaders=0 n=1000           ***     -4.71 %       ±2.51% ±3.33% ±4.31%
 http2/headers.js nheaders=1000 n=1000                -1.72 %       ±2.24% ±2.97% ±3.84%
 http2/headers.js nheaders=100 n=1000                  1.01 %       ±3.03% ±4.01% ±5.18%
 http2/headers.js nheaders=10 n=1000          ***     -5.58 %       ±1.71% ±2.27% ±2.93%

[aduh95]

Replacing <array>.forEach with for(;;) loop fixed the perf regressions. If no one objects, I'm planning to land this later this week, I think the perf looks OK now.

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

                                       confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js nheaders=0 n=1000                   -0.43 %       ±2.00% ±2.65% ±3.42%
 http2/headers.js nheaders=1000 n=1000        ***     -3.28 %       ±1.87% ±2.48% ±3.20%
 http2/headers.js nheaders=100 n=1000                  0.02 %       ±2.95% ±3.91% ±5.05%
 http2/headers.js nheaders=10 n=1000           **     -2.32 %       ±1.71% ±2.26% ±2.92%

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Landed in 0fd9bbb