lib: only build the ESM facade for builtins when they are needed

[joyeecheung]

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.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/startup

[joyeecheung]

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% 

[joyeecheung]

Updated the CODEOWNERS file for the rename.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57101/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57148/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57157/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57184/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57207/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57212/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57218/

[nodejs-github-bot]

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 Cheung (@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

https://github.com/nodejs/node/actions/runs/7981158394

[joyeecheung]

Landed in 3e57b93...39e3d21

[marco-ippolito]

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: #51898

[joyeecheung]

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.

[joyeecheung]

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.