From 1eab04b7264552325cfe3b5bbbd9f6f9be4f27e7 Mon Sep 17 00:00:00 2001 From: Adam Klein Date: Tue, 18 Jun 2019 23:13:52 +0000 Subject: [PATCH] Revert "[roheap] Check that ro-heap is always passed the same read-only snapshot" This reverts commit a5fa211f3091f6966a005a0502693c521e5636d1. Reason for revert: breaks ARM Lite builder: https://ci.chromium.org/p/v8/builders/ci/V8%20Linux%20-%20arm%20-%20sim%20-%20lite/4843 Original change's description: > [roheap] Check that ro-heap is always passed the same read-only snapshot > > Previously the ReadOnlyHeap simply discarded all but the first > ReadOnlyDeseralizer. ClearSharedHeapForTest should be called if using a > new ReadOnlyDeserializer (this might change in the future). > > Remove an obsolete 'StartupSerializerRootMapDependencies' test. It used > to test Map::WeakCellForMap which doesn't exist anymore and was > difficult to adapt to a shared read-only heap. > > Bug: v8:7464 > Change-Id: I64b8e953b0e3466e003541ec8a9321e439a01d33 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1660612 > Reviewed-by: Yang Guo > Reviewed-by: Dan Elphick > Commit-Queue: Maciej Goszczycki > Cr-Commit-Position: refs/heads/master@{#62250} TBR=yangguo@chromium.org,delphick@chromium.org,goszczycki@google.com Change-Id: I099544913bec3bbd67840b1818a6ad6029fdf380 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:7464 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1666453 Reviewed-by: Adam Klein Commit-Queue: Adam Klein Cr-Commit-Position: refs/heads/master@{#62264} --- src/heap/read-only-heap.cc | 41 ++++------------- src/heap/read-only-heap.h | 12 +---- src/snapshot/deserializer.h | 4 -- src/snapshot/snapshot-source-sink.h | 8 ---- test/cctest/test-serialize.cc | 69 ++++++++++++++++++++--------- 5 files changed, 58 insertions(+), 76 deletions(-) diff --git a/src/heap/read-only-heap.cc b/src/heap/read-only-heap.cc index ee467ea3b9..12bac2fce2 100644 --- a/src/heap/read-only-heap.cc +++ b/src/heap/read-only-heap.cc @@ -28,39 +28,16 @@ ReadOnlyHeap* ReadOnlyHeap::shared_ro_heap_ = nullptr; void ReadOnlyHeap::SetUp(Isolate* isolate, ReadOnlyDeserializer* des) { DCHECK_NOT_NULL(isolate); #ifdef V8_SHARED_RO_HEAP - bool call_once_ran = false; -#ifdef DEBUG - base::Optional des_checksum; - if (des != nullptr) des_checksum = des->GetChecksum(); -#endif // DEBUG + // Make sure we are only sharing read-only space when deserializing. Otherwise + // we would be trying to create heap objects inside an already initialized + // read-only space. Use ClearSharedHeapForTest if you need a new read-only + // space. + DCHECK_IMPLIES(shared_ro_heap_ != nullptr, des != nullptr); - base::CallOnce(&setup_ro_heap_once, - [isolate, des, des_checksum, &call_once_ran]() { - shared_ro_heap_ = CreateAndAttachToIsolate(isolate); - if (des != nullptr) { -#ifdef DEBUG - shared_ro_heap_->read_only_blob_checksum_ = des_checksum; -#endif // DEBUG - shared_ro_heap_->DeseralizeIntoIsolate(isolate, des); - } -#ifdef DEBUG - call_once_ran = true; -#endif // DEBUG - }); - -#ifdef DEBUG - 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 - // the same snapshot. - CHECK_EQ(last_checksum, des_checksum); - } else { - // The read-only heap objects were created. Make sure this happens only - // once, during this call. - CHECK(call_once_ran); - } -#endif // DEBUG + base::CallOnce(&setup_ro_heap_once, [isolate, des]() { + shared_ro_heap_ = CreateAndAttachToIsolate(isolate); + if (des != nullptr) shared_ro_heap_->DeseralizeIntoIsolate(isolate, des); + }); isolate->heap()->SetUpFromReadOnlyHeap(shared_ro_heap_); if (des != nullptr) { diff --git a/src/heap/read-only-heap.h b/src/heap/read-only-heap.h index 4c1da62a15..59e3e431d4 100644 --- a/src/heap/read-only-heap.h +++ b/src/heap/read-only-heap.h @@ -5,10 +5,7 @@ #ifndef V8_HEAP_READ_ONLY_HEAP_H_ #define V8_HEAP_READ_ONLY_HEAP_H_ -#include - #include "src/base/macros.h" -#include "src/base/optional.h" #include "src/objects/heap-object.h" #include "src/objects/objects.h" #include "src/roots/roots.h" @@ -64,8 +61,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 @@ -82,15 +77,10 @@ class ReadOnlyHeap final { std::vector read_only_object_cache_; #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_; -#endif // DEBUG - Address read_only_roots_[kEntriesCount]; V8_EXPORT_PRIVATE static ReadOnlyHeap* shared_ro_heap_; -#endif // V8_SHARED_RO_HEAP +#endif explicit ReadOnlyHeap(ReadOnlySpace* ro_space) : read_only_space_(ro_space) {} DISALLOW_COPY_AND_ASSIGN(ReadOnlyHeap); diff --git a/src/snapshot/deserializer.h b/src/snapshot/deserializer.h index b1c7f4214e..6e3f497d38 100644 --- a/src/snapshot/deserializer.h +++ b/src/snapshot/deserializer.h @@ -5,7 +5,6 @@ #ifndef V8_SNAPSHOT_DESERIALIZER_H_ #define V8_SNAPSHOT_DESERIALIZER_H_ -#include #include #include "src/objects/allocation-site.h" @@ -40,9 +39,6 @@ class V8_EXPORT_PRIVATE Deserializer : public SerializerDeserializer { ~Deserializer() override; void SetRehashability(bool v) { can_rehash_ = v; } - std::pair GetChecksum() const { - return source_.GetChecksum(); - } protected: // Create a deserializer from a snapshot byte source. diff --git a/src/snapshot/snapshot-source-sink.h b/src/snapshot/snapshot-source-sink.h index f20f2ad33f..61396aaa71 100644 --- a/src/snapshot/snapshot-source-sink.h +++ b/src/snapshot/snapshot-source-sink.h @@ -5,10 +5,7 @@ #ifndef V8_SNAPSHOT_SNAPSHOT_SOURCE_SINK_H_ #define V8_SNAPSHOT_SNAPSHOT_SOURCE_SINK_H_ -#include - #include "src/base/logging.h" -#include "src/snapshot/serializer-common.h" #include "src/utils/utils.h" namespace v8 { @@ -69,11 +66,6 @@ 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()}; - } - private: const byte* data_; int length_; diff --git a/test/cctest/test-serialize.cc b/test/cctest/test-serialize.cc index a263dd41b6..78c2ccef42 100644 --- a/test/cctest/test-serialize.cc +++ b/test/cctest/test-serialize.cc @@ -148,10 +148,8 @@ namespace { // Convenience wrapper around the convenience wrapper. v8::StartupData CreateSnapshotDataBlob(const char* embedded_source) { - v8::StartupData data = CreateSnapshotDataBlobInternal( + return CreateSnapshotDataBlobInternal( v8::SnapshotCreator::FunctionCodeHandling::kClear, embedded_source); - ReadOnlyHeap::ClearSharedHeapForTest(); - return data; } } // namespace @@ -277,6 +275,53 @@ UNINITIALIZED_TEST(StartupSerializerOnce32K) { TestStartupSerializerOnceImpl(); } +UNINITIALIZED_TEST(StartupSerializerRootMapDependencies) { + DisableAlwaysOpt(); + v8::SnapshotCreator snapshot_creator; + v8::Isolate* isolate = snapshot_creator.GetIsolate(); + { + v8::Isolate::Scope isolate_scope(isolate); + v8::HandleScope handle_scope(isolate); + Isolate* internal_isolate = reinterpret_cast(isolate); + // Here is interesting retaining path: + // - FreeSpaceMap + // - Map for Map types itself + // - NullValue + // - Internalized one byte string + // - Map for Internalized one byte string + // - TheHoleValue + // - HeapNumber + // HeapNumber objects require kDoubleUnaligned on 32-bit + // platforms. So, without special measures we're risking to serialize + // object, requiring alignment before FreeSpaceMap is fully serialized. + v8::internal::Handle map( + ReadOnlyRoots(internal_isolate).one_byte_internalized_string_map(), + internal_isolate); + // Need to avoid DCHECKs inside SnapshotCreator. + snapshot_creator.SetDefaultContext(v8::Context::New(isolate)); + } + + v8::StartupData startup_data = snapshot_creator.CreateBlob( + v8::SnapshotCreator::FunctionCodeHandling::kKeep); + + v8::Isolate::CreateParams params; + params.snapshot_blob = &startup_data; + params.array_buffer_allocator = CcTest::array_buffer_allocator(); + isolate = v8::Isolate::New(params); + + { + v8::HandleScope handle_scope(isolate); + v8::Isolate::Scope isolate_scope(isolate); + + v8::Local env = v8::Context::New(isolate); + env->Enter(); + + SanityCheck(isolate); + } + isolate->Dispose(); + delete[] startup_data.data; +} + UNINITIALIZED_TEST(StartupSerializerTwice) { DisableAlwaysOpt(); v8::Isolate* isolate = TestSerializer::NewIsolateInitialized(); @@ -766,7 +811,6 @@ void TestCustomSnapshotDataBlobWithIrregexpCode( DisableEmbeddedBlobRefcounting(); v8::StartupData data1 = CreateSnapshotDataBlobInternal(function_code_handling, source); - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params1; params1.snapshot_blob = &data1; @@ -914,7 +958,6 @@ void TypedArrayTestHelper( creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear); } - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams create_params; create_params.snapshot_blob = &blob; create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -1042,7 +1085,6 @@ UNINITIALIZED_TEST(CustomSnapshotDataBlobDetachedArrayBuffer) { creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear); } - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams create_params; create_params.snapshot_blob = &blob; create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -1113,7 +1155,6 @@ UNINITIALIZED_TEST(CustomSnapshotDataBlobOnOrOffHeapTypedArray) { creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear); } - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams create_params; create_params.snapshot_blob = &blob; create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -1167,7 +1208,6 @@ UNINITIALIZED_TEST(CustomSnapshotDataBlobTypedArrayNoEmbedderFieldCallback) { creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear); } - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams create_params; create_params.snapshot_blob = &blob; create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2551,7 +2591,6 @@ UNINITIALIZED_TEST(SnapshotCreatorMultipleContexts) { creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear); } - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2690,7 +2729,6 @@ UNINITIALIZED_TEST(SnapshotCreatorExternalReferences) { // Deserialize with the original external reference. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2716,7 +2754,6 @@ UNINITIALIZED_TEST(SnapshotCreatorExternalReferences) { // Deserialize with some other external reference. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2765,7 +2802,6 @@ UNINITIALIZED_TEST(SnapshotCreatorShortExternalReferences) { // Deserialize with an incomplete list of external references. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2826,7 +2862,6 @@ UNINITIALIZED_TEST(SnapshotCreatorNoExternalReferencesDefault) { // Deserialize with an incomplete list of external references. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2876,7 +2911,6 @@ UNINITIALIZED_TEST(SnapshotCreatorPreparseDataAndNoOuterScope) { // Deserialize with an incomplete list of external references. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2916,7 +2950,6 @@ UNINITIALIZED_TEST(SnapshotCreatorArrayJoinWithKeep) { DisableAlwaysOpt(); DisableEmbeddedBlobRefcounting(); v8::StartupData blob = CreateCustomSnapshotArrayJoinWithKeep(); - ReadOnlyHeap::ClearSharedHeapForTest(); // Deserialize with an incomplete list of external references. { @@ -2944,7 +2977,6 @@ TEST(SnapshotCreatorNoExternalReferencesCustomFail1) { // Deserialize with an incomplete list of external references. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -2970,7 +3002,6 @@ TEST(SnapshotCreatorNoExternalReferencesCustomFail2) { // Deserialize with an incomplete list of external references. { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -3089,7 +3120,6 @@ UNINITIALIZED_TEST(SnapshotCreatorTemplates) { } { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -3503,7 +3533,6 @@ UNINITIALIZED_TEST(SnapshotCreatorIncludeGlobalProxy) { } { - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams params; params.snapshot_blob = &blob; params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -3638,7 +3667,6 @@ UNINITIALIZED_TEST(ReinitializeHashSeedNotRehashable) { CHECK(!blob.CanBeRehashed()); } - ReadOnlyHeap::ClearSharedHeapForTest(); i::FLAG_hash_seed = 1337; v8::Isolate::CreateParams create_params; create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); @@ -3774,7 +3802,6 @@ UNINITIALIZED_TEST(WeakArraySerializationInSnapshot) { creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear); } - ReadOnlyHeap::ClearSharedHeapForTest(); v8::Isolate::CreateParams create_params; create_params.snapshot_blob = &blob; create_params.array_buffer_allocator = CcTest::array_buffer_allocator();