bootstrap: lazy load non-essential modules #45659
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1241/ Results |
|
|
||
| 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()
|
this is awesome, amazing work @joyeecheung |
|
From the CI: I think it's okay/predictable for the |
|
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? |
@GeoffreyBooth did you see #45659 (comment)? |
|
@joyeecheung Can you fix the js lint errors? |
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) |
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? |
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. |
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 |
GeoffreyBooth
added
notable-change
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 |
|
@joyeecheung this caused merge conflicts when landing in v18.x-staging. Do you mind creating a backport PR? |
danielleadams
added
the
backport-requested-v18.x
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
|
@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 |
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
|
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 |
|
@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 |
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Tierney Cyren <[email protected]>
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).