Skip to content

[WIP] reduce circular dependencies in ESM loader and move more essential modules into the snapshot

This is not yet ready for review, unless you enjoy reading a huge diff that's mostly just moving code around. I am opening this to get some numbers from the CI and see if it needs fixups in niche build configs. My plan is to try to split it into more logical commits to make it easier to review/backport - this is meant to be a refactoring that does not change any behavior, if for some reason something subtle gets broken (e.g. the race caused by a V8 bug that needs to be worked around in test/sequential/test-inspector-break-when-eval.js, which probably can't be reproduced in real-world debug sessions). At least it's easier to trace that down with smaller commits. But if for some reason the patch just can't be broken into smaller commits, I'd send this one for review instead.

Locally the test passes and I get a pretty good boost in the startup benchmarks:

                                                                                     confidence improvement accuracy (*)   (**)  (***)
misc/startup.js count=30 mode='process' script='benchmark/fixtures/require-builtins'        ***      4.71 %       ±0.76% ±1.01% ±1.32%
misc/startup.js count=30 mode='process' script='test/fixtures/semicolon'                    ***     11.94 %       ±1.39% ±1.85% ±2.40%
misc/startup.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'         ***      4.25 %       ±0.72% ±0.96% ±1.25%
misc/startup.js count=30 mode='worker' script='test/fixtures/semicolon'                     ***     11.55 %       ±1.21% ±1.61% ±2.10%

A brief summary of what's been done here:

  • Move modules that we have to load anyway during startup into the snapshot, instead of loading them at pre-execution (runtime). This includes changes in is_main_thread.js and browser.js (worker thread currently only has a very basic built-iin snapshot without modules, we can merge the list when the worker thread also has modules in its snapshot)
  • Reduce the circular dependencies in lib/internal/modules/esm/ as much as possible by merging most ESM-only utilities (esm/assert.js, esm/create_dynamic_module.js, esm/formats.js, esm/get_format.js, esm/handle_process_exit.js, esm/initialize_import_meta.js, esm/module_job.js, esm/module_map.js, esm/package_config.js) into one lib/internal/modules/esm/utils.js and make most lib/internal/modules/esm/ inter-dependencies lazy. I don't think splitting the ESM loader into small modules really yields much benefit when that results in landmines of circular dependencies and TDZs that we need to watch out for. Fewer modules also means lower startup costs.
    • esm/translators.js is merged into esm/loader.js since the translators aren't used anywhere else
    • modules/cjs/helpers is now modules/helpers because it's shared by two loaders, and some utils used by the both loaders are moved there as well.
    • It's inevitable that there is still some circular dependency involving the cascaded ESM loader. The cascaded ESM loader needs the ESMLoader constructor to construct itself (duh), while the default implementation of ESMLoader depends on the cascaded ESM loader too (why it doesn't use this, I don't know, but it's not the time to break that). Meanwhile almost all module parts depend on the cascaded ESM loader (actually in a pretty dangerous way, because many of them have been getting the loader synchronously even though the loader hooks are loaded asynchronously so it's non-deterministic whether they will get a loader with hooks or not, but that's a pretty old problem that this refactoring PR does not plan to solve), so the cascaded loader still has it's own file that lazily loads the constructor in order to reduce the severity of the circular dependency.
  • Remove most top-level getOptionValue invocations and top-level access to process.env in lib/internal/modules/esm/ by making those accesses lazy. This also makes lib/internal/modules/esm/utils.js snapshottable.
  • Register the external references of module_wrap to make it snapshotable too.

After applying this patch, loading a user-land CJS module alone does not incur compilation of any additional modules. If the builtins loaded by that module is already in the snapshot (e.g. fs, url, timers, util, path..modules that are in test-bootstrap-modules.js), there's no additional cost of compilation either. That explains the benchmark numbers because compilation is still the bottleneck of startup. Some ESM loader parts still need to be compiled for a user-land ESM module, depending on what that module actually does, though a big chunk of ESM loader is now in the utils.js which is also snapshotted (before this patch none of the ESM loader is in the snapshot because without the refactoring it was not snapshottable).

Merge request reports

Loading