node
node copied to clipboard
nodejs
lib: only build the ESM facade for builtins when they are needed
lib: only build the ESM facade for builtins when they are needed
We previously build the ESM facade (synthetic modules re-exporting builtin's exports) for builtins even when they are not directly import'ed (which rarely happens for internal builtins as that requires --expose-internals). This patch removes the eager generation to avoid the overhead and the extra promises created in facade building when it's not reqested by the user. When the facade is needed by the ESM loader that can be requested it in the translator on-demand.
Drive-by: set the ModuleWrap prototype to null in the built-in snapshot.
benchmark: rename startup.js to startup-core.js
It is easier to filter the core startup benchmark with this name.
Local benchmark result from Apple M2 Max
confidence improvement accuracy (*) (**) (***)
misc/startup-cli-version.js count=30 cli='deps/corepack/dist/corepack.js' ** 2.25 % ±1.63% ±2.17% ±2.82%
misc/startup-cli-version.js count=30 cli='deps/npm/bin/npm-cli.js' ** 2.78 % ±2.01% ±2.67% ±3.48%
misc/startup-cli-version.js count=30 cli='deps/npm/bin/npx-cli.js' * 2.28 % ±2.17% ±2.89% ±3.76%
misc/startup-cli-version.js count=30 cli='tools/node_modules/eslint/bin/eslint.js' 1.23 % ±1.90% ±2.53% ±3.30%
misc/startup-core.js count=30 mode='process' script='benchmark/fixtures/require-builtins' *** 5.03 % ±1.40% ±1.86% ±2.42%
misc/startup-core.js count=30 mode='process' script='test/fixtures/semicolon' 0.05 % ±1.31% ±1.74% ±2.27%
misc/startup-core.js count=30 mode='worker' script='benchmark/fixtures/require-builtins' *** 9.11 % ±1.91% ±2.54% ±3.31%
misc/startup-core.js count=30 mode='worker' script='test/fixtures/semicolon' -0.12 % ±1.92% ±2.55% ±3.32%
Commit Queue failed
- Loading data for nodejs/node/pull/51669 ✔ Done loading data for nodejs/node/pull/51669 ----------------------------------- PR info ------------------------------------ Title lib: only build the ESM facade for builtins when they are needed (#51669) Author Joyee Cheunghttps://github.com/nodejs/node/actions/runs/7981158394(@joyeecheung) Branch joyeecheung:lazy-facade -> nodejs:main Labels lib / src, needs-ci, commit-queue-rebase Commits 2 - lib: only build the ESM facade for builtins when they are needed - benchmark: rename startup.js to startup-core.js Committers 1 - Joyee Cheung PR-URL: https://github.com/nodejs/node/pull/51669 Reviewed-By: Michaël Zasso Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/51669 Reviewed-By: Michaël Zasso Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - benchmark: rename startup.js to startup-core.js ℹ This PR was created on Mon, 05 Feb 2024 14:37:32 GMT ✔ Approvals: 4 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/51669#pullrequestreview-1863024303 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/51669#pullrequestreview-1863256052 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/51669#pullrequestreview-1863362060 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/51669#pullrequestreview-1863385762 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-02-20T21:33:30Z: https://ci.nodejs.org/job/node-test-pull-request/57218/ - Querying data for job/node-test-pull-request/57218/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
It fails on Node 21 https://github.com/nodejs/node/actions/runs/8049972458/job/21984543795?pr=51768 I've bisect and found out its this commit Fix: https://github.com/nodejs/node/pull/51898
The test itself looks unrelated to this PR but it is gc-dependent, so this PR on v21 might somehow nudge the graph into a certain shape that triggers some incorrect assumptions the test makes about finalizers. Maybe @gabrielschulhof has more insights.
From local testing, it seems the test is making the assumption that, if you create a finalizer during addon initialization that's to be run when the object's first pass weak callback is invoked, and in the first finalizer you schedule an immediate callback to invoke the second pass finalizer, the second pass finalizer must be invoked when env->can_call_into_js() is true, which is not a reliable assumption. The event loop can continue to run and clear the natives before environment shutdown, so the second pass finalizer can be called early. The test should be updated to account for that IMO.