From 87ce9fce749c8f1fa41dee2bd9499391dca76d49 Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Tue, 6 Sep 2022 13:50:55 -0700 Subject: [PATCH] [shared-struct] Rework shared value serializer API again This CL fixes redesigns the current API, which does not correctly manage lifetimes of the shared object conveyors. See design doc at https://docs.google.com/document/d/1TV6agY9dafVJFvdPrUAGbEvos8wL2WDnsmf84n3OJVU/edit?usp=sharing This CL also removes the incorrect behavior of serializing all shared strings by sharing instead of copying. Shared strings may be sent to another process, which should still work. Bug: v8:12547 Change-Id: I7413abd2d871fd3d52c9b433445cfa1d03e4a732 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3868713 Commit-Queue: Shu-yu Guo Reviewed-by: Adam Klein Reviewed-by: Michael Lippautz Cr-Commit-Position: refs/heads/main@{#83044} --- BUILD.bazel | 4 +- BUILD.gn | 4 +- include/v8-value-serializer.h | 51 +++++++++- src/api/api.cc | 34 ++++++- src/d8/d8.cc | 54 ++++------- src/d8/d8.h | 8 +- src/execution/isolate.cc | 3 - src/execution/isolate.h | 15 --- src/handles/shared-object-conveyor-handles.cc | 25 +++++ src/handles/shared-object-conveyor-handles.h | 56 +++++++++++ src/handles/shared-object-conveyors.cc | 78 --------------- src/handles/shared-object-conveyors.h | 82 ---------------- src/objects/value-serializer.cc | 94 ++++++------------- src/objects/value-serializer.h | 4 +- test/mjsunit/shared-memory/shared-string.js | 36 ++++--- .../shared-memory/shared-struct-workers.js | 6 -- .../mjsunit/shared-memory/value-serializer.js | 13 +-- 17 files changed, 247 insertions(+), 320 deletions(-) create mode 100644 src/handles/shared-object-conveyor-handles.cc create mode 100644 src/handles/shared-object-conveyor-handles.h delete mode 100644 src/handles/shared-object-conveyors.cc delete mode 100644 src/handles/shared-object-conveyors.h diff --git a/BUILD.bazel b/BUILD.bazel index be2fe622dd..4e89f90e7e 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1390,8 +1390,8 @@ filegroup( "src/handles/maybe-handles.h", "src/handles/persistent-handles.cc", "src/handles/persistent-handles.h", - "src/handles/shared-object-conveyors.cc", - "src/handles/shared-object-conveyors.h", + "src/handles/shared-object-conveyor-handles.cc", + "src/handles/shared-object-conveyor-handles.h", "src/heap/base/active-system-pages.cc", "src/heap/base/active-system-pages.h", "src/heap/allocation-observer.cc", diff --git a/BUILD.gn b/BUILD.gn index bdd6854ec8..f80b01dc10 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -3089,7 +3089,7 @@ v8_header_set("v8_internal_headers") { "src/handles/maybe-handles-inl.h", "src/handles/maybe-handles.h", "src/handles/persistent-handles.h", - "src/handles/shared-object-conveyors.h", + "src/handles/shared-object-conveyor-handles.h", "src/heap/allocation-observer.h", "src/heap/allocation-result.h", "src/heap/allocation-stats.h", @@ -4497,7 +4497,7 @@ v8_source_set("v8_base_without_compiler") { "src/handles/handles.cc", "src/handles/local-handles.cc", "src/handles/persistent-handles.cc", - "src/handles/shared-object-conveyors.cc", + "src/handles/shared-object-conveyor-handles.cc", "src/heap/allocation-observer.cc", "src/heap/array-buffer-sweeper.cc", "src/heap/base-space.cc", diff --git a/include/v8-value-serializer.h b/include/v8-value-serializer.h index 779a042efa..729730c608 100644 --- a/include/v8-value-serializer.h +++ b/include/v8-value-serializer.h @@ -8,6 +8,7 @@ #include #include +#include #include #include "v8-local-handle.h" // NOLINT(build/include_directory) @@ -26,8 +27,37 @@ class Value; namespace internal { struct ScriptStreamingData; +class SharedObjectConveyorHandles; +class ValueDeserializer; +class ValueSerializer; } // namespace internal +/** + * A move-only class for managing the lifetime of shared value conveyors used + * by V8 to keep JS shared values alive in transit when serialized. + * + * This class is not directly constructible and is always passed to a + * ValueSerializer::Delegate via ValueSerializer::SetSharedValueConveyor. + * + * The embedder must not destruct the SharedValueConveyor until the associated + * serialized data will no longer be deserialized. + */ +class V8_EXPORT SharedValueConveyor final { + public: + SharedValueConveyor(SharedValueConveyor&&) noexcept; + ~SharedValueConveyor(); + + SharedValueConveyor& operator=(SharedValueConveyor&&) noexcept; + + private: + friend class internal::ValueSerializer; + friend class internal::ValueDeserializer; + + explicit SharedValueConveyor(Isolate* isolate); + + std::unique_ptr private_; +}; + /** * Value serialization compatible with the HTML structured clone algorithm. * The format is backward-compatible (i.e. safe to store to disk). @@ -69,9 +99,20 @@ class V8_EXPORT ValueSerializer { Isolate* isolate, Local module); /** - * Returns whether conveying shared values are supported. + * Called when the first shared value is serialized. All subsequent shared + * values will use the same conveyor. + * + * The embedder must ensure the lifetime of the conveyor matches the + * lifetime of the serialized data. + * + * If the embedder supports serializing shared values, this method should + * return true. Otherwise the embedder should throw an exception and return + * false. + * + * This method is called at most once per serializer. */ - virtual bool SupportsSharedValues() const; + virtual bool AdoptSharedValueConveyor(Isolate* isolate, + SharedValueConveyor&& conveyor); /** * Allocates memory for the buffer of at least the size provided. The actual @@ -183,6 +224,12 @@ class V8_EXPORT ValueDeserializer { */ virtual MaybeLocal GetSharedArrayBufferFromId( Isolate* isolate, uint32_t clone_id); + + /** + * Get the SharedValueConveyor previously provided by + * ValueSerializer::Delegate::AdoptSharedValueConveyor. + */ + virtual const SharedValueConveyor* GetSharedValueConveyor(Isolate* isolate); }; ValueDeserializer(Isolate* isolate, const uint8_t* data, size_t size); diff --git a/src/api/api.cc b/src/api/api.cc index 99d03f3613..04cfe90c08 100644 --- a/src/api/api.cc +++ b/src/api/api.cc @@ -62,6 +62,7 @@ #include "src/execution/vm-state-inl.h" #include "src/handles/global-handles.h" #include "src/handles/persistent-handles.h" +#include "src/handles/shared-object-conveyor-handles.h" #include "src/heap/embedder-tracing.h" #include "src/heap/heap-inl.h" #include "src/heap/heap-write-barrier.h" @@ -3330,6 +3331,21 @@ MaybeLocal JSON::Stringify(Local context, // --- V a l u e S e r i a l i z a t i o n --- +SharedValueConveyor::SharedValueConveyor(SharedValueConveyor&& other) noexcept + : private_(std::move(other.private_)) {} + +SharedValueConveyor::~SharedValueConveyor() = default; + +SharedValueConveyor& SharedValueConveyor::operator=( + SharedValueConveyor&& other) noexcept { + private_ = std::move(other.private_); + return *this; +} + +SharedValueConveyor::SharedValueConveyor(Isolate* v8_isolate) + : private_(std::make_unique( + reinterpret_cast(v8_isolate))) {} + Maybe ValueSerializer::Delegate::WriteHostObject(Isolate* v8_isolate, Local object) { i::Isolate* i_isolate = reinterpret_cast(v8_isolate); @@ -3353,7 +3369,14 @@ Maybe ValueSerializer::Delegate::GetWasmModuleTransferId( return Nothing(); } -bool ValueSerializer::Delegate::SupportsSharedValues() const { return false; } +bool ValueSerializer::Delegate::AdoptSharedValueConveyor( + Isolate* v8_isolate, SharedValueConveyor&& conveyor) { + i::Isolate* i_isolate = reinterpret_cast(v8_isolate); + i_isolate->ScheduleThrow(*i_isolate->factory()->NewError( + i_isolate->error_function(), i::MessageTemplate::kDataCloneError, + i_isolate->factory()->NewStringFromAsciiChecked("shared value"))); + return false; +} void* ValueSerializer::Delegate::ReallocateBufferMemory(void* old_buffer, size_t size, @@ -3454,6 +3477,15 @@ ValueDeserializer::Delegate::GetSharedArrayBufferFromId(Isolate* v8_isolate, return MaybeLocal(); } +const SharedValueConveyor* ValueDeserializer::Delegate::GetSharedValueConveyor( + Isolate* v8_isolate) { + i::Isolate* i_isolate = reinterpret_cast(v8_isolate); + i_isolate->ScheduleThrow(*i_isolate->factory()->NewError( + i_isolate->error_function(), + i::MessageTemplate::kDataCloneDeserializationError)); + return nullptr; +} + struct ValueDeserializer::PrivateData { PrivateData(i::Isolate* i_isolate, base::Vector data, Delegate* delegate) diff --git a/src/d8/d8.cc b/src/d8/d8.cc index b788a8a749..8c46cc6a9f 100644 --- a/src/d8/d8.cc +++ b/src/d8/d8.cc @@ -2712,23 +2712,6 @@ void Shell::WorkerNew(const v8::FunctionCallbackInfo& args) { } } -namespace { -bool GetDisableSharedValuesSupportOption(Isolate* isolate, - Local options) { - if (options->IsObject()) { - Local context = isolate->GetCurrentContext(); - Local options_obj = options->ToObject(context).ToLocalChecked(); - Local name = String::NewFromUtf8Literal( - isolate, "disableSharedValuesSupport", NewStringType::kNormal); - Local value; - if (options_obj->Get(context, name).ToLocal(&value)) { - return value->BooleanValue(isolate); - } - } - return false; -} -} // namespace - void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); HandleScope handle_scope(isolate); @@ -2747,12 +2730,8 @@ void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo& args) { Local message = args[0]; Local transfer = args.Length() >= 2 ? args[1] : Undefined(isolate).As(); - Local options = - args.Length() >= 3 ? args[2] : Undefined(isolate).As(); - bool supports_shared_values = - !GetDisableSharedValuesSupportOption(isolate, options); std::unique_ptr data = - Shell::SerializeValue(isolate, message, transfer, supports_shared_values); + Shell::SerializeValue(isolate, message, transfer); if (data) { worker->PostMessage(std::move(data)); } @@ -4627,12 +4606,8 @@ void Worker::PostMessageOut(const v8::FunctionCallbackInfo& args) { Local message = args[0]; Local transfer = Undefined(isolate); - Local options = - args.Length() >= 3 ? args[2] : Undefined(isolate).As(); - bool supports_shared_values = - !GetDisableSharedValuesSupportOption(isolate, options); std::unique_ptr data = - Shell::SerializeValue(isolate, message, transfer, supports_shared_values); + Shell::SerializeValue(isolate, message, transfer); if (data) { DCHECK(args.Data()->IsExternal()); Local this_value = args.Data().As(); @@ -5184,9 +5159,8 @@ bool Shell::HandleUnhandledPromiseRejections(Isolate* isolate) { class Serializer : public ValueSerializer::Delegate { public: - Serializer(Isolate* isolate, bool supports_shared_values) - : supports_shared_values_(supports_shared_values), - isolate_(isolate), + explicit Serializer(Isolate* isolate) + : isolate_(isolate), serializer_(isolate, this), current_memory_usage_(0) {} @@ -5277,7 +5251,11 @@ class Serializer : public ValueSerializer::Delegate { void FreeBufferMemory(void* buffer) override { base::Free(buffer); } - bool SupportsSharedValues() const override { return supports_shared_values_; } + bool AdoptSharedValueConveyor(Isolate* isolate, + SharedValueConveyor&& conveyor) override { + data_->shared_value_conveyor_.emplace(std::move(conveyor)); + return true; + } private: Maybe PrepareTransfer(Local context, Local transfer) { @@ -5337,7 +5315,6 @@ class Serializer : public ValueSerializer::Delegate { } // This must come before ValueSerializer as it caches this value. - bool supports_shared_values_; Isolate* isolate_; ValueSerializer serializer_; std::unique_ptr data_; @@ -5394,6 +5371,14 @@ class Deserializer : public ValueDeserializer::Delegate { isolate_, data_->compiled_wasm_modules().at(transfer_id)); } + const SharedValueConveyor* GetSharedValueConveyor(Isolate* isolate) override { + DCHECK_NOT_NULL(data_); + if (data_->shared_value_conveyor()) { + return &data_->shared_value_conveyor().value(); + } + return nullptr; + } + private: Isolate* isolate_; ValueDeserializer deserializer_; @@ -5428,11 +5413,10 @@ class D8Testing { }; std::unique_ptr Shell::SerializeValue( - Isolate* isolate, Local value, Local transfer, - bool supports_shared_values) { + Isolate* isolate, Local value, Local transfer) { bool ok; Local context = isolate->GetCurrentContext(); - Serializer serializer(isolate, supports_shared_values); + Serializer serializer(isolate); std::unique_ptr data; if (serializer.WriteValue(context, value, transfer).To(&ok)) { data = serializer.Release(); diff --git a/src/d8/d8.h b/src/d8/d8.h index c858bdbd33..00e71a12e0 100644 --- a/src/d8/d8.h +++ b/src/d8/d8.h @@ -17,6 +17,7 @@ #include "include/v8-array-buffer.h" #include "include/v8-isolate.h" #include "include/v8-script.h" +#include "include/v8-value-serializer.h" #include "src/base/once.h" #include "src/base/platform/time.h" #include "src/base/platform/wrappers.h" @@ -149,6 +150,9 @@ class SerializationData { const std::vector& compiled_wasm_modules() { return compiled_wasm_modules_; } + const base::Optional& shared_value_conveyor() { + return shared_value_conveyor_; + } private: struct DataDeleter { @@ -160,6 +164,7 @@ class SerializationData { std::vector> backing_stores_; std::vector> sab_backing_stores_; std::vector compiled_wasm_modules_; + base::Optional shared_value_conveyor_; private: friend class Serializer; @@ -526,8 +531,7 @@ class Shell : public i::AllStatic { static void PostBlockingBackgroundTask(std::unique_ptr task); static std::unique_ptr SerializeValue( - Isolate* isolate, Local value, Local transfer, - bool supports_shared_values); + Isolate* isolate, Local value, Local transfer); static MaybeLocal DeserializeValue( Isolate* isolate, std::unique_ptr data); static int* LookupCounter(const char* name); diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc index 981442327e..fa360401ef 100644 --- a/src/execution/isolate.cc +++ b/src/execution/isolate.cc @@ -61,7 +61,6 @@ #include "src/execution/vm-state-inl.h" #include "src/handles/global-handles-inl.h" #include "src/handles/persistent-handles.h" -#include "src/handles/shared-object-conveyors.h" #include "src/heap/heap-inl.h" #include "src/heap/heap-verifier.h" #include "src/heap/local-heap.h" @@ -3433,8 +3432,6 @@ Isolate::Isolate(std::unique_ptr isolate_allocator, #endif next_module_async_evaluating_ordinal_( SourceTextModule::kFirstAsyncEvaluatingOrdinal), - shared_object_conveyors_(is_shared ? new SharedObjectConveyors(this) - : nullptr), cancelable_task_manager_(new CancelableTaskManager()) { TRACE_ISOLATE(constructor); CheckIsolateLayout(); diff --git a/src/execution/isolate.h b/src/execution/isolate.h index 9300970de1..7353cfa854 100644 --- a/src/execution/isolate.h +++ b/src/execution/isolate.h @@ -134,7 +134,6 @@ class ReadOnlyArtifacts; class RegExpStack; class RootVisitor; class SetupIsolateDelegate; -class SharedObjectConveyors; class Simulator; class SnapshotData; class StringForwardingTable; @@ -2000,14 +1999,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { bool owns_shareable_data() { return owns_shareable_data_; } - SharedObjectConveyors* GetSharedObjectConveyors() const { - if (is_shared()) return shared_object_conveyors_.get(); - if (shared_isolate()) { - return shared_isolate()->shared_object_conveyors_.get(); - } - return nullptr; - } - bool log_object_relocation() const { return log_object_relocation_; } // TODO(pthier): Unify with owns_shareable_data() once the flag @@ -2400,12 +2391,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { // Otherwise this is populated for all Isolates. std::vector shared_heap_object_cache_; - // When sharing data among isolates, an isolate can send and receive shared - // objects with the ValueSerializer and ValueDeserializer. Shared objects that - // are in transit use PersistentHandles owned by this data structure to keep - // them alive. - std::unique_ptr shared_object_conveyors_; - // Used during builtins compilation to build the builtins constants table, // which is stored on the root list prior to serialization. BuiltinsConstantsTableBuilder* builtins_constants_table_builder_ = nullptr; diff --git a/src/handles/shared-object-conveyor-handles.cc b/src/handles/shared-object-conveyor-handles.cc new file mode 100644 index 0000000000..839ce212bd --- /dev/null +++ b/src/handles/shared-object-conveyor-handles.cc @@ -0,0 +1,25 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "src/handles/shared-object-conveyor-handles.h" + +#include "src/objects/objects-inl.h" + +namespace v8 { +namespace internal { + +// TODO(v8:12547): Currently the shared isolate owns all the conveyors. Change +// the owner to the main isolate once the shared isolate is removed. +SharedObjectConveyorHandles::SharedObjectConveyorHandles(Isolate* isolate) + : persistent_handles_(isolate->shared_isolate()->NewPersistentHandles()) {} + +uint32_t SharedObjectConveyorHandles::Persist(HeapObject shared_object) { + DCHECK(shared_object.IsShared()); + uint32_t id = static_cast(shared_objects_.size()); + shared_objects_.push_back(persistent_handles_->NewHandle(shared_object)); + return id; +} + +} // namespace internal +} // namespace v8 diff --git a/src/handles/shared-object-conveyor-handles.h b/src/handles/shared-object-conveyor-handles.h new file mode 100644 index 0000000000..f6b50de6c3 --- /dev/null +++ b/src/handles/shared-object-conveyor-handles.h @@ -0,0 +1,56 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef V8_HANDLES_SHARED_OBJECT_CONVEYOR_HANDLES_H_ +#define V8_HANDLES_SHARED_OBJECT_CONVEYOR_HANDLES_H_ + +#include +#include + +#include "src/handles/persistent-handles.h" + +namespace v8 { +namespace internal { + +class PersistentHandles; + +// Wrapper around PersistentHandles that is used to convey shared objects +// (i.e. keep them alive) from a ValueSerializer to a ValueDeserializer for APIs +// like postMessage. +// +// The conveyor must be allocated in an isolate that remains alive until the +// ValueDeserializer in the receiving isolate finishes processing the message. +// +// Each shared object gets an id that is stable across GCs. +// +// The embedder owns the lifetime of instances of this class. See +// v8::SharedValueConveyor. +class SharedObjectConveyorHandles { + public: + explicit SharedObjectConveyorHandles(Isolate* isolate); + + SharedObjectConveyorHandles(const SharedObjectConveyorHandles&) = delete; + SharedObjectConveyorHandles& operator=(const SharedObjectConveyorHandles&) = + delete; + + uint32_t Persist(HeapObject shared_object); + + bool HasPersisted(uint32_t object_id) const { + return object_id < shared_objects_.size(); + } + + HeapObject GetPersisted(uint32_t object_id) const { + DCHECK(HasPersisted(object_id)); + return *shared_objects_[object_id]; + } + + private: + std::unique_ptr persistent_handles_; + std::vector> shared_objects_; +}; + +} // namespace internal +} // namespace v8 + +#endif // V8_HANDLES_SHARED_OBJECT_CONVEYOR_HANDLES_H_ diff --git a/src/handles/shared-object-conveyors.cc b/src/handles/shared-object-conveyors.cc deleted file mode 100644 index 416236a431..0000000000 --- a/src/handles/shared-object-conveyors.cc +++ /dev/null @@ -1,78 +0,0 @@ -// Copyright 2022 the V8 project authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "src/handles/shared-object-conveyors.h" - -#include "src/objects/objects-inl.h" - -namespace v8 { -namespace internal { - -SharedObjectConveyorHandles::SharedObjectConveyorHandles(Isolate* isolate, - uint32_t id) - : id(id), persistent_handles_(isolate->NewPersistentHandles()) {} - -uint32_t SharedObjectConveyorHandles::Persist(HeapObject shared_object) { - DCHECK(shared_object.IsShared()); - uint32_t id = static_cast(shared_objects_.size()); - shared_objects_.push_back(persistent_handles_->NewHandle(shared_object)); - return id; -} - -HeapObject SharedObjectConveyorHandles::GetPersisted(uint32_t object_id) { - DCHECK(HasPersisted(object_id)); - return *shared_objects_[object_id]; -} - -void SharedObjectConveyorHandles::Delete() { - persistent_handles_->isolate()->GetSharedObjectConveyors()->DeleteConveyor( - id); -} - -SharedObjectConveyorHandles* SharedObjectConveyors::NewConveyor() { - base::MutexGuard guard(&conveyors_mutex_); - - uint32_t id; - if (conveyors_.empty()) { - id = 0; - } else { - size_t i; - for (i = 0; i < conveyors_.size(); i++) { - if (conveyors_[i] == nullptr) break; - } - id = static_cast(i); - } - - auto handles = std::make_unique(isolate_, id); - if (id < conveyors_.size()) { - conveyors_[id] = std::move(handles); - } else { - DCHECK_EQ(id, conveyors_.size()); - conveyors_.push_back(std::move(handles)); - } - - return conveyors_[id].get(); -} - -SharedObjectConveyorHandles* SharedObjectConveyors::MaybeGetConveyor( - uint32_t conveyor_id) { - base::MutexGuard guard(&conveyors_mutex_); - if (HasConveyor(conveyor_id)) return conveyors_[conveyor_id].get(); - return nullptr; -} - -void SharedObjectConveyors::DeleteConveyor(uint32_t conveyor_id) { - base::MutexGuard guard(&conveyors_mutex_); - DCHECK(HasConveyor(conveyor_id)); - conveyors_[conveyor_id].reset(nullptr); -} - -bool SharedObjectConveyors::HasConveyor(uint32_t conveyor_id) const { - return conveyor_id < conveyors_.size() && - conveyors_[conveyor_id] != nullptr && - conveyors_[conveyor_id]->id == conveyor_id; -} - -} // namespace internal -} // namespace v8 diff --git a/src/handles/shared-object-conveyors.h b/src/handles/shared-object-conveyors.h deleted file mode 100644 index 5b40075d07..0000000000 --- a/src/handles/shared-object-conveyors.h +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright 2022 the V8 project authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef V8_HANDLES_SHARED_OBJECT_CONVEYORS_H_ -#define V8_HANDLES_SHARED_OBJECT_CONVEYORS_H_ - -#include -#include - -#include "src/handles/persistent-handles.h" - -namespace v8 { -namespace internal { - -class PersistentHandles; - -// Wrapper around PersistentHandles that is used to convey shared objects -// (i.e. keep them alive) from a ValueSerializer to a ValueDeserializer for APIs -// like postMessage. -// -// The conveyor must be allocated in an isolate that remains alive until the -// ValueDeserializer in the receiving isolate finishes processing the message. -// -// Each conveyor has an id that is stable across GCs. Each shared object that is -// conveyed is gets an id pair (conveyor_id, object_id). Once all objects in a -// conveyor are received, the conveyor is deleted and its id may be reused for -// future conveyance. -// -// TODO(v8:12547): Currently the shared isolate owns all the conveyors. Change -// the owner to the main isolate once the shared isolate is removed. -class SharedObjectConveyorHandles { - public: - SharedObjectConveyorHandles(Isolate* isolate, uint32_t id); - - SharedObjectConveyorHandles(const SharedObjectConveyorHandles&) = delete; - SharedObjectConveyorHandles& operator=(const SharedObjectConveyorHandles&) = - delete; - - // Persist, GetPersisted, and HasPersisted are not threadsafe. A particular - // conveyor is used by a single thread at a time, either during sending a - // message or receiving a message. - uint32_t Persist(HeapObject shared_object); - HeapObject GetPersisted(uint32_t object_id); - bool HasPersisted(uint32_t object_id) const { - return object_id < shared_objects_.size(); - } - - // Deleting conveyors is threadsafe and may be called from multiple threads. - void Delete(); - - const uint32_t id; - - private: - std::unique_ptr persistent_handles_; - std::vector> shared_objects_; -}; - -// A class to own and manage conveyors. All methods are threadsafe and may be -// called from multiple threads. -class SharedObjectConveyors { - public: - explicit SharedObjectConveyors(Isolate* isolate) : isolate_(isolate) {} - - SharedObjectConveyorHandles* NewConveyor(); - SharedObjectConveyorHandles* MaybeGetConveyor(uint32_t conveyor_id); - - private: - friend class SharedObjectConveyorHandles; - - bool HasConveyor(uint32_t conveyor_id) const; - void DeleteConveyor(uint32_t conveyor_id); - - Isolate* isolate_; - base::Mutex conveyors_mutex_; - std::vector> conveyors_; -}; - -} // namespace internal -} // namespace v8 - -#endif // V8_HANDLES_SHARED_OBJECT_CONVEYORS_H_ diff --git a/src/objects/value-serializer.cc b/src/objects/value-serializer.cc index ba74cbd54a..faf802df7e 100644 --- a/src/objects/value-serializer.cc +++ b/src/objects/value-serializer.cc @@ -18,7 +18,7 @@ #include "src/handles/global-handles-inl.h" #include "src/handles/handles-inl.h" #include "src/handles/maybe-handles-inl.h" -#include "src/handles/shared-object-conveyors.h" +#include "src/handles/shared-object-conveyor-handles.h" #include "src/heap/factory.h" #include "src/numbers/conversions.h" #include "src/objects/heap-number-inl.h" @@ -171,8 +171,6 @@ enum class SerializationTag : uint8_t { kSharedArrayBuffer = 'u', // A HeapObject shared across Isolates. sharedValueID:uint32_t kSharedObject = 'p', - // The SharedObjectConveyor used to get shared objects. conveyorID:uint32_t - kSharedObjectConveyor = 'q', // A wasm module object transfer. next value is its index. kWasmModuleTransfer = 'w', // The delegate is responsible for processing all following data. @@ -264,7 +262,6 @@ ValueSerializer::ValueSerializer(Isolate* isolate, v8::ValueSerializer::Delegate* delegate) : isolate_(isolate), delegate_(delegate), - supports_shared_values_(delegate && delegate->SupportsSharedValues()), zone_(isolate->allocator(), ZONE_NAME), id_map_(isolate->heap(), ZoneAllocationPolicy(&zone_)), array_buffer_transfer_map_(isolate->heap(), @@ -472,11 +469,7 @@ Maybe ValueSerializer::WriteObject(Handle object) { } default: if (InstanceTypeChecker::IsString(instance_type)) { - auto string = Handle::cast(object); - if (v8_flags.shared_string_table && supports_shared_values_) { - return WriteSharedObject(String::Share(isolate_, string)); - } - WriteString(string); + WriteString(Handle::cast(object)); return ThrowIfOutOfMemory(); } else if (InstanceTypeChecker::IsJSReceiver(instance_type)) { return WriteJSReceiver(Handle::cast(object)); @@ -1103,21 +1096,26 @@ Maybe ValueSerializer::WriteWasmMemory(Handle object) { #endif // V8_ENABLE_WEBASSEMBLY Maybe ValueSerializer::WriteSharedObject(Handle object) { - if (!supports_shared_values_) { + if (!delegate_ || isolate_->shared_isolate() == nullptr) { return ThrowDataCloneError(MessageTemplate::kDataCloneError, object); } DCHECK(object->IsShared()); - // The first time a shared object is serialized, a new conveyor is made and - // its id is written. This conveyor is used for every shared object in this - // serialization and subsequent deserialization session. Once deserialization - // is complete, the conveyor is deleted. + // The first time a shared object is serialized, a new conveyor is made. This + // conveyor is used for every shared object in this serialization and + // subsequent deserialization sessions. The embedder owns the lifetime of the + // conveyor. if (!shared_object_conveyor_) { - shared_object_conveyor_ = - isolate_->GetSharedObjectConveyors()->NewConveyor(); - WriteTag(SerializationTag::kSharedObjectConveyor); - WriteVarint(shared_object_conveyor_->id); + v8::Isolate* v8_isolate = reinterpret_cast(isolate_); + v8::SharedValueConveyor v8_conveyor(v8_isolate); + shared_object_conveyor_ = v8_conveyor.private_.get(); + if (!delegate_->AdoptSharedValueConveyor(v8_isolate, + std::move(v8_conveyor))) { + shared_object_conveyor_ = nullptr; + RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, Nothing()); + return Nothing(); + } } WriteTag(SerializationTag::kSharedObject); @@ -1223,11 +1221,6 @@ ValueDeserializer::~ValueDeserializer() { if (array_buffer_transfer_map_.ToHandle(&transfer_map_handle)) { GlobalHandles::Destroy(transfer_map_handle.location()); } - - if (shared_object_conveyor_) { - shared_object_conveyor_->Delete(); - shared_object_conveyor_ = nullptr; - } } Maybe ValueDeserializer::ReadHeader() { @@ -1568,16 +1561,9 @@ MaybeHandle ValueDeserializer::ReadObjectInternal() { case SerializationTag::kHostObject: return ReadHostObject(); case SerializationTag::kSharedObject: - case SerializationTag::kSharedObjectConveyor: - if (version_ >= 15) { - if (tag == SerializationTag::kSharedObject) { - return ReadSharedObject(); - } - if (!ReadSharedObjectConveyor()) return MaybeHandle(); - return ReadObject(); - } - // If the delegate doesn't support shared values (e.g. older version, or - // is for deserializing from storage), treat the tag as unknown. + if (version_ >= 15) return ReadSharedObject(); + // If the data doesn't support shared values because it is from an older + // version, treat the tag as unknown. V8_FALLTHROUGH; default: // Before there was an explicit tag for host objects, all unknown tags @@ -2280,47 +2266,27 @@ MaybeHandle ValueDeserializer::ReadSharedObject() { return MaybeHandle(); } - // The conveyor must have already been gotten via the kSharedObjectConveyor - // tag. - if (shared_object_conveyor_ == nullptr || - !shared_object_conveyor_->HasPersisted(shared_object_id)) { + if (!delegate_) { ThrowDeserializationExceptionIfNonePending(isolate_); return MaybeHandle(); } + if (shared_object_conveyor_ == nullptr) { + const v8::SharedValueConveyor* conveyor = delegate_->GetSharedValueConveyor( + reinterpret_cast(isolate_)); + if (!conveyor) { + RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_, HeapObject); + return MaybeHandle(); + } + shared_object_conveyor_ = conveyor->private_.get(); + } + Handle shared_object( shared_object_conveyor_->GetPersisted(shared_object_id), isolate_); DCHECK(shared_object->IsShared()); return shared_object; } -bool ValueDeserializer::ReadSharedObjectConveyor() { - STACK_CHECK(isolate_, false); - DCHECK_GE(version_, 15); - - uint32_t conveyor_id; - if (!ReadVarint().To(&conveyor_id)) { - RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, false); - return false; - } - - // This tag appears at most once per deserialization data. - if (shared_object_conveyor_ != nullptr || - isolate_->GetSharedObjectConveyors() == nullptr) { - ThrowDeserializationExceptionIfNonePending(isolate_); - return false; - } - - shared_object_conveyor_ = - isolate_->GetSharedObjectConveyors()->MaybeGetConveyor(conveyor_id); - if (shared_object_conveyor_ == nullptr) { - ThrowDeserializationExceptionIfNonePending(isolate_); - return false; - } - - return true; -} - MaybeHandle ValueDeserializer::ReadHostObject() { if (!delegate_) return MaybeHandle(); STACK_CHECK(isolate_, MaybeHandle()); diff --git a/src/objects/value-serializer.h b/src/objects/value-serializer.h index 87f405c333..1c7e7bb76f 100644 --- a/src/objects/value-serializer.h +++ b/src/objects/value-serializer.h @@ -174,7 +174,6 @@ class ValueSerializer { uint8_t* buffer_ = nullptr; size_t buffer_size_ = 0; size_t buffer_capacity_ = 0; - const bool supports_shared_values_; bool treat_array_buffer_views_as_host_objects_ = false; bool out_of_memory_ = false; Zone zone_; @@ -313,7 +312,6 @@ class ValueDeserializer { MaybeHandle ReadWasmMemory() V8_WARN_UNUSED_RESULT; #endif // V8_ENABLE_WEBASSEMBLY MaybeHandle ReadSharedObject() V8_WARN_UNUSED_RESULT; - bool ReadSharedObjectConveyor() V8_WARN_UNUSED_RESULT; MaybeHandle ReadHostObject() V8_WARN_UNUSED_RESULT; /* @@ -343,7 +341,7 @@ class ValueDeserializer { MaybeHandle array_buffer_transfer_map_; // The conveyor used to keep shared objects alive. - SharedObjectConveyorHandles* shared_object_conveyor_ = nullptr; + const SharedObjectConveyorHandles* shared_object_conveyor_ = nullptr; }; } // namespace internal diff --git a/test/mjsunit/shared-memory/shared-string.js b/test/mjsunit/shared-memory/shared-string.js index da6fb39f43..1ecbaac358 100644 --- a/test/mjsunit/shared-memory/shared-string.js +++ b/test/mjsunit/shared-memory/shared-string.js @@ -2,34 +2,44 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // -// Flags: --shared-string-table --allow-natives-syntax +// Flags: --harmony-struct --shared-string-table --allow-natives-syntax if (this.Worker) { (function TestSharedStringPostMessage() { let workerScript = - `postMessage("started"); - onmessage = function(str) { - if (!%IsSharedString(str)) { + `let Box = new SharedStructType(['payload']); + let b1 = new Box(); + b1.payload = "started"; + postMessage(b1); + onmessage = function(box) { + if (!%IsSharedString(box.payload)) { throw new Error("str isn't shared"); } - postMessage(str); + let b2 = new Box(); + b2.payload = box.payload; + postMessage(b2); };`; + // Strings referenced in shared structs are serialized by sharing. + let worker = new Worker(workerScript, { type: 'string' }); let started = worker.getMessage(); - assertTrue(%IsSharedString(started)); - assertEquals("started", started); + assertTrue(%IsSharedString(started.payload)); + assertEquals("started", started.payload); // The string literal appears in source and is internalized, so should // already be shared. - let str_to_send = 'foo'; - assertTrue(%IsSharedString(str_to_send)); - worker.postMessage(str_to_send); - let str_received = worker.getMessage(); - assertTrue(%IsSharedString(str_received)); + let Box = new SharedStructType(['payload']); + let box_to_send = new Box(); + box_to_send.payload = 'foo'; + assertTrue(%IsSharedString(box_to_send.payload)); + worker.postMessage(box_to_send); + let box_received = worker.getMessage(); + assertTrue(%IsSharedString(box_received.payload)); + assertFalse(box_to_send === box_received); // Object.is and === won't check pointer equality of Strings. - assertTrue(%IsSameHeapObject(str_to_send, str_received)); + assertTrue(%IsSameHeapObject(box_to_send.payload, box_received.payload)); worker.terminate(); })(); diff --git a/test/mjsunit/shared-memory/shared-struct-workers.js b/test/mjsunit/shared-memory/shared-struct-workers.js index acb33e51b7..ddc78e5bbb 100644 --- a/test/mjsunit/shared-memory/shared-struct-workers.js +++ b/test/mjsunit/shared-memory/shared-struct-workers.js @@ -33,12 +33,6 @@ if (this.Worker) { assertEquals("worker", struct.string_field); assertEquals(42, struct.struct_field.payload); - // A serializer that doesn't support shared objects should throw. - assertThrows(() => { - worker.postMessage(struct, undefined, - { disableSharedValuesSupport: true }); - }); - worker.terminate(); })(); diff --git a/test/mjsunit/shared-memory/value-serializer.js b/test/mjsunit/shared-memory/value-serializer.js index 6646f6511d..39d9d419b8 100644 --- a/test/mjsunit/shared-memory/value-serializer.js +++ b/test/mjsunit/shared-memory/value-serializer.js @@ -6,19 +6,8 @@ // should throw instead of crash on non-existent shared objects in serialized // data. -(function Conveyor() { - // Conveyor is 'q', ASCII 113. - const data = new Uint8Array([255, 15, 113, 0]); - assertThrows(() => { d8.serializer.deserialize(data.buffer); }); -})(); - -(function SharedObjectNoConveyor() { +(function SharedObject() { // Shared object is 'p', ASCII 112. const data = new Uint8Array([255, 15, 112, 0]); assertThrows(() => { d8.serializer.deserialize(data.buffer); }); })(); - -(function SharedObjectAndConveyor() { - const data = new Uint8Array([255, 15, 113, 0, 112, 0]); - assertThrows(() => { d8.serializer.deserialize(data.buffer); }); -})();