From 9eb5d0b842066a67ae0febdb951ccf2756eb1786 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 30 Jul 2024 13:06:14 +0200 Subject: [PATCH 1/2] src: remove cached data tag from snapshot metadata 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. --- src/env.cc | 1 - src/env.h | 2 -- src/node_snapshotable.cc | 22 ---------------------- test/parallel/parallel.status | 7 +++++++ 4 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/env.cc b/src/env.cc index 83a4ca3be186f7..329cf2c189934e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -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; diff --git a/src/env.h b/src/env.h index 0dd3a0d0b1832a..06102af620fa7e 100644 --- a/src/env.h +++ b/src/env.h @@ -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; }; diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 426066fb9d63d4..54ae7ac5f54373 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -542,7 +542,6 @@ SnapshotMetadata SnapshotDeserializer::Read() { result.node_version = ReadString(); result.node_arch = ReadString(); result.node_platform = ReadString(); - result.v8_cache_version_tag = ReadArithmetic(); result.flags = static_cast(ReadArithmetic()); if (is_debug) { @@ -570,9 +569,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(data.v8_cache_version_tag); Debug("Write snapshot flags %" PRIx32 "\n", static_cast(data.flags)); written_total += WriteArithmetic(static_cast(data.flags)); @@ -697,23 +693,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; } @@ -1180,7 +1159,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 diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 50ba57dcbdf3af..ad541eb1c8a8b9 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -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 From d93f0a018db1d72226b43683572a58f586bbef94 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 19 Aug 2024 13:49:41 +0200 Subject: [PATCH 2/2] fixup! src: remove cached data tag from snapshot metadata --- src/node_snapshotable.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 54ae7ac5f54373..fe04a8ee8d708b 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -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;