module: synchronously load most ES modules - #62530
GeoffreyBooth wants to merge 8 commits into
Conversation
|
Review requested:
|
2bb88f9 to
9a7728c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62530 +/- ##
==========================================
+ Coverage 89.99% 90.01% +0.01%
==========================================
Files 785 785
Lines 268715 269377 +662
Branches 51200 51313 +113
==========================================
+ Hits 241823 242472 +649
+ Misses 17432 17401 -31
- Partials 9460 9504 +44
🚀 New features to boost your workflow:
|
|
The PR description says there is an improvement but the number shows a regression? Although I don't think "a flat graph importing hundreds/thousands of modules" is a representative use case, so a regression probably doesn't matter all that much anyway. A more typical graph probably consists of a lot of nodes each with a dozen or so imports.. |
9a7728c to
0aa5399
Compare
My apologies, I ran the benchmark where the new binary was built with
I updated the benchmark to create a tree with 10 imports per node, as large as necessary to match the desired size of the graph. I updated the PR description with the new results. Basically, they’re inconclusive, as you might expect for such a small change. Promises just don’t add much overhead. |
2958720 to
e5294fb
Compare
JakobJingleheimer
left a comment
There was a problem hiding this comment.
LGTM and sounds right. Would be great to get 19 promises → 1 😁
e5294fb to
f8823ee
Compare
|
This pull request has been marked as stale due to 90 days of inactivity. |
f8823ee to
ae550d4
Compare
|
Benchmark GHA (esm / startup-esm-graph.js): https://github.com/nodejs/node/actions/runs/34718545834 Results
Benchmark results:
|
Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
…th a branching factor of 10 Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Previously the pause was set up in ModuleLoader, which forced the entry point onto the async ModuleJob and paused during instantiation, landing the debugger on internal frames or on a dependency rather than on the entry point itself. Move the handling into ModuleJobSync, where the root module is actually evaluated. Direct dependencies are pre-evaluated first so that V8 skips them and stops at the entry point's first executable line. Both the synchronous path and the top-level await fallback are covered. Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
An uncaught error thrown by an ES module entry point now escapes through ModuleJobSync's evaluateSync() instead of the async triggerUncaughtException path in run_main, so the internal frame shown above the error changes. Signed-off-by: Geoffrey Booth <webadmin@geoffreybooth.com>
ae550d4 to
f1dad35
Compare
|
Benchmark GHA (esm / startup-esm-graph): https://github.com/nodejs/node/actions/runs/34828910699 Results
Benchmark results:
|
|
@joyeecheung Do you want to take another look at this one? |
Building on #55782, this PR uses the path @joyeecheung created for
require(esm)to synchronously resolve and load all ES modules that lack top-levelawait, which is the vast majority of modules. The sync path is used when no async loader hooks,--importflags, or--inspect-brkare active; it falls back to the existing async path otherwise. Top-levelawaitpresence can only be determined after the module graph is instantiated, so if TLA is detected the already-instantiated graph falls back to async evaluation. In all cases the behavior is identical to the existing async path.On current
main, an ES module graph generates 14 + 5N promises for N modules; so 19 promises for a single module graph (one entry point that doesn’t import anything), 24 promises if that entry point imports one file, 29 promises for a three-module graph and so on.In this PR, only one promise is created regardless of graph size: the low-level V8
module.evaluate()call that happens withinmodule.evaluateSync(), where an immediately-resolved promise is created even for modules that don’t have top-levelawait. But still, it’s only one promise for an entire application, no matter how big the app is.This PR adds a benchmark that focuses on the module loading flow that this PR improves:
So basically it’s within the margin of error.