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

joyeecheung · 2024-02-05T14:37:32Z

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 · 2024-02-05T14:37:37Z

Review requested:

@nodejs/loaders
@nodejs/startup

joyeecheung · 2024-02-05T14:54:41Z

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.

lib/internal/bootstrap/realm.js

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

joyeecheung · 2024-02-13T14:46:52Z

Updated the CODEOWNERS file for the rename.

nodejs-github-bot · 2024-02-15T16:37:44Z

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

nodejs-github-bot · 2024-02-16T18:15:13Z

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

nodejs-github-bot · 2024-02-17T16:58:19Z

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

nodejs-github-bot · 2024-02-20T01:28:14Z

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

nodejs-github-bot · 2024-02-20T18:10:27Z

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

nodejs-github-bot · 2024-02-20T18:55:54Z

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

nodejs-github-bot · 2024-02-20T21:33:30Z

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

nodejs-github-bot · 2024-02-20T23:00:56Z

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>

joyeecheung · 2024-02-21T01:40:58Z

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>

marco-ippolito · 2024-02-27T11:12:46Z

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 · 2024-02-27T12:37:26Z

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 · 2024-02-27T15:15:15Z

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>

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

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

lib/internal/bootstrap/realm.js Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #51669

joyeecheung commented Feb 5, 2024 •

edited

Loading

nodejs-github-bot commented Feb 5, 2024

joyeecheung commented Feb 5, 2024 •

edited

Loading

joyeecheung commented Feb 13, 2024

nodejs-github-bot commented Feb 15, 2024

nodejs-github-bot commented Feb 16, 2024

nodejs-github-bot commented Feb 17, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

joyeecheung commented Feb 21, 2024

marco-ippolito commented Feb 27, 2024 •

edited

Loading

joyeecheung commented Feb 27, 2024 •

edited

Loading

joyeecheung commented Feb 27, 2024

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 #51669

Conversation

joyeecheung commented Feb 5, 2024 • edited Loading

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

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

nodejs-github-bot commented Feb 5, 2024

joyeecheung commented Feb 5, 2024 • edited Loading

joyeecheung commented Feb 13, 2024

nodejs-github-bot commented Feb 15, 2024

nodejs-github-bot commented Feb 16, 2024

nodejs-github-bot commented Feb 17, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

nodejs-github-bot commented Feb 20, 2024

joyeecheung commented Feb 21, 2024

marco-ippolito commented Feb 27, 2024 • edited Loading

joyeecheung commented Feb 27, 2024 • edited Loading

joyeecheung commented Feb 27, 2024

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