lib: only build the ESM facade for builtins when they are needed by joyeecheung · Pull Request #51669 · nodejs/node · GitHub

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.

Review requested:

@nodejs/loaders
@nodejs/startup

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%

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

It is easier to filter the core startup benchmark with this name.

Updated the CODEOWNERS file for the rename.

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

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

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

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

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

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

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

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

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 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. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

It is easier to filter the core startup benchmark with this name. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Landed in 3e57b93...39e3d21

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 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. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

It is easier to filter the core startup benchmark with this name. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

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.

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 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. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

It is easier to filter the core startup benchmark with this name. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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 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. PR-URL: #51669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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 Feb 5, 2024

joyeecheung force-pushed the lazy-facade branch from 33053df to 159f8ac Compare February 5, 2024 14:56

targos approved these changes Feb 5, 2024

View reviewed changes

aduh95 reviewed Feb 5, 2024

View reviewed changes

Comment thread lib/internal/bootstrap/realm.js Outdated

GeoffreyBooth approved these changes Feb 5, 2024

View reviewed changes

anonrig approved these changes Feb 5, 2024

View reviewed changes

aduh95 approved these changes Feb 5, 2024

View reviewed changes

joyeecheung mentioned this pull request Feb 9, 2024

lib: create global console properties at snapshot build time #51700

Merged

benchmark: rename startup.js to startup-core.js

397aadb

It is easier to filter the core startup benchmark with this name.

joyeecheung force-pushed the lazy-facade branch from 159f8ac to 397aadb Compare February 13, 2024 14:46

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

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 Feb 20, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 20, 2024

joyeecheung closed this Feb 21, 2024

marco-ippolito added the backport-requested-v21.x label Feb 27, 2024

targos mentioned this pull request Feb 27, 2024

deps: update V8 to 12.2 #51362

Closed

marco-ippolito removed the backport-requested-v21.x label Mar 1, 2024

marco-ippolito mentioned this pull request Mar 1, 2024

v21.7.0 proposal #51932

Merged

joyeecheung mentioned this pull request Mar 21, 2024

module: add __esModule to require()'d ESM #52166

Closed

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

jwoo0122 mentioned this pull request Apr 24, 2024

[Bug?]: fs.rm is broken with recursive option in yarn 3 pnp, node >= 20.12.0 yarnpkg/berry#6247

Open

1 task

lib: only build the ESM facade for builtins when they are needed by joyeecheung · Pull Request #51669 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

lib: only build the ESM facade for builtins when they are needed#51669joyeecheung wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom joyeecheung:lazy-facadejoyeecheung/node:lazy-facadeCopy head branch name to clipboard

Conversation

joyeecheung commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

benchmark: rename startup.js to startup-core.js

Uh oh!

nodejs-github-bot commented Feb 5, 2024

Uh oh!

joyeecheung commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Feb 13, 2024

Uh oh!

nodejs-github-bot commented Feb 15, 2024

Uh oh!

nodejs-github-bot commented Feb 16, 2024

Uh oh!

nodejs-github-bot commented Feb 17, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

joyeecheung commented Feb 21, 2024

Uh oh!

marco-ippolito commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lib: only build the ESM facade for builtins when they are needed#51669
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:lazy-facade

joyeecheung commented Feb 5, 2024 •
edited
Loading

joyeecheung commented Feb 5, 2024 •
edited
Loading

marco-ippolito commented Feb 27, 2024 •
edited
Loading

joyeecheung commented Feb 27, 2024 •
edited
Loading