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: build code cache from deserialized isolate #49099

Merged
merged 2 commits into from
Aug 18, 2023

Conversation

joyeecheung
Copy link
Member

V8 now requires the code cache to be compiled with a finalized read-only space, so we need to serialize the snapshot to get a finalized read-only space first, then deserialize it to compile the code cache.

Refs: #47636
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13789

V8 now requires the code cache to be compiled with a finalized
read-only space, so we need to serialize the snapshot to get
a finalized read-only space first, then deserialize it to compile
the code cache.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@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 Aug 10, 2023
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 10, 2023

I am making this draft because there could be a better alternative: we can also compile all the builtins (but do not actually load them) from the JS land during the snapshot building process. With SnapshotCreator::FunctionCodeHandling::kKeep we might be able to get a similar result without having to deserialize the snapshot. I'll need to do some benchmarking to see the impact of that approach. IIRC we made the builtin code cache separate from the snapshot because code cache was implemented first and we wanted to have builds with code cache but not snapshot while built-in snapshot was experimental. Now that we already only support built-in code cache as part of the snapshot, we could just streamline the whole thing in JS.

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 10, 2023

hmm, giving it another look, I think we could only remove the C++ land code cache after we extend the worker snapshot to include the builtin loader, otherwise it would be a startup performance regression for workers, so that might involve a bit more work than I thought, meanwhile we could probably do this PR first to make it work with the latest V8.

@joyeecheung joyeecheung marked this pull request as ready for review August 10, 2023 17:44
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

src/node_snapshotable.cc Outdated Show resolved Hide resolved
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2023
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 16, 2023
@nodejs-github-bot 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 Aug 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49099
✔  Done loading data for nodejs/node/pull/49099
----------------------------------- PR info ------------------------------------
Title      bootstrap: build code cache from deserialized isolate (#49099)
Author     Joyee Cheung  (@joyeecheung)
Branch     joyeecheung:snapshot-des -> nodejs:main
Labels     c++, needs-ci, commit-queue-squash
Commits    2
 - bootstrap: build code cache from deserialized isolate
 - fixup! bootstrap: build code cache from deserialized isolate
Committers 2
 - Joyee Cheung 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/49099
Refs: https://github.com/nodejs/node/issues/47636
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13789
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49099
Refs: https://github.com/nodejs/node/issues/47636
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13789
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 10 Aug 2023 17:04:33 GMT
   ✔  Approvals: 1
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49099#pullrequestreview-1581134095
   ✘  This PR needs to wait 20 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-16T17:23:16Z: https://ci.nodejs.org/job/node-test-pull-request/53386/
   ℹ  Last Benchmark CI on 2023-08-10T17:47:50Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1356/
- Querying data for job/node-test-pull-request/53386/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/5883429281

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 7b7a68b into nodejs:main Aug 18, 2023
36 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7b7a68b

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
V8 now requires the code cache to be compiled with a finalized
read-only space, so we need to serialize the snapshot to get
a finalized read-only space first, then deserialize it to compile
the code cache.

PR-URL: #49099
Refs: #47636
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13789
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants