module: synchronously load most ES modules

[GeoffreyBooth]

Building on #55782, this PR uses the path @joyeecheung created for require(esm) to synchronously resolve and load all ES modules that lack top-level await, which is the vast majority of modules. The sync path is used when no async loader hooks, --import flags, or --inspect-brk are active; it falls back to the existing async path otherwise. Top-level await presence 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 within module.evaluateSync(), where an immediately-resolved promise is created even for modules that don’t have top-level await. 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:

                                              confidence improvement accuracy (*)   (**)  (***)
esm/startup-esm-graph.js n=100 modules='0250'                 0.71 %       ±3.39% ±4.47% ±5.74%
esm/startup-esm-graph.js n=100 modules='0500'                 0.45 %       ±3.15% ±4.15% ±5.33%
esm/startup-esm-graph.js n=100 modules='1000'                 1.96 %       ±3.19% ±4.21% ±5.40%
esm/startup-esm-graph.js n=100 modules='2000'                 1.08 %       ±3.12% ±4.11% ±5.28%

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

So basically it’s within the margin of error.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/performance

[codecov]

Codecov Report

❌ Patch coverage is 94.25287% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.35%. Comparing base (2e3daf6) to head (f8823ee).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/module_job.js	91.93%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62530      +/-   ##
==========================================
+ Coverage   90.34%   90.35%   +0.01%     
==========================================
  Files         730      730              
  Lines      234359   234459     +100     
  Branches    43923    43935      +12     
==========================================
+ Hits       211720   211836     +116     
+ Misses      14361    14340      -21     
- Partials     8278     8283       +5     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/loader.js	99.53% <100.00%> (-0.18%)	⬇️
lib/internal/modules/run_main.js	97.82% <100.00%> (+0.17%)	⬆️
lib/internal/modules/esm/module_job.js	95.23% <91.93%> (-1.16%)	⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[joyeecheung]

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..

[joyeecheung]

Why is --inspect-brk an exception?

[GeoffreyBooth]

In module_job.js there’s special handling for --inspect-brk:

node/lib/internal/modules/esm/module_job.js

Lines 321 to 326 in a9ac9b1

	if (!hasPausedEntry && this.inspectBrk) {
	hasPausedEntry = true;
	const initWrapper = internalBinding('inspector').callAndPauseOnStart;
	initWrapper(this.module.instantiate, this.module);
	} else {
	this.module.instantiate();

But further down in the file in the equivalent ModuleJobSync, there’s no such handling that I can see.

[GeoffreyBooth]

I mimicked the pattern so that --inspect-brk is supported now.

[GeoffreyBooth]

The PR description says there is an improvement but the number shows a regression?

My apologies, I ran the benchmark where the new binary was built with --node-builtin-modules-path "$(pwd)", so it was running much slower due to loading the built-in modules from the current path. When I ran the benchmark again with a properly built binary, the results are nearly indistinguishable from main.

A more typical graph probably consists of a lot of nodes each with a dozen or so imports.

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.

[JakobJingleheimer]

LGTM and sounds right. Would be great to get 19 promises → 1 😁

[joyeecheung]

I don't think this is the right place to pause: it should pause at the evaluation of the root module. Otherwise users would be confused that now it pauses on some internals when they use this with ESM.

[mcollina]

lgtm

[nodejs-github-bot]

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