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

bootstrap: support more builtins in the embedded code cache #44018

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

This patch:

  • Make NativeModuleLoader::LookupAndCompile() detect parameters based
    on module IDs. This allows us to compile more builtins when
    generating the embedded bootstrap, including
    • internal/per_context/*
    • internal/bootstrap/*
    • internal/main/*
  • Move pre_execution.js to lib/internal/process as it needs to be
    compiled as a regular built-in module, unlike other scripts
    in lib/internal/bootstrap
  • Move markBootstrapComplete() to the performance binding instead of
    making it a function-wrapper-based global to reduce number of
    special cases.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @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 Jul 28, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

This pattern is more static and cleaner.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

This patch:

- Make NativeModuleLoader::LookupAndCompile() detect parameters based
  on module IDs. This allows us to compile more builtins when
  generating the embedded bootstrap, including
  - internal/per_context/*
  - internal/bootstrap/*
  - internal/main/*
- Move pre_execution.js to lib/internal/process as it needs to be
  compiled as a regular built-in module, unlike other scripts
  in lib/internal/bootstrap
- Move markBootstrapComplete() to the performance binding instead of
  making it a function-wrapper-based global to reduce number of
  special cases.
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Rebased to resolve the merge conflict

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Aug 4, 2022
This patch:

- Make NativeModuleLoader::LookupAndCompile() detect parameters based
  on module IDs. This allows us to compile more builtins when
  generating the embedded bootstrap, including
  - internal/per_context/*
  - internal/bootstrap/*
  - internal/main/*
- Move pre_execution.js to lib/internal/process as it needs to be
  compiled as a regular built-in module, unlike other scripts
  in lib/internal/bootstrap
- Move markBootstrapComplete() to the performance binding instead of
  making it a function-wrapper-based global to reduce number of
  special cases.

PR-URL: #44018
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@joyeecheung
Copy link
Member Author

Landed in 06f5d45

@joyeecheung joyeecheung closed this Aug 4, 2022
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
This patch:

- Make NativeModuleLoader::LookupAndCompile() detect parameters based
  on module IDs. This allows us to compile more builtins when
  generating the embedded bootstrap, including
  - internal/per_context/*
  - internal/bootstrap/*
  - internal/main/*
- Move pre_execution.js to lib/internal/process as it needs to be
  compiled as a regular built-in module, unlike other scripts
  in lib/internal/bootstrap
- Move markBootstrapComplete() to the performance binding instead of
  making it a function-wrapper-based global to reduce number of
  special cases.

PR-URL: #44018
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
This patch:

- Make NativeModuleLoader::LookupAndCompile() detect parameters based
  on module IDs. This allows us to compile more builtins when
  generating the embedded bootstrap, including
  - internal/per_context/*
  - internal/bootstrap/*
  - internal/main/*
- Move pre_execution.js to lib/internal/process as it needs to be
  compiled as a regular built-in module, unlike other scripts
  in lib/internal/bootstrap
- Move markBootstrapComplete() to the performance binding instead of
  making it a function-wrapper-based global to reduce number of
  special cases.

PR-URL: #44018
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mmomtchev
Copy link
Contributor

@joyeecheung I am late to the party, but aren't the new ExecuteBootstrapper parameters more error-prone since they rely on an order that is determined by a different file - I mean that list in src/node_builtins.cc? Have you considered simply having one and only full list of all parameters that are to be received by all bootstrapping scripts? The performance hit would be minimal or is there another reason?

@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 1, 2022

@mmomtchev The parameters are guarded by the linter and when passed wrong it’s likely to trigger an error early in the bootstrap process. It’s inevitable that some of the arguments are just unavailable to some scripts (because they are not initialized yet or aren’t supposed to be accessible), it would be nicer to see a ReferenceError instead of a TypeError (something is undefined) in that case. Personally I would find it more error-prone if per-context scripts get an undefined internalBinding or process…

@mmomtchev
Copy link
Contributor

@joyeecheung You can always skip those values that are undefined - you check if it has been initialised and then add it only it has a value. What I meant is that the part that adds the names and the part that adds the values are in two different files and they must have the exactly same order. Isn't it possible to generate the values in src/node_builtins.cc? Or maybe to pass pairs of name/values?

@legendecas
Copy link
Member

What I meant is that the part that adds the names and the part that adds the values are in two different files and they must have the exactly same order.

Isn't this already the fact of how we define the function parameters and how we invoke functions with arguments?

@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 2, 2022

Isn't it possible to generate the values in src/node_builtins.cc? Or maybe to pass pairs of name/values?

I gave it a try in #44488 - some of the values can be generated, but not all of them (e.g. the per-context arguments, the ones generated by the JS-land loader). And as @legendecas pointed out that's just how JS functions work, it's sometimes possible to place the definition and the callsite closer together, other times you just have to look up the parameters somewhere else and make sure that the arguments passed into it are in the right order.

Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
This patch:

- Make NativeModuleLoader::LookupAndCompile() detect parameters based
  on module IDs. This allows us to compile more builtins when
  generating the embedded bootstrap, including
  - internal/per_context/*
  - internal/bootstrap/*
  - internal/main/*
- Move pre_execution.js to lib/internal/process as it needs to be
  compiled as a regular built-in module, unlike other scripts
  in lib/internal/bootstrap
- Move markBootstrapComplete() to the performance binding instead of
  making it a function-wrapper-based global to reduce number of
  special cases.

PR-URL: nodejs#44018
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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