lib: make safe primordials safe to construct by aduh95 · Pull Request #36428 · nodejs/node · GitHub

The current constructors of Safe(Weak)?(Map|Set) classes are not "safe", they executes %ArrayIteratorPrototype%.next which may have been mutated in user-land.

That's the expected behaviour of subclasses default constructor defined the ECMAScript spec, and I think it makes sense to disable this behaviour in this case.

Checklist

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

I wonder if there's a performance impact on this? Not saying that this should be blocked if the performance degraded, just curious how big the impact would be.

I wonder if there's a performance impact on this? Not saying that this should be blocked if the performance degraded, just curious how big the impact would be.

That would be indeed interesting to mesure. My guess is it would perform better with this PR (because it no longer iterates over the argument array), but maybe V8 has some optimisation shortcuts for this kind of thing.

@nodejs/benchmarking My understanding is that benchmarking.nodejs.org is no longer updating and the Benchmarking WG is dormant and on its way to not being a thing anymore (if it hasn't already happened) but I wonder if there's a host or job that can run a good cross-set of benchmarks without taking days or weeks to run. This PR could have wide-ranging performance impacts (positive, I hope, but would like to measure).

Note that once tc39/ecma262#2216 is implemented, it will be possible to revert this.

@Trott From what I can tell https://benchmarking.nodejs.org/ is still updating?

@Trott From what I can tell https://benchmarking.nodejs.org/ is still updating?

It seems that the graphs are being updated for 10.x and 12.x It doesn't look like there have ever been results recorded there for 14.x or 15.x, and master branch/nightly doesn't seem to be recorded anymore either.

To ensure the correct length, this should use the following, which also makes it possible to remove // eslint‑disable‑line no‑useless‑constructor comment:

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

I'm curious if the regressions from #36532 stem from the changes in this PR. If so, then this PR would have quite a negative performance impact.

I wonder if there's a host or job that can run a good cross-set of benchmarks without taking days or weeks to run.

FWIW I've been doing something like that on my own dedicated (older and spare) hardware since February. The total time to run all of the benchmarks used there is currently around 22-24 hours. I think that kind of time frame is the sweet spot for detecting (committed) performance regressions in a timely manner while covering a decent number of categories.

Given that #36587 doesn't have any negative impact on buffer creation perf, it's probably safe to assume this PR won't have much impact either.

Seems OK to me, although I would love to see a benchmark out of caution. Just not sure which ones are most likely to be affected by this.

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

                                                                                   confidence improvement accuracy (*)    (**)   (***)
 module/module-loader-deep.js cache='true' files=1000 ext=''                              ***     17.25 %       ±9.69% ±12.94% ±16.95%
 module/module-loader-deep.js cache='true' files=1000 ext='.js'                                    3.60 %      ±12.18% ±16.20% ±21.09%

Results

                                                                                   confidence improvement accuracy (*)    (**)   (***)
 module/module-loader-circular.js n=10000                                                          1.54 %       ±2.89%  ±3.85%  ±5.02%
 module/module-loader-deep.js cache='false' files=1000 ext=''                                      2.31 %       ±4.40%  ±5.87%  ±7.68%
 module/module-loader-deep.js cache='false' files=1000 ext='.js'                                  -2.45 %       ±3.23%  ±4.30%  ±5.60%
 module/module-loader-deep.js cache='true' files=1000 ext=''                              ***     17.25 %       ±9.69% ±12.94% ±16.95%
 module/module-loader-deep.js cache='true' files=1000 ext='.js'                                    3.60 %      ±12.18% ±16.20% ±21.09%
 module/module-loader.js cache='false' n=1000 files=500 dir='abs' name='/'                        -1.77 %       ±3.14%  ±4.19%  ±5.49%
 module/module-loader.js cache='false' n=1000 files=500 dir='abs' name=''                          0.09 %       ±2.55%  ±3.40%  ±4.42%
 module/module-loader.js cache='false' n=1000 files=500 dir='abs' name='/index.js'                 0.33 %       ±4.46%  ±5.95%  ±7.80%
 module/module-loader.js cache='false' n=1000 files=500 dir='rel' name='/'                         1.12 %       ±4.61%  ±6.14%  ±8.01%
 module/module-loader.js cache='false' n=1000 files=500 dir='rel' name=''                          2.61 %       ±4.43%  ±5.93%  ±7.79%
 module/module-loader.js cache='false' n=1000 files=500 dir='rel' name='/index.js'                -2.22 %       ±5.68%  ±7.56%  ±9.84%
 module/module-loader.js cache='false' n=1 files=500 dir='abs' name='/'                            1.12 %       ±5.88%  ±7.83% ±10.20%
 module/module-loader.js cache='false' n=1 files=500 dir='abs' name=''                            -4.75 %       ±5.64%  ±7.50%  ±9.76%
 module/module-loader.js cache='false' n=1 files=500 dir='abs' name='/index.js'                   -1.29 %       ±4.19%  ±5.58%  ±7.28%
 module/module-loader.js cache='false' n=1 files=500 dir='rel' name='/'                            0.08 %       ±4.26%  ±5.67%  ±7.39%
 module/module-loader.js cache='false' n=1 files=500 dir='rel' name=''                            -1.30 %       ±4.65%  ±6.19%  ±8.05%
 module/module-loader.js cache='false' n=1 files=500 dir='rel' name='/index.js'                   -3.58 %       ±3.67%  ±4.89%  ±6.36%
 module/module-loader.js cache='true' n=1000 files=500 dir='abs' name='/'                          0.03 %       ±1.30%  ±1.73%  ±2.26%
 module/module-loader.js cache='true' n=1000 files=500 dir='abs' name=''                          -0.89 %       ±1.50%  ±2.00%  ±2.61%
 module/module-loader.js cache='true' n=1000 files=500 dir='abs' name='/index.js'                  0.10 %       ±1.54%  ±2.05%  ±2.67%
 module/module-loader.js cache='true' n=1000 files=500 dir='rel' name='/'                         -1.41 %       ±1.95%  ±2.62%  ±3.44%
 module/module-loader.js cache='true' n=1000 files=500 dir='rel' name=''                          -2.28 %       ±2.85%  ±3.79%  ±4.93%
 module/module-loader.js cache='true' n=1000 files=500 dir='rel' name='/index.js'                  0.31 %       ±2.88%  ±3.84%  ±5.02%
 module/module-loader.js cache='true' n=1 files=500 dir='abs' name='/'                            -3.51 %       ±5.01%  ±6.67%  ±8.69%
 module/module-loader.js cache='true' n=1 files=500 dir='abs' name=''                              0.85 %       ±4.77%  ±6.43%  ±8.53%
 module/module-loader.js cache='true' n=1 files=500 dir='abs' name='/index.js'                    -0.92 %       ±3.98%  ±5.30%  ±6.91%
 module/module-loader.js cache='true' n=1 files=500 dir='rel' name='/'                            -0.48 %       ±4.09%  ±5.44%  ±7.08%
 module/module-loader.js cache='true' n=1 files=500 dir='rel' name=''                             -1.50 %       ±2.99%  ±4.00%  ±5.26%
 module/module-loader.js cache='true' n=1 files=500 dir='rel' name='/index.js'                     0.97 %       ±4.88%  ±6.52%  ±8.52%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 29 comparisons, you can thus
expect the following amount of false-positive results:
  1.45 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.29 false positives, when considering a   1% risk acceptance (**, ***),
  0.03 false positives, when considering a 0.1% risk acceptance (***)

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

Landed in 997f2fc...40fc395

PR-URL: #36428 Reviewed-By: Rich Trott <rtrott@gmail.com>