src: remove cached data tag from snapshot metadata #54122
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
c++
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54122 +/- ##
==========================================
+ Coverage 87.08% 87.32% +0.24%
==========================================
Files 648 648
Lines 182341 182306 -35
Branches 34982 34976 -6
==========================================
+ Hits 158783 159199 +416
+ Misses 16831 16377 -454
- Partials 6727 6730 +3
|
legendecas
reviewed
Jul 30, 2024
There was a problem hiding this comment.
The V8 flags aren't necessarily important for snapshot/code cache mismatches (only a small subset are), and the CPU features currently don't matter
If there are still a subset of V8 flags that matter, can the CpuFeatures flag be removed from v8::ScriptCompiler::CachedDataVersionTag() at https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=2854;drc=105770df485ace262780d95126bb60b1a16ec340;bpv=1;bpt=1 instead?
We had that discussion in https://chromium-review.googlesource.com/c/v8/v8/+/4905290 and the short answer is no,because the V8 team wants to reserve the ability to make the code cache CPU dependent. This API is used by Chromium as a key for cache storage to detect staleness, so it's not really something we can just break for Node.js either. For the hardware-specific data in code cache, I think the better way forward would be something like what I mentioned in nodejs/build#3043 (comment) - we should ideally update V8 to persist "flags/CPU features required by this cache", instead of "flags/CPU features used to compile this cache", and let the cache get rejected if it doesn't work. But for the data accompanying snapshots, this is what we can do for now. |
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
anonrig
approved these changes
Jul 30, 2024
legendecas
reviewed
Aug 5, 2024
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
legendecas
approved these changes
Aug 5, 2024
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
removed
the
request-ci
github-actions
bot
removed
the
request-ci
joyeecheung
added
request-ci
github-actions
bot
removed
the
request-ci
|
Fixed linter complaints.. |
github-actions
bot
removed
the
request-ci
github-actions
bot
removed
the
request-ci
joyeecheung
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/54122 ✔ Done loading data for nodejs/node/pull/54122 ----------------------------------- PR info ------------------------------------ Title src: remove cached data tag from snapshot metadata (#54122) Author Joyee Cheung <joyeec9h3@gmail.com> (@joyeecheung) Branch joyeecheung:remove-flag -> nodejs:main Labels c++, needs-ci, commit-queue-squash Commits 2 - src: remove cached data tag from snapshot metadata - fixup! src: remove cached data tag from snapshot metadata Committers 1 - Joyee Cheung <joyeec9h3@gmail.com> PR-URL: https://github.com/nodejs/node/pull/54122 Refs: https://github.com/nodejs/build/issues/3043 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54122 Refs: https://github.com/nodejs/build/issues/3043 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - src: remove cached data tag from snapshot metadata ⚠ - fixup! src: remove cached data tag from snapshot metadata ℹ This PR was created on Tue, 30 Jul 2024 11:11:10 GMT ✔ Approvals: 2 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54122#pullrequestreview-2208197134 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/54122#pullrequestreview-2219446395 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-08-22T16:08:27Z: https://ci.nodejs.org/job/node-test-pull-request/61351/ - Querying data for job/node-test-pull-request/61351/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/10514538417 |
legendecas
approved these changes
Aug 23, 2024
legendecas
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 9ee3a72 |
RafaelGSS
pushed a commit
that referenced
this pull request
Aug 25, 2024
This only served as a preemptive check, but serializing this in the snapshot would make it unreproducible on different hardware. In the current cached data version tag, the V8 version can already be checked as part of the existing Node.js version check. The V8 flags aren't necessarily important for snapshot/code cache mismatches (only a small subset are), and the CPU features currently don't matter, so doing an exact match is stricter than necessary. Removing the check to help making the snapshot more reproducible on different hardware. PR-URL: #54122 Refs: nodejs/build#3043 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Merged
RafaelGSS
pushed a commit
that referenced
this pull request
Aug 30, 2024
This only served as a preemptive check, but serializing this in the snapshot would make it unreproducible on different hardware. In the current cached data version tag, the V8 version can already be checked as part of the existing Node.js version check. The V8 flags aren't necessarily important for snapshot/code cache mismatches (only a small subset are), and the CPU features currently don't matter, so doing an exact match is stricter than necessary. Removing the check to help making the snapshot more reproducible on different hardware. PR-URL: #54122 Refs: nodejs/build#3043 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos
added
the
dont-land-on-v20.x
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This only served as a preemptive check, but serializing this in the snapshot would make it unreproducible on different hardware. In the current cached data version tag, the V8 version can already be checked as part of the existing Node.js version check. The V8 flags aren't necessarily important for snapshot/code cache mismatches (only a small subset are), and the CPU features currently don't matter, so doing an exact match is stricter than necessary. Removing the check to help making the snapshot more reproducible on different hardware.
Refs: nodejs/build#3043