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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.73%. Comparing base (5ff1eab) to head (e5294fb).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62530      +/-   ##
==========================================
+ Coverage   89.69%   89.73%   +0.03%     
==========================================
  Files         695      695              
  Lines      214417   214505      +88     
  Branches    41059    41071      +12     
==========================================
+ Hits       192321   192476     +155     
+ Misses      14156    14071      -85     
- Partials     7940     7958      +18     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/loader.js	98.63% <100.00%> (-0.16%)	⬇️
lib/internal/modules/esm/module_job.js	96.27% <100.00%> (-0.30%)	⬇️
lib/internal/modules/run_main.js	97.82% <100.00%> (+0.17%)	⬆️

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