GeezerVison

Review requested:

• @nodejs/modules
• @nodejs/net
• @nodejs/startup

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

                                                                                  confidence improvement accuracy (*)   (**)  (***)
misc/startup.js mode='process' script='benchmark/fixtures/require-builtins' dur=1        ***     -4.80 %       ±2.32% ±3.10% ±4.05%
misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***     -9.93 %       ±2.27% ±3.01% ±3.93%
misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***     16.66 %       ±1.78% ±2.37% ±3.09%
misc/startup.js mode='worker' script='benchmark/fixtures/require-builtins' dur=1         ***     11.31 %       ±4.03% ±5.38% ±7.04%
misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***      6.71 %       ±2.01% ±2.68% ±3.49%
misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***     36.80 %       ±2.69% ±3.59% ±4.69%

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

this is awesome, amazing work @joyeecheung

From the CI:

I think it's okay/predictable for the require-builtins and require-cachable benchmarks of the main process to regress a bit in favor of semicolon and the worker benchmarks. Most applications really only need a subset of the builtins, instead of all of them, and the modules removed here IMO either only work for a specific type of application (but not others), or there are multiple alternatives of them so it makes more sense to only pay the cost of one of them (when the user actually need them) instead of eagerly loading all the alternatives and paying for the extra cost.

17:49:10                                                                                   confidence improvement accuracy (*)   (**)  (***)
17:49:11 misc/startup.js mode='process' script='benchmark/fixtures/require-builtins' dur=1        ***     -4.80 %       ±2.32% ±3.10% ±4.05%
17:49:11 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***     -9.93 %       ±2.27% ±3.01% ±3.93%
17:49:11 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***     16.66 %       ±1.78% ±2.37% ±3.09%
17:49:11 misc/startup.js mode='worker' script='benchmark/fixtures/require-builtins' dur=1         ***     11.31 %       ±4.03% ±5.38% ±7.04%
17:49:11 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***      6.71 %       ±2.01% ±2.68% ±3.49%
17:49:11 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***     36.80 %       ±2.69% ±3.59% ±4.69%

cc @nodejs/performance

Can you please summarize the overall benefit of this change? I assume it produces a faster startup (?) and how much faster does it get?

I assume it produces a faster startup (?) and how much faster does it get?

@GeoffreyBooth did you see #45659 (comment)?

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

@joyeecheung Can you fix the js lint errors?

/home/runner/work/node/node/lib/internal/util.js
[10](https://github.com/nodejs/node/actions/runs/3575782975/jobs/6012760242#step:5:11)
Error:   508:14  error  Multiple spaces found before '='  no-multi-spaces
[11](https://github.com/nodejs/node/actions/runs/3575782975/jobs/6012760242#step:5:12)

[12](https://github.com/nodejs/node/actions/runs/3575782975/jobs/6012760242#step:5:13)
✖ 1 problem (1 error, 0 warnings)
[13](https://github.com/nodejs/node/actions/runs/3575782975/jobs/6012760242#step:5:14)
  1 error and 0 warnings potentially fixable with the `--fix` option.

@GeoffreyBooth did you see #45659 (comment)?

Yes, but I don’t know how to interpret that. Is the semicolon benchmark a proxy for “start up and do nothing”? So base startup is 17% faster?

@GeoffreyBooth did you see #45659 (comment)?

Yes, but I don’t know how to interpret that. Is the semicolon benchmark a proxy for “start up and do nothing”? So base startup is 17% faster?

@GeoffreyBooth Startup benchmark only benchmarks the startup time of a Node.js application. Loading a semicolon is a way of not calling any of the node applications, and benchmark the load time of all essential packages in Node.js (which directly effects the load time)

Startup benchmark only benchmarks the startup time of a Node.js application. Loading a semicolon is a way of not calling any of the node applications, and benchmark the load time of all essential packages in Node.js (which directly effects the load time)

Okay, so is this a benchmark of how long Node itself takes to startup, when loading an application that does nothing? So therefore it’s fair to say that for the “noop app” case, this PR makes Node start up 17% faster?

So therefore it’s fair to say that for the “noop app” case, this PR makes Node start up 17% faster?

The benchmark is 99.9% confident that this PR improves the performance from 16.66 % - 3.09% up to 16.66 % + 3.09%. 17% is in this range, but it can be any other value in this range. Also, the benchmark is run on a specific machine, on a specific platform, so if you run it again on a different machine you are very likely to get a different figure. The prudent takeaway is "this PR makes Node.js start up faster", trying to be more specific is more likely to get you away from the truth than closer to it.

trying to be more specific is more likely to get you away from the truth than closer to it.

I think we should have some language that’s friendly enough that we can put it in the release notes. Making the runtime start 17-ish percent faster is a headline; so if that’s too imprecise, let’s come up with an alternative summary that’s still correct but not pedantic. @joyeecheung is it possible to pull the “semicolon” benchmark numbers for all the machines/platforms we test on? So that we can phrase a release note like “The node binary now starts up X% to Y% faster when loading an empty application, as measured on X CI machines covering X platforms.”

“The node binary now starts up X% to Y% faster when loading an empty application, as measured on X CI machines covering X platforms.”

That's not how I would frame this PR, because this is only fixing a regression caused by being too careless about loading non-essential modules before, the startup is still slower compared to previous release lines. Part of the regression can be attributed to the "bloating" of the core - many of the modules loaded here were only added in recent releases, on main out/Release/node -p "process.moduleLoadList.join('\n')" | wc -l is 186, while it was ~136 for v16, ~88 for v14, this PR takes it back to 97. Another suspect can be the use of primordials, it could slow down the creation of vm contexts even with the help of the snapshots (-90% without snapshot, -30% with snapshot, from #44252 (comment)) . Even node --version, which involves no JS execution, has been much slower than before, which I am still looking into.

@danielleadams I think this probably needs more of a rewrite for v18.x instead of a simple backport. Will do it after the new years holidays

This would need a non-semver-major version of #44483 to backport properly

@joyeecheung is it correct that you actually only need to pull from #44483 the changes made in lib/internal/validators.js and some of those made in lib/internal/bootstrap/browser.js? If so, that seems certainly feasible to do a non-semver-major backport. Otherwise, it seems very hard to backport anything else from that PR without risking breaking something somewhere.

@aduh95 Not sure if this explanation makes sense - I need to pull in the changes to the module graph, ideally including some of the refactoring that reduces the conflicts