module: synchronously load most ES modules - #62530
GeoffreyBooth wants to merge 11 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 90.37% 90.39% +0.01%
==========================================
Files 790 792 +2
Lines 274809 275501 +692
Branches 52631 52797 +166
==========================================
+ Hits 248356 249031 +675
+ Misses 16927 16878 -49
- Partials 9526 9592 +66
🚀 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:
|
ae550d4 to
f1dad35
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Benchmark GHA (esm / startup-esm-graph): https://github.com/nodejs/node/actions/runs/34828910699 Results
Benchmark results:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
@joyeecheung Do you want to take another look at this one? |
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>
f1dad35 to
5397839
Compare
|
https://ci.nodejs.org/job/node-test-commit-windows-fanned/80823/ |
…indows/Linux agnostic
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.