Skip to content

Conversation

@joyeecheung
Copy link
Member

To improve determinism of snapshot generation, add --predictable to the V8 flags used to initialize a process launched to generate snapshot. Also add a kGeneratePredictableSnapshot flag to ProcessInitializationFlags for this and moves the configuration of these flags into node::InitializeOncePerProcess() so that it can be shared by embedders.

Refs: nodejs/build#3043

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-botnodejs-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 Jul 12, 2023
@joyeecheungjoyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jul 12, 2023
@joyeecheung
Copy link
MemberAuthor

@targos
Copy link
Member

joyeecheung added a commit to joyeecheung/node that referenced this pull request Jul 20, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: nodejs#48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d
@joyeecheungjoyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Jul 20, 2023
@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Aug 14, 2023

It turns out all the flags implied by --predictable need to be ignored in the flag hash computation too:

--no-concurrent-recompilation --no-parallel-scavenge --no-concurrent-marking --no-concurrent-array-buffer-sweeping --no-parallel-marking --no-concurrent-sweeping --no-parallel-compaction --no-parallel-pointer-update --no-memory-reducer --single-threaded-gc 

Although I now wonder if --predictable is actually needed for our current use case (it seems --predictable currently does not actually introduce additional changes to the snapshot?) This might still be needed down the line though, as V8 does use --predictable for their own snapshot. I may come back and pick this up later when it turns out to be necessary

joyeecheung added a commit to joyeecheung/node that referenced this pull request Aug 14, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: nodejs#48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d
@joyeecheung
Copy link
MemberAuthor

hmm, looks like this is necessary after all or otherwise we get 2 more differences: nodejs/build#3043 (comment)

joyeecheung added a commit to joyeecheung/node that referenced this pull request Sep 7, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: nodejs#48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023
@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit to joyeecheung/node that referenced this pull request Sep 18, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: nodejs#48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot pushed a commit that referenced this pull request Sep 25, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: #48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d PR-URL: #49703 Refs: v8/v8@de9a5de Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>
To improve determinism of snapshot generation, add --predictable to the V8 flags used to initialize a process launched to generate snapshot. Also add a kGeneratePredictableSnapshot flag to ProcessInitializationFlags for this and moves the configuration of these flags into node::InitializeOncePerProcess() so that it can be shared by embedders.
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023
@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Sep 26, 2023

Rebased after #49703 is backported and also moved the flag settings into InitializeNodeWithArgsInternal(), together with other V8::SetFlagsFromString calls. We also do not need to set --harmony-import-assertions now because a) that's true by default now and b) we take care of that already in ProcessGlobalArgsInternal()

@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
MemberAuthor

@anonrig@richardlau This has been rebased and updated. Can I get some new LGTM please?

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: #48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d PR-URL: #49703 Refs: v8/v8@de9a5de Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: #48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d PR-URL: #49703 Refs: v8/v8@de9a5de Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>
@legendecaslegendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2023
@nodejs-github-botnodejs-github-bot merged commit 387e292 into nodejs:mainOct 9, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 387e292

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Original commit message: Ignore --predictable when computing flag list hashes This allows reproducible code cache generation. Refs: nodejs#48749 Change-Id: Ib4693de60ddff1fe41d95c10980f763463db3f95 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4681766 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#88943} Refs: v8/v8@b33bf2d PR-URL: nodejs#49703 Refs: v8/v8@de9a5de Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
To improve determinism of snapshot generation, add --predictable to the V8 flags used to initialize a process launched to generate snapshot. Also add a kGeneratePredictableSnapshot flag to ProcessInitializationFlags for this and moves the configuration of these flags into node::InitializeOncePerProcess() so that it can be shared by embedders. PR-URL: nodejs#48749 Refs: nodejs/build#3043 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
To improve determinism of snapshot generation, add --predictable to the V8 flags used to initialize a process launched to generate snapshot. Also add a kGeneratePredictableSnapshot flag to ProcessInitializationFlags for this and moves the configuration of these flags into node::InitializeOncePerProcess() so that it can be shared by embedders. PR-URL: #48749 Refs: nodejs/build#3043 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
@targostargos mentioned this pull request Nov 12, 2023
codebytere added a commit to electron/electron that referenced this pull request Dec 8, 2023
codebytere added a commit to electron/electron that referenced this pull request Dec 8, 2023
codebytere added a commit to electron/electron that referenced this pull request Dec 11, 2023
* chore: bump node in DEPS to v20.10.0 * chore: update feat_initialize_asar_support.patch no code changes; patch just needed an update due to nearby upstream changes Xref: nodejs/node#49986 * chore: update pass_all_globals_through_require.patch no manual changes; patch applied with fuzz Xref: nodejs/node#49657 * chore: update refactor_allow_embedder_overriding_of_internal_fs_calls Xref: nodejs/node#49912 no code changes; patch just needed an update due to nearby upstream changes * chore: update chore_allow_the_node_entrypoint_to_be_a_builtin_module.patch Xref: nodejs/node#49986 minor manual changes needed to sync with upstream change * update fix_expose_the_built-in_electron_module_via_the_esm_loader.patch Xref: nodejs/node#50096 Xref: nodejs/node#50314 in lib/internal/modules/esm/load.js, update the code that checks for `format === 'electron'`. I'd like 👀 on this Xref: nodejs/node#49657 add braces in lib/internal/modules/esm/translators.js to sync with upstream * fix: lazyload fs in esm loaders to apply asar patches * nodejs/node#50127 * nodejs/node#50096 * esm: jsdoc for modules code nodejs/node#49523 * test: set test-cli-node-options as flaky nodejs/node#50296 * deps: update c-ares to 1.20.1 nodejs/node#50082 * esm: bypass CommonJS loader under --default-type=module nodejs/node#49986 * deps: update uvwasi to 0.0.19 nodejs/node#49908 * lib,test: do not hardcode Buffer.kMaxLength nodejs/node#49876 * crypto: account for disabled SharedArrayBuffer nodejs/node#50034 * test: fix edge snapshot stack traces nodejs/node#49659 * src: generate snapshot with --predictable nodejs/node#48749 * chore: fixup patch indices * fs: throw errors from sync branches instead of separate implementations nodejs/node#49913 * crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey nodejs/node#50234 * esm: detect ESM syntax in ambiguous JavaScrip nodejs/node#50096 * fixup! test: fix edge snapshot stack traces * esm: unflag extensionless ES module JavaScript and Wasm in module scope nodejs/node#49974 * [tagged-ptr] Arrowify objects https://chromium-review.googlesource.com/c/v8/v8/+/4705331 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Charles Kerr <[email protected]> Co-authored-by: Shelley Vohr <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
To improve determinism of snapshot generation, add --predictable to the V8 flags used to initialize a process launched to generate snapshot. Also add a kGeneratePredictableSnapshot flag to ProcessInitializationFlags for this and moves the configuration of these flags into node::InitializeOncePerProcess() so that it can be shared by embedders. PR-URL: nodejs#48749 Refs: nodejs/build#3043 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for freeto 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++.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@joyeecheung@nodejs-github-bot@targos@bnoordhuis@anonrig@richardlau@legendecas