src: remove cached data tag from snapshot metadata

[joyeecheung]

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

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.32%. Comparing base (2c14615) to head (d93f0a0).
Report is 381 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/env.cc	85.29% <ø> (-0.20%)	⬇️
src/env.h	97.87% <ø> (ø)
src/node_snapshotable.cc	78.21% <ø> (-0.55%)	⬇️

... and 50 files with indirect coverage changes

[legendecas]

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?

[joyeecheung]

If there are still a subset of V8 flags that matter, can the CpuFeatures flag be removed

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.

[legendecas]

Sounds good to me. A minor fix to address complaining CI.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60979/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61058/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61118/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61164/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61205/

[joyeecheung]

Fixed linter complaints..

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61229/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61248/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61314/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61342/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61351/

[nodejs-github-bot]

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 <[email protected]> (@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 <[email protected]> PR-URL: https://github.com/nodejs/node/pull/54122 Refs: https://github.com/nodejs/build/issues/3043 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54122 Refs: https://github.com/nodejs/build/issues/3043 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> -------------------------------------------------------------------------------- ⚠ 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/.ncu

https://github.com/nodejs/node/actions/runs/10514538417

[nodejs-github-bot]

Landed in 9ee3a72