Skip to content

Conversation

joyeecheung
Copy link
Member

This patch:

  • Make sure that the vm.Module.evaluate() method is conditionally synchronous based on the specification. Previously, it's unconditionally asynchronous (even for synthetic modules and source text modules without top-level await).
  • Clarify the synchronicity of this method in the documentation
  • Add more tests for the synchronicity of this method.

Refs: #59656
Refs: #37648

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Oct 10, 2025
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.55%. Comparing base (db0121b) to head (d308c89).
⚠️ Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60205      +/-   ##
==========================================
+ Coverage   88.53%   88.55%   +0.01%     
==========================================
  Files         704      704              
  Lines      208087   208151      +64     
  Branches    40010    40013       +3     
==========================================
+ Hits       184239   184322      +83     
+ Misses      15866    15863       -3     
+ Partials     7982     7966      -16     
Files with missing lines Coverage Δ
lib/internal/vm/module.js 96.37% <100.00%> (+0.03%) ⬆️

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

@joyeecheung
Copy link
Member Author

Added a benchmark for source text modules - there are some performance improvements by removing the async-await:

                                                                      confidence improvement accuracy (*)    (**)   (***)
vm/source-text-module-leaf.js n=1000 type='async' stage='all'                ***      4.24 %       ±2.05%  ±2.72%  ±3.55%
vm/source-text-module-leaf.js n=1000 type='async' stage='compile'                    -0.58 %       ±1.54%  ±2.05%  ±2.68%
vm/source-text-module-leaf.js n=1000 type='async' stage='evaluate'           ***     31.67 %       ±8.54% ±11.40% ±14.91%
vm/source-text-module-leaf.js n=1000 type='async' stage='instantiate'                 6.96 %       ±8.02% ±10.68% ±13.90%
vm/source-text-module-leaf.js n=1000 type='async' stage='link'                        1.29 %       ±2.93%  ±3.92%  ±5.15%
vm/source-text-module-leaf.js n=1000 type='sync' stage='all'                   *      1.28 %       ±1.06%  ±1.42%  ±1.85%
vm/source-text-module-leaf.js n=1000 type='sync' stage='compile'                     -0.97 %       ±1.32%  ±1.76%  ±2.29%
vm/source-text-module-leaf.js n=1000 type='sync' stage='evaluate'            ***     15.50 %       ±3.84%  ±5.11%  ±6.65%
vm/source-text-module-leaf.js n=1000 type='sync' stage='instantiate'                  3.47 %       ±4.66%  ±6.21%  ±8.08%
vm/source-text-module-leaf.js n=1000 type='sync' stage='link'                        -1.68 %       ±2.83%  ±3.80%  ±5.01%

- Make sure that the vm.Module.evaluate() method is conditionally
  synchronous based on the specification. Previously, the returned
  promise is unconditionally pending (even for synthetic modules and
  source text modules without top-level await) instead of immediately
  fulfilled, making it harder to debug as it deviates from the
  specified behavior.
- Clarify the synchronicity of this method in the documentation
- Add more tests for the synchronicity of this method.
@joyeecheung
Copy link
Member Author

Updated the evaluate implementation so that it keeps rejecting for loader errors yet still returns an immediately fulfilled promise for the actual evaluation. The perf numbers are still similar:

                                                                      confidence improvement accuracy (*)    (**)   (***)
vm/source-text-module-leaf.js n=1000 type='async' stage='all'                ***      3.81 %       ±1.19%  ±1.59%  ±2.07%
vm/source-text-module-leaf.js n=1000 type='async' stage='compile'                    -0.85 %       ±1.11%  ±1.48%  ±1.93%
vm/source-text-module-leaf.js n=1000 type='async' stage='evaluate'           ***     34.00 %       ±8.05% ±10.74% ±14.02%
vm/source-text-module-leaf.js n=1000 type='async' stage='instantiate'                -1.24 %       ±7.81% ±10.40% ±13.54%
vm/source-text-module-leaf.js n=1000 type='async' stage='link'                       -0.58 %       ±2.57%  ±3.44%  ±4.51%
vm/source-text-module-leaf.js n=1000 type='sync' stage='all'                          0.67 %       ±1.27%  ±1.70%  ±2.21%
vm/source-text-module-leaf.js n=1000 type='sync' stage='compile'                     -0.24 %       ±1.12%  ±1.49%  ±1.94%
vm/source-text-module-leaf.js n=1000 type='sync' stage='evaluate'            ***     19.77 %       ±3.54%  ±4.72%  ±6.14%
vm/source-text-module-leaf.js n=1000 type='sync' stage='instantiate'                  1.93 %       ±4.66%  ±6.20%  ±8.07%
vm/source-text-module-leaf.js n=1000 type='sync' stage='link'                        -0.09 %       ±3.83%  ±5.09%  ±6.63%

@joyeecheung
Copy link
Member Author

Opened a separate issue about the rejection #60242

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

Labels

needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants