bootstrap: lazy load non-essential modules - #45659
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1241/ Resultsconfidence 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 (***) |
Sorry, something went wrong.
|
|
||
| const { | ||
| newReadableStreamFromStreamBase, | ||
| } = require('internal/webstreams/adapters'); |
There was a problem hiding this comment.
The internal require() is actually just one map load (to see if the module is already loaded) + one property load (state check for circular dependencies) for modules that are already loaded, so I think it's fine to just inline these as they are followed by more expensive operations that will shadow the overhead of require()
Sorry, something went wrong.
|
this is awesome, amazing work @joyeecheung |
Sorry, something went wrong.
|
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% |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
anonrig
left a comment
There was a problem hiding this comment.
fantastic work. lgtm!
Sorry, something went wrong.
|
cc @nodejs/performance |
Sorry, something went wrong.
daeyeon
left a comment
There was a problem hiding this comment.
Amazing the number of reduced modules bootstrapped. Thanks for this great work!
Sorry, something went wrong.
|
Can you please summarize the overall benefit of this change? I assume it produces a faster startup (?) and how much faster does it get? |
Sorry, something went wrong.
@GeoffreyBooth did you see #45659 (comment)? |
Sorry, something went wrong.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
@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) |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
JakobJingleheimer
left a comment
There was a problem hiding this comment.
Huzzah 🙌
Sorry, something went wrong.
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.” |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
The internal `require()` is actually just one map load (to see if the module is already loaded) + one property load (state check for circular dependencies) for modules that are already loaded. Refs: #45659 (comment) PR-URL: #45809 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
|
@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 |
Sorry, something went wrong.
The internal `require()` is actually just one map load (to see if the module is already loaded) + one property load (state check for circular dependencies) for modules that are already loaded. Refs: #45659 (comment) PR-URL: #45809 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
The internal `require()` is actually just one map load (to see if the module is already loaded) + one property load (state check for circular dependencies) for modules that are already loaded. Refs: #45659 (comment) PR-URL: #45809 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
|
This would need a non-semver-major version of #44483 to backport properly |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
It turns out that even with startup snapshots, there is a non-trivial overhead for loading internal modules. This patch makes the loading of the non-essential modules lazy again. Caveat: we have to make some of the globals lazily-loaded too, so the WPT runner is updated to test what the state of the global scope is after the globals are accessed (and replaced with the loaded value). PR-URL: nodejs#45659 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im>
It turns out that even with startup snapshots, there is a non-trivial overhead for loading internal modules. This patch makes the loading of the non-essential modules lazy again. Caveat: we have to make some of the globals lazily-loaded too, so the WPT runner is updated to test what the state of the global scope is after the globals are accessed (and replaced with the loaded value). PR-URL: nodejs#45659 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im>
It turns out that even with startup snapshots, there is a non-trivial overhead for loading internal modules. This patch makes the loading of the non-essential modules lazy again. Caveat: we have to make some of the globals lazily-loaded too, so the WPT runner is updated to test what the state of the global scope is after the globals are accessed (and replaced with the loaded value). PR-URL: #45659 Backport-PR-URL: #46425 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im>
It turns out that even with startup snapshots, there is a non-trivial overhead for loading internal modules. This patch makes the loading of the non-essential modules lazy again. Caveat: we have to make some of the globals lazily-loaded too, so the WPT runner is updated to test what the state of the global scope is after the globals are accessed (and replaced with the loaded value). PR-URL: nodejs/node#45659 Backport-PR-URL: nodejs/node#46425 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im>
It turns out that even with startup snapshots, there is a non-trivial overhead for loading internal modules. This patch makes the loading of the non-essential modules lazy again. Caveat: we have to make some of the globals lazily-loaded too, so the WPT runner is updated to test what the state of the global scope is after the globals are accessed (and replaced with the loaded value). PR-URL: nodejs/node#45659 Backport-PR-URL: nodejs/node#46425 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im>
benchmark: make benchmarks runnable in older versions of Node.js
In addition make it runnable with --no-browser-globals
bootstrap: lazy load non-essential modules
It turns out that even with startup snapshots, there is a non-trivial
overhead for loading internal modules. This patch makes the loading
of the non-essential modules lazy again.
Caveat: we have to make some of the globals lazily-loaded too,
so the WPT runner is updated to test what the state of the global
scope is after the globals are accessed (and replaced with the
loaded value).