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

src: get embedder options on-demand #40357

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

Only query embedder options when they are needed so that the bootstrap
remains as stateless as possible so that the bootstrap snapshot is
controlled solely by configure options, and subsequent runtime changes
should be done in pre-execution.

Only query embedder options when they are needed so that the bootstrap
remains as stateless as possible so that the bootstrap snapshot is
controlled solely by configure options, and subsequent runtime changes
should be done in pre-execution.
@joyeecheung
Copy link
Member Author

cc @nodejs/embedders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 7, 2021
src/node_options.cc Outdated Show resolved Hide resolved
Co-authored-by: Anna Henningsen <github@addaleax.net>
@Mesteery Mesteery added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Oct 12, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
@nodejs-github-bot
Copy link
Collaborator

@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

There are intermittent timeouts on arm builds with test-crypto-dh and test-crypto-binary-default, though I can't tell how this PR could be related to these two tests. My guess is that the arm bots are not powerful enough and the tests are putting too many computation-intensive cases into single files. I'll see if splitting the test cases out helps.

@Mesteery Mesteery removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2021
joyeecheung added a commit that referenced this pull request Oct 16, 2021
Only query embedder options when they are needed so that the bootstrap
remains as stateless as possible so that the bootstrap snapshot is
controlled solely by configure options, and subsequent runtime changes
should be done in pre-execution.

PR-URL: #40357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
@joyeecheung
Copy link
Member Author

Tests were green. Landed in 38aa7cc

@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2021
targos pushed a commit that referenced this pull request Nov 4, 2021
Only query embedder options when they are needed so that the bootstrap
remains as stateless as possible so that the bootstrap snapshot is
controlled solely by configure options, and subsequent runtime changes
should be done in pre-execution.

PR-URL: #40357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants