Skip to content

Conversation

@joyeecheung
Copy link
Member

When the source text module is compiled without custom callbacks, instead of calling into JS land from the per-isolate import.meta initializer and then back to C++ land to set up lazy data properties, just do the initialization all in C++ land. Only import.meta.resolve initialization will call back into JS land to generate a closure that call the cascaded loader for resolution.

In addition, simplify the loader structure by merging allowImportMetaResolve into isForAsyncLoaderHookWorker - the two are essentially equivalent, as import.meta.resolve is only allowed in a non-loader-hook worker thread's loader.

When the source text module is compiled without custom callbacks,
instead of calling into JS land from the per-isolate import.meta
initializer and then back to C++ land to set up lazy data
properties, just do the initialization all in C++ land.
Only import.meta.resolve initialization will call back
into JS land to generate a closure that call the cascaded loader
for resolution.

In addition, simplify the loader structure by merging
allowImportMetaResolve into isForAsyncLoaderHookWorker - the two
are essentially equivalent, as import.meta.resolve is only
allowed in a non-loader-hook worker thread's loader.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/vm

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 6, 2025
@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 75.58140% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (e72761f) to head (5d486b7).
⚠️ Report is 77 commits behind head on main.

Files with missing lines Patch % Lines
src/module_wrap.cc 62.16% 21 Missing and 21 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60603      +/-   ##
==========================================
- Coverage   88.57%   88.53%   -0.05%     
==========================================
  Files         704      703       -1     
  Lines      208087   208075      -12     
  Branches    40090    40088       -2     
==========================================
- Hits       184306   184212      -94     
- Misses      15849    15886      +37     
- Partials     7932     7977      +45     
Files with missing lines Coverage Δ
lib/internal/modules/esm/hooks.js 86.05% <ø> (-0.12%) ⬇️
lib/internal/modules/esm/loader.js 96.96% <100.00%> (+0.10%) ⬆️
lib/internal/modules/esm/utils.js 100.00% <100.00%> (ø)
lib/internal/modules/package_json_reader.js 97.61% <100.00%> (ø)
src/module_wrap.h 52.94% <ø> (ø)
src/node_modules.cc 77.83% <ø> (+1.61%) ⬆️
src/module_wrap.cc 74.20% <62.16%> (-1.49%) ⬇️

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

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the lint failure

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect that this should result in performance improvements, especially for a project with a lot of modules; should we run a benchmark?

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2025
@nodejs-github-bot
Copy link
Collaborator

Instead of measuring the performance of the entire module
initialization, focus only on the import.meta initialization.
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 7, 2025

I would expect that this should result in performance improvements, especially for a project with a lot of modules

Yes...and no. If I modify the benchmarks like what I just pushed so that it entirely focuses on import.meta initialization, not the entire module loading, then there's an improvement:

                                                              confidence improvement accuracy (*)   (**)  (***)
esm/import-meta.js valuesToRead='dirname-and-filename' n=1000        ***      5.41 %       ±2.06% ±2.74% ±3.57%
esm/import-meta.js valuesToRead='dirname' n=1000                     ***      7.17 %       ±3.18% ±4.26% ±5.60%
esm/import-meta.js valuesToRead='filename' n=1000                    ***      9.60 %       ±2.31% ±3.08% ±4.01%
esm/import-meta.js valuesToRead='url' n=1000                         ***      9.78 %       ±1.76% ±2.35% ±3.07%

For the original version of the benchmark, which measures the entire performance of running import(...), there's no significant performance difference, since import.meta initialization isn't all that much work compared the entire process of resolving/loading/compiling/evaluating a module.

Nonetheless it seems useful to have a more focused benchmark, since we already have other benchmarks for measuring the entire loading process.

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Pushed the new benchmark. Can I get another review please? @GeoffreyBooth @aduh95

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 7, 2025
@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Nov 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in b15070b...53ee038

nodejs-github-bot pushed a commit that referenced this pull request Nov 8, 2025
When the source text module is compiled without custom callbacks,
instead of calling into JS land from the per-isolate import.meta
initializer and then back to C++ land to set up lazy data
properties, just do the initialization all in C++ land.
Only import.meta.resolve initialization will call back
into JS land to generate a closure that call the cascaded loader
for resolution.

In addition, simplify the loader structure by merging
allowImportMetaResolve into isForAsyncLoaderHookWorker - the two
are essentially equivalent, as import.meta.resolve is only
allowed in a non-loader-hook worker thread's loader.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 8, 2025
Instead of measuring the performance of the entire module
initialization, focus only on the import.meta initialization.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
aduh95 pushed a commit that referenced this pull request Nov 10, 2025
Instead of measuring the performance of the entire module
initialization, focus only on the import.meta initialization.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
aduh95 pushed a commit that referenced this pull request Nov 10, 2025
When the source text module is compiled without custom callbacks,
instead of calling into JS land from the per-isolate import.meta
initializer and then back to C++ land to set up lazy data
properties, just do the initialization all in C++ land.
Only import.meta.resolve initialization will call back
into JS land to generate a closure that call the cascaded loader
for resolution.

In addition, simplify the loader structure by merging
allowImportMetaResolve into isForAsyncLoaderHookWorker - the two
are essentially equivalent, as import.meta.resolve is only
allowed in a non-loader-hook worker thread's loader.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
aduh95 pushed a commit that referenced this pull request Nov 11, 2025
Instead of measuring the performance of the entire module
initialization, focus only on the import.meta initialization.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
aduh95 pushed a commit that referenced this pull request Nov 11, 2025
When the source text module is compiled without custom callbacks,
instead of calling into JS land from the per-isolate import.meta
initializer and then back to C++ land to set up lazy data
properties, just do the initialization all in C++ land.
Only import.meta.resolve initialization will call back
into JS land to generate a closure that call the cascaded loader
for resolution.

In addition, simplify the loader structure by merging
allowImportMetaResolve into isForAsyncLoaderHookWorker - the two
are essentially equivalent, as import.meta.resolve is only
allowed in a non-loader-hook worker thread's loader.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@aduh95 aduh95 added the backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. label Nov 13, 2025
aduh95 pushed a commit that referenced this pull request Nov 13, 2025
Instead of measuring the performance of the entire module
initialization, focus only on the import.meta initialization.

PR-URL: #60603
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Nov 13, 2025

Build is failing with 9c7d5ae on v24.x-staging

../../src/module_wrap.cc:1336:12: error: no member named 'IsEmpty' in 'v8::Maybe<bool>'
 1333 |   if (meta->Set(context,
      |       ~~~~~~~~~~~~~~~~~~
 1334 |                 FIXED_ONE_BYTE_STRING(isolate, "main"),
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1335 |                 Boolean::New(isolate, is_main->IsTrue()))
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1336 |           .IsEmpty()) {
      |            ^
../../src/module_wrap.cc:1354:50: error: no member named 'IsEmpty' in 'v8::Maybe<bool>'
 1354 |   if (meta->Set(context, env->url_string(), url).IsEmpty()) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants