From 082e56fe5a5e91f2370207b79c10ec2e785a7b07 Mon Sep 17 00:00:00 2001 From: Sam Maier Date: Mon, 25 Nov 2019 10:29:12 -0500 Subject: [PATCH] Changing checksum implementation to use zlib's adler32 adler32 is strictly faster than the old checksum - see this doc: https://docs.google.com/document/d/1fFhuShavlUwf0FqTc-6L3XLYbAVe0DhpmHSv4oenZL8/edit?pli=1#heading=h.ojvfq6akbz5f adler32 also no longer requires alignment to be maintained. Bug: chromium:833361 Change-Id: I3dbfa699b712aa908c87e6f8261756a4a1209df4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1925562 Commit-Queue: Sam Maier Reviewed-by: Yang Guo Reviewed-by: Jakob Gruber Cr-Commit-Position: refs/heads/master@{#65160} --- BUILD.gn | 2 ++ DEPS | 2 ++ src/heap/read-only-heap.cc | 4 +-- src/heap/read-only-heap.h | 4 +-- src/snapshot/DEPS | 3 ++ src/snapshot/code-serializer.cc | 9 ++---- src/snapshot/code-serializer.h | 11 ++----- src/snapshot/deserializer.h | 4 +-- src/snapshot/serializer-common.cc | 12 ++++++++ src/snapshot/serializer-common.h | 45 +---------------------------- src/snapshot/snapshot-common.cc | 11 +++---- src/snapshot/snapshot-source-sink.h | 5 ++-- src/snapshot/snapshot.h | 20 +++++-------- tools/gcmole/BUILD.gn | 1 + 14 files changed, 45 insertions(+), 88 deletions(-) diff --git a/BUILD.gn b/BUILD.gn index 55db34b23c..27994952e4 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -3301,6 +3301,8 @@ v8_source_set("v8_base_without_compiler") { ] } + deps += [ "//third_party/zlib" ] + if (v8_postmortem_support) { sources += [ "$target_gen_dir/debug-support.cc" ] deps += [ ":postmortem-metadata" ] diff --git a/DEPS b/DEPS index 18f768292f..fba5695b69 100644 --- a/DEPS +++ b/DEPS @@ -256,6 +256,8 @@ deps = { Var('android_url') + '/platform/external/perfetto.git' + '@' + '12dc10e0278cded35205cf84f80a821348cb6c56', 'v8/third_party/protobuf': Var('chromium_url') + '/external/github.com/google/protobuf'+ '@' + 'b68a347f56137b4b1a746e8c7438495a6ac1bd91', + 'v8/third_party/zlib': + Var('chromium_url') + '/chromium/src/third_party/zlib.git'+ '@' + 'e5c4d8c45ed18f84ea68f5029c9bceb1f67268b8', } include_rules = [ diff --git a/src/heap/read-only-heap.cc b/src/heap/read-only-heap.cc index 7fc93737f6..fe10f0e698 100644 --- a/src/heap/read-only-heap.cc +++ b/src/heap/read-only-heap.cc @@ -29,7 +29,7 @@ void ReadOnlyHeap::SetUp(Isolate* isolate, ReadOnlyDeserializer* des) { DCHECK_NOT_NULL(isolate); #ifdef V8_SHARED_RO_HEAP bool call_once_ran = false; - base::Optional des_checksum; + base::Optional des_checksum; #ifdef DEBUG if (des != nullptr) des_checksum = des->GetChecksum(); #endif // DEBUG @@ -50,7 +50,7 @@ void ReadOnlyHeap::SetUp(Isolate* isolate, ReadOnlyDeserializer* des) { USE(call_once_ran); USE(des_checksum); #ifdef DEBUG - const base::Optional last_checksum = + const base::Optional last_checksum = shared_ro_heap_->read_only_blob_checksum_; if (last_checksum || des_checksum) { // The read-only heap was set up from a snapshot. Make sure it's the always diff --git a/src/heap/read-only-heap.h b/src/heap/read-only-heap.h index 531df651be..87c369942b 100644 --- a/src/heap/read-only-heap.h +++ b/src/heap/read-only-heap.h @@ -66,8 +66,6 @@ class ReadOnlyHeap final { ReadOnlySpace* read_only_space() const { return read_only_space_; } private: - using Checksum = std::pair; - // Creates a new read-only heap and attaches it to the provided isolate. static ReadOnlyHeap* CreateAndAttachToIsolate(Isolate* isolate); // Runs the read-only deserailizer and calls InitFromIsolate to complete @@ -86,7 +84,7 @@ class ReadOnlyHeap final { #ifdef V8_SHARED_RO_HEAP #ifdef DEBUG // The checksum of the blob the read-only heap was deserialized from, if any. - base::Optional read_only_blob_checksum_; + base::Optional read_only_blob_checksum_; #endif // DEBUG Address read_only_roots_[kEntriesCount]; diff --git a/src/snapshot/DEPS b/src/snapshot/DEPS index 810dfd6e84..70ef55e340 100644 --- a/src/snapshot/DEPS +++ b/src/snapshot/DEPS @@ -2,4 +2,7 @@ specific_include_rules = { "mksnapshot\.cc": [ "+include/libplatform/libplatform.h", ], + "serializer-common.cc": [ + "+third_party/zlib", + ], } diff --git a/src/snapshot/code-serializer.cc b/src/snapshot/code-serializer.cc index 06efc4cc79..7e12c82b89 100644 --- a/src/snapshot/code-serializer.cc +++ b/src/snapshot/code-serializer.cc @@ -403,9 +403,7 @@ SerializedCodeData::SerializedCodeData(const std::vector* payload, CopyBytes(data_ + padded_payload_offset, payload->data(), static_cast(payload->size())); - Checksum checksum(ChecksummedContent()); - SetHeaderValue(kChecksumPartAOffset, checksum.a()); - SetHeaderValue(kChecksumPartBOffset, checksum.b()); + SetHeaderValue(kChecksumOffset, Checksum(ChecksummedContent())); } SerializedCodeData::SanityCheckResult SerializedCodeData::SanityCheck( @@ -417,8 +415,7 @@ SerializedCodeData::SanityCheckResult SerializedCodeData::SanityCheck( uint32_t source_hash = GetHeaderValue(kSourceHashOffset); uint32_t flags_hash = GetHeaderValue(kFlagHashOffset); uint32_t payload_length = GetHeaderValue(kPayloadLengthOffset); - uint32_t c1 = GetHeaderValue(kChecksumPartAOffset); - uint32_t c2 = GetHeaderValue(kChecksumPartBOffset); + uint32_t c = GetHeaderValue(kChecksumOffset); if (version_hash != Version::Hash()) return VERSION_MISMATCH; if (source_hash != expected_source_hash) return SOURCE_MISMATCH; if (flags_hash != FlagList::Hash()) return FLAGS_MISMATCH; @@ -427,7 +424,7 @@ SerializedCodeData::SanityCheckResult SerializedCodeData::SanityCheck( POINTER_SIZE_ALIGN(kHeaderSize + GetHeaderValue(kNumReservationsOffset) * kInt32Size); if (payload_length > max_payload_length) return LENGTH_MISMATCH; - if (!Checksum(ChecksummedContent()).Check(c1, c2)) return CHECKSUM_MISMATCH; + if (Checksum(ChecksummedContent()) != c) return CHECKSUM_MISMATCH; return CHECK_SUCCESS; } diff --git a/src/snapshot/code-serializer.h b/src/snapshot/code-serializer.h index 78b1cdc77c..ace50b26f3 100644 --- a/src/snapshot/code-serializer.h +++ b/src/snapshot/code-serializer.h @@ -94,8 +94,7 @@ class SerializedCodeData : public SerializedData { // [3] flag hash // [4] number of reservation size entries // [5] payload length - // [6] payload checksum part A - // [7] payload checksum part B + // [6] payload checksum // ... reservations // ... code stub keys // ... serialized payload @@ -105,12 +104,8 @@ class SerializedCodeData : public SerializedData { static const uint32_t kNumReservationsOffset = kFlagHashOffset + kUInt32Size; static const uint32_t kPayloadLengthOffset = kNumReservationsOffset + kUInt32Size; - static const uint32_t kChecksumPartAOffset = - kPayloadLengthOffset + kUInt32Size; - static const uint32_t kChecksumPartBOffset = - kChecksumPartAOffset + kUInt32Size; - static const uint32_t kUnalignedHeaderSize = - kChecksumPartBOffset + kUInt32Size; + static const uint32_t kChecksumOffset = kPayloadLengthOffset + kUInt32Size; + static const uint32_t kUnalignedHeaderSize = kChecksumOffset + kUInt32Size; static const uint32_t kHeaderSize = POINTER_SIZE_ALIGN(kUnalignedHeaderSize); // Used when consuming. diff --git a/src/snapshot/deserializer.h b/src/snapshot/deserializer.h index 9f66c37ac5..c56ded6894 100644 --- a/src/snapshot/deserializer.h +++ b/src/snapshot/deserializer.h @@ -41,9 +41,7 @@ class V8_EXPORT_PRIVATE Deserializer : public SerializerDeserializer { ~Deserializer() override; void SetRehashability(bool v) { can_rehash_ = v; } - std::pair GetChecksum() const { - return source_.GetChecksum(); - } + uint32_t GetChecksum() const { return source_.GetChecksum(); } protected: // Create a deserializer from a snapshot byte source. diff --git a/src/snapshot/serializer-common.cc b/src/snapshot/serializer-common.cc index 3db56ad275..1703af7717 100644 --- a/src/snapshot/serializer-common.cc +++ b/src/snapshot/serializer-common.cc @@ -8,6 +8,7 @@ #include "src/objects/foreign-inl.h" #include "src/objects/objects-inl.h" #include "src/objects/slots.h" +#include "third_party/zlib/zlib.h" namespace v8 { namespace internal { @@ -145,5 +146,16 @@ void SerializerDeserializer::RestoreExternalReferenceRedirectors( } } +V8_EXPORT_PRIVATE extern uint32_t Checksum(Vector payload) { +#ifdef MEMORY_SANITIZER + // Computing the checksum includes padding bytes for objects like strings. + // Mark every object as initialized in the code serializer. + MSAN_MEMORY_IS_INITIALIZED(payload.begin(), payload.length()); +#endif // MEMORY_SANITIZER + // Priming the adler32 call so it can see what CPU features are available. + adler32(0, NULL, 0); + return static_cast(adler32(0, payload.begin(), payload.length())); +} + } // namespace internal } // namespace v8 diff --git a/src/snapshot/serializer-common.h b/src/snapshot/serializer-common.h index 9f0085a462..3636da3aa4 100644 --- a/src/snapshot/serializer-common.h +++ b/src/snapshot/serializer-common.h @@ -358,50 +358,7 @@ class SerializedData { DISALLOW_COPY_AND_ASSIGN(SerializedData); }; -class Checksum { - public: - explicit Checksum(Vector payload) { -#ifdef MEMORY_SANITIZER - // Computing the checksum includes padding bytes for objects like strings. - // Mark every object as initialized in the code serializer. - MSAN_MEMORY_IS_INITIALIZED(payload.begin(), payload.length()); -#endif // MEMORY_SANITIZER - // Fletcher's checksum. Modified to reduce 64-bit sums to 32-bit. - uintptr_t a = 1; - uintptr_t b = 0; - // TODO(jgruber, v8:9171): The following DCHECK should ideally hold since we - // access payload through an uintptr_t pointer later on; and some - // architectures, e.g. arm, may generate instructions that expect correct - // alignment. However, we do not control alignment for external snapshots. - // DCHECK(IsAligned(reinterpret_cast(payload.begin()), - // kIntptrSize)); - DCHECK(IsAligned(payload.length(), kIntptrSize)); - const uintptr_t* cur = reinterpret_cast(payload.begin()); - const uintptr_t* end = cur + payload.length() / kIntptrSize; - while (cur < end) { - // Unsigned overflow expected and intended. - a += *cur++; - b += a; - } -#if V8_HOST_ARCH_64_BIT - a ^= a >> 32; - b ^= b >> 32; -#endif // V8_HOST_ARCH_64_BIT - a_ = static_cast(a); - b_ = static_cast(b); - } - - bool Check(uint32_t a, uint32_t b) const { return a == a_ && b == b_; } - - uint32_t a() const { return a_; } - uint32_t b() const { return b_; } - - private: - uint32_t a_; - uint32_t b_; - - DISALLOW_COPY_AND_ASSIGN(Checksum); -}; +V8_EXPORT_PRIVATE uint32_t Checksum(Vector payload); } // namespace internal } // namespace v8 diff --git a/src/snapshot/snapshot-common.cc b/src/snapshot/snapshot-common.cc index f489999f88..8e0c28c6d6 100644 --- a/src/snapshot/snapshot-common.cc +++ b/src/snapshot/snapshot-common.cc @@ -192,9 +192,7 @@ v8::StartupData Snapshot::CreateSnapshotBlob( DCHECK_EQ(total_length, payload_offset); v8::StartupData result = {data, static_cast(total_length)}; - Checksum checksum(ChecksummedContent(&result)); - SetHeaderValue(data, kChecksumPartAOffset, checksum.a()); - SetHeaderValue(data, kChecksumPartBOffset, checksum.b()); + SetHeaderValue(data, kChecksumOffset, Checksum(ChecksummedContent(&result))); return result; } @@ -208,14 +206,13 @@ uint32_t Snapshot::ExtractNumContexts(const v8::StartupData* data) { bool Snapshot::VerifyChecksum(const v8::StartupData* data) { base::ElapsedTimer timer; if (FLAG_profile_deserialization) timer.Start(); - uint32_t expected_a = GetHeaderValue(data, kChecksumPartAOffset); - uint32_t expected_b = GetHeaderValue(data, kChecksumPartBOffset); - Checksum checksum(ChecksummedContent(data)); + uint32_t expected = GetHeaderValue(data, kChecksumOffset); + uint32_t result = Checksum(ChecksummedContent(data)); if (FLAG_profile_deserialization) { double ms = timer.Elapsed().InMillisecondsF(); PrintF("[Verifying snapshot checksum took %0.3f ms]\n", ms); } - return checksum.Check(expected_a, expected_b); + return result == expected; } uint32_t Snapshot::ExtractContextOffset(const v8::StartupData* data, diff --git a/src/snapshot/snapshot-source-sink.h b/src/snapshot/snapshot-source-sink.h index 9cdb85089e..adcfa8df61 100644 --- a/src/snapshot/snapshot-source-sink.h +++ b/src/snapshot/snapshot-source-sink.h @@ -87,9 +87,8 @@ class SnapshotByteSource final { int position() { return position_; } void set_position(int position) { position_ = position; } - std::pair GetChecksum() const { - Checksum checksum(Vector(data_, length_)); - return {checksum.a(), checksum.b()}; + uint32_t GetChecksum() const { + return Checksum(Vector(data_, length_)); } private: diff --git a/src/snapshot/snapshot.h b/src/snapshot/snapshot.h index 4a4da9f755..156d873019 100644 --- a/src/snapshot/snapshot.h +++ b/src/snapshot/snapshot.h @@ -111,12 +111,11 @@ class Snapshot : public AllStatic { // Snapshot blob layout: // [0] number of contexts N // [1] rehashability - // [2] checksum part A - // [3] checksum part B - // [4] (128 bytes) version string - // [5] offset to readonly - // [6] offset to context 0 - // [7] offset to context 1 + // [2] checksum + // [3] (128 bytes) version string + // [4] offset to readonly + // [5] offset to context 0 + // [6] offset to context 1 // ... // ... offset to context N - 1 // ... startup snapshot data @@ -128,12 +127,8 @@ class Snapshot : public AllStatic { // TODO(yangguo): generalize rehashing, and remove this flag. static const uint32_t kRehashabilityOffset = kNumberOfContextsOffset + kUInt32Size; - static const uint32_t kChecksumPartAOffset = - kRehashabilityOffset + kUInt32Size; - static const uint32_t kChecksumPartBOffset = - kChecksumPartAOffset + kUInt32Size; - static const uint32_t kVersionStringOffset = - kChecksumPartBOffset + kUInt32Size; + static const uint32_t kChecksumOffset = kRehashabilityOffset + kUInt32Size; + static const uint32_t kVersionStringOffset = kChecksumOffset + kUInt32Size; static const uint32_t kVersionStringLength = 64; static const uint32_t kReadOnlyOffsetOffset = kVersionStringOffset + kVersionStringLength; @@ -141,6 +136,7 @@ class Snapshot : public AllStatic { kReadOnlyOffsetOffset + kUInt32Size; static Vector ChecksummedContent(const v8::StartupData* data) { + STATIC_ASSERT(kVersionStringOffset == kChecksumOffset + kUInt32Size); const uint32_t kChecksumStart = kVersionStringOffset; return Vector( reinterpret_cast(data->data + kChecksumStart), diff --git a/tools/gcmole/BUILD.gn b/tools/gcmole/BUILD.gn index ba2d67fd79..fc7d81e561 100644 --- a/tools/gcmole/BUILD.gn +++ b/tools/gcmole/BUILD.gn @@ -31,6 +31,7 @@ group("v8_run_gcmole") { "../../third_party/icu/source/", "../../third_party/wasm-api/wasm.h", "../../third_party/wasm-api/wasm.hh", + "../../third_party/zlib/", "../../third_party/inspector_protocol/", "$target_gen_dir/../../", "$target_gen_dir/../../torque-generated/",