Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: do less CJS module loader initialization at run time #47194

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

joyeecheung
Copy link
Member

This patch:

  • Builds the set of modules that can be required by users with/without the node: prefix at snapshot building time. We only modify it when --expose-internals or the flag for experimental wasi module is true but the default set is now in the snapshot. At run time the CJS module loader only creates a frozen array out of it.
  • BuiltinModule.canBeRequiredWithoutScheme() is now enough to determine if an id can be required without node: without an additional call to BuiltinModule.canBeRequiredByUsers()
  • Replace the pending-to-deprecate methods on Module with an internal implementation that only queries the CLI flags when being invoked. So we can install these methods in the snapshot.

Local benchmark numbers:

                                                                                     confidence improvement accuracy (*)   (**)  (***)
misc/startup.js count=30 mode='process' script='benchmark/fixtures/require-builtins'                 0.32 %       ±0.40% ±0.53% ±0.69%
misc/startup.js count=30 mode='process' script='test/fixtures/semicolon'                    ***      1.27 %       ±0.57% ±0.76% ±0.99%
misc/startup.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'                 -0.40 %       ±0.44% ±0.59% ±0.77%
misc/startup.js count=30 mode='worker' script='test/fixtures/semicolon'                              0.23 %       ±0.54% ±0.72% ±0.94%

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-bot 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 Mar 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

CI was happy. @nodejs/startup @nodejs/loaders can I have some reviews please?

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this doesn’t require any new tests?

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved
lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

I guess this doesn’t require any new tests?

Probably not, the public parts are already well-covered in the tests.

@joyeecheung
Copy link
Member Author

Rebased to remove the wasi experimental flag now that #47286 landed, so the experimentalModuleList is now empty (I kept it here though, so that people can add experimental modules to it later, the isn't a runtime cost after this anyway)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.
@joyeecheung
Copy link
Member Author

Rebased to move the changes to loaders.js to realm.js

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 5, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 5, 2023
@nodejs-github-bot nodejs-github-bot merged commit 5572f8f into nodejs:main Apr 5, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5572f8f

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: #47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: #47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: #47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@RafaelGSS
Copy link
Member

Hi @joyeecheung this commit also didn't land cleanly, could you please backport it?

RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: #47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jun 12, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: #47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: nodejs/node#47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: nodejs/node#47194
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants