Skip to content

Commit

Permalink
src: remove cached data tag from snapshot metadata
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
joyeecheung committed Aug 23, 2024
1 parent 628469c commit 9ee3a72
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 26 deletions.
1 change: 0 additions & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@ std::ostream& operator<<(std::ostream& output, const SnapshotMetadata& i) {
<< " \"" << i.node_version << "\", // node_version\n"
<< " \"" << i.node_arch << "\", // node_arch\n"
<< " \"" << i.node_platform << "\", // node_platform\n"
<< " " << i.v8_cache_version_tag << ", // v8_cache_version_tag\n"
<< " " << i.flags << ", // flags\n"
<< "}";
return output;
Expand Down
2 changes: 0 additions & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -545,8 +545,6 @@ struct SnapshotMetadata {
std::string node_version;
std::string node_arch;
std::string node_platform;
// Result of v8::ScriptCompiler::CachedDataVersionTag().
uint32_t v8_cache_version_tag;
SnapshotFlags flags;
};

Expand Down
23 changes: 0 additions & 23 deletions src/node_snapshotable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ using v8::Isolate;
using v8::Local;
using v8::Object;
using v8::ObjectTemplate;
using v8::ScriptCompiler;
using v8::SnapshotCreator;
using v8::StartupData;
using v8::String;
Expand Down Expand Up @@ -542,7 +541,6 @@ SnapshotMetadata SnapshotDeserializer::Read() {
result.node_version = ReadString();
result.node_arch = ReadString();
result.node_platform = ReadString();
result.v8_cache_version_tag = ReadArithmetic<uint32_t>();
result.flags = static_cast<SnapshotFlags>(ReadArithmetic<uint32_t>());

if (is_debug) {
Expand Down Expand Up @@ -570,9 +568,6 @@ size_t SnapshotSerializer::Write(const SnapshotMetadata& data) {
written_total += WriteString(data.node_arch);
Debug("Write Node.js platform %s\n", data.node_platform);
written_total += WriteString(data.node_platform);
Debug("Write V8 cached data version tag %" PRIx32 "\n",
data.v8_cache_version_tag);
written_total += WriteArithmetic<uint32_t>(data.v8_cache_version_tag);
Debug("Write snapshot flags %" PRIx32 "\n",
static_cast<uint32_t>(data.flags));
written_total += WriteArithmetic<uint32_t>(static_cast<uint32_t>(data.flags));
Expand Down Expand Up @@ -697,23 +692,6 @@ bool SnapshotData::Check() const {
return false;
}

if (metadata.type == SnapshotMetadata::Type::kFullyCustomized &&
!WithoutCodeCache(metadata.flags)) {
uint32_t current_cache_version = v8::ScriptCompiler::CachedDataVersionTag();
if (metadata.v8_cache_version_tag != current_cache_version) {
// For now we only do this check for the customized snapshots - we know
// that the flags we use in the default snapshot are limited and safe
// enough so we can relax the constraints for it.
fprintf(stderr,
"Failed to load the startup snapshot because it was built with "
"a different version of V8 or with different V8 configurations.\n"
"Expected tag %" PRIx32 ", read %" PRIx32 "\n",
current_cache_version,
metadata.v8_cache_version_tag);
return false;
}
}

// TODO(joyeecheung): check incompatible Node.js flags.
return true;
}
Expand Down Expand Up @@ -1180,7 +1158,6 @@ ExitCode SnapshotBuilder::CreateSnapshot(SnapshotData* out,
per_process::metadata.versions.node,
per_process::metadata.arch,
per_process::metadata.platform,
v8::ScriptCompiler::CachedDataVersionTag(),
config->flags};

// We cannot resurrect the handles from the snapshot, so make sure that
Expand Down
7 changes: 7 additions & 0 deletions test/parallel/parallel.status
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ test-fs-read-stream-concurrent-reads: PASS, FLAKY
# https://github.com/nodejs/node/issues/52630
test-error-serdes: PASS, FLAKY

# Until V8 provides a better way to check for flag mismatch without
# making the code cache/snapshot unreproducible, disable the test
# for a preemptive check now. It should idealy fail more gracefully
# with a better checking mechanism.
# https://github.com/nodejs/build/issues/3043
test-snapshot-incompatible: SKIP

[$system==win32]

# Windows on ARM
Expand Down

0 comments on commit 9ee3a72

Please sign in to comment.