From bd5b3ae5422e9fa1d0f7a281bbdf709e6db65f62 Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Mon, 21 Nov 2022 13:04:07 -0800 Subject: [PATCH] [shared-struct] Store length per-SharedArray instance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With shared space (instead of the shared isolate), the AccessorInfo implementation of SharedArray's length property is no longer threadsafe. Until AccessorInfos can be put into shared or RO space, go back to storing the length field as a per-instance in-object field, which is unfrotunately a little wasteful. Bug: v8:12547 Change-Id: I99c1cbf26047da48a4b4c11e14ab7def7d4e4f60 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4039309 Reviewed-by: Dominik Inführ Auto-Submit: Shu-yu Guo Commit-Queue: Shu-yu Guo Commit-Queue: Dominik Inführ Cr-Commit-Position: refs/heads/main@{#84408} --- src/builtins/accessors.cc | 19 --- src/builtins/accessors.h | 2 - src/heap/factory.cc | 4 + src/init/bootstrapper.cc | 25 ++-- src/objects/js-shared-array.h | 12 ++ .../shared-memory/shared-array-surface.js | 2 +- tools/v8heapconst.py | 111 +++++++++--------- 7 files changed, 83 insertions(+), 92 deletions(-) diff --git a/src/builtins/accessors.cc b/src/builtins/accessors.cc index 3c137aa115..7b1a1446fd 100644 --- a/src/builtins/accessors.cc +++ b/src/builtins/accessors.cc @@ -230,25 +230,6 @@ Handle Accessors::MakeArrayLengthInfo(Isolate* isolate) { &ArrayLengthGetter, &ArrayLengthSetter); } -// -// Accessors::SharedArrayLength -// - -void Accessors::SharedArrayLengthGetter( - v8::Local name, const v8::PropertyCallbackInfo& info) { - i::Isolate* isolate = reinterpret_cast(info.GetIsolate()); - DisallowGarbageCollection no_gc; - HandleScope scope(isolate); - auto holder = JSSharedArray::cast(*Utils::OpenHandle(*info.Holder())); - Object result = Smi::FromInt(holder.elements().length()); - info.GetReturnValue().Set(Utils::ToLocal(Handle(result, isolate))); -} - -Handle Accessors::MakeSharedArrayLengthInfo(Isolate* isolate) { - return MakeAccessor(isolate, isolate->factory()->length_string(), - &SharedArrayLengthGetter, nullptr); -} - // // Accessors::ModuleNamespaceEntry // diff --git a/src/builtins/accessors.h b/src/builtins/accessors.h index 8a8ea66b1f..f2edcd8978 100644 --- a/src/builtins/accessors.h +++ b/src/builtins/accessors.h @@ -44,8 +44,6 @@ class JavaScriptFrame; kHasSideEffectToReceiver) \ V(_, function_prototype, FunctionPrototype, kHasNoSideEffect, \ kHasSideEffectToReceiver) \ - V(_, shared_array_length, SharedArrayLength, kHasNoSideEffect, \ - kHasSideEffectToReceiver) \ V(_, string_length, StringLength, kHasNoSideEffect, \ kHasSideEffectToReceiver) \ V(_, value_unavailable, ValueUnavailable, kHasNoSideEffect, \ diff --git a/src/heap/factory.cc b/src/heap/factory.cc index 75f96759e6..1973b3adf1 100644 --- a/src/heap/factory.cc +++ b/src/heap/factory.cc @@ -3991,6 +3991,10 @@ Handle Factory::NewJSSharedArray(Handle constructor, Handle instance = Handle::cast( NewJSObject(constructor, AllocationType::kSharedOld)); instance->set_elements(*storage); + FieldIndex index = FieldIndex::ForDescriptor( + constructor->initial_map(), + InternalIndex(JSSharedArray::kLengthFieldIndex)); + instance->FastPropertyAtPut(index, Smi::FromInt(length), SKIP_WRITE_BARRIER); return instance; } diff --git a/src/init/bootstrapper.cc b/src/init/bootstrapper.cc index 053fdfcee4..7c7b917502 100644 --- a/src/init/bootstrapper.cc +++ b/src/init/bootstrapper.cc @@ -523,7 +523,7 @@ V8_NOINLINE Handle InstallFunction( V8_NOINLINE Handle CreateSharedObjectConstructor( Isolate* isolate, Handle name, InstanceType type, int instance_size, - ElementsKind element_kind, Builtin builtin) { + int inobject_properties, ElementsKind element_kind, Builtin builtin) { Factory* factory = isolate->factory(); Handle info = factory->NewSharedFunctionInfoForBuiltin( name, builtin, FunctionKind::kNormalFunction); @@ -532,9 +532,8 @@ V8_NOINLINE Handle CreateSharedObjectConstructor( Factory::JSFunctionBuilder{isolate, info, isolate->native_context()} .set_map(isolate->strict_function_with_readonly_prototype_map()) .Build(); - constexpr int in_object_properties = 0; Handle instance_map = - factory->NewMap(type, instance_size, element_kind, in_object_properties, + factory->NewMap(type, instance_size, element_kind, inobject_properties, AllocationType::kSharedMap); // Shared objects have fixed layout ahead of time, so there's no slack. instance_map->SetInObjectUnusedPropertyFields(0); @@ -4686,8 +4685,8 @@ void Genesis::InitializeGlobal_harmony_struct() { Handle shared_array_str = isolate()->factory()->InternalizeUtf8String("SharedArray"); Handle shared_array_fun = CreateSharedObjectConstructor( - isolate(), shared_array_str, JS_SHARED_ARRAY_TYPE, - JSSharedArray::kHeaderSize, SHARED_ARRAY_ELEMENTS, + isolate(), shared_array_str, JS_SHARED_ARRAY_TYPE, JSSharedArray::kSize, + JSSharedArray::kInObjectFieldCount, SHARED_ARRAY_ELEMENTS, Builtin::kSharedArrayConstructor); shared_array_fun->shared().set_internal_formal_parameter_count( JSParameterCount(0)); @@ -4697,14 +4696,12 @@ void Genesis::InitializeGlobal_harmony_struct() { Handle descriptors = isolate()->factory()->NewDescriptorArray(1, 0, AllocationType::kSharedOld); - Descriptor descriptor = Descriptor::AccessorConstant( + Descriptor length_descriptor = Descriptor::DataField( isolate()->shared_heap_isolate()->factory()->length_string(), - isolate() - ->shared_heap_isolate() - ->factory() - ->shared_array_length_accessor(), - ALL_ATTRIBUTES_MASK); - descriptors->Set(InternalIndex(0), &descriptor); + JSSharedArray::kLengthFieldIndex, ALL_ATTRIBUTES_MASK, + PropertyConstness::kConst, Representation::Smi(), + MaybeObjectHandle(FieldType::Any(isolate()))); + descriptors->Set(InternalIndex(0), &length_descriptor); shared_array_fun->initial_map().InitializeDescriptors(isolate(), *descriptors); @@ -4721,7 +4718,7 @@ void Genesis::InitializeGlobal_harmony_struct() { isolate()->factory()->InternalizeUtf8String("Mutex"); Handle mutex_fun = CreateSharedObjectConstructor( isolate(), mutex_str, JS_ATOMICS_MUTEX_TYPE, - JSAtomicsMutex::kHeaderSize, TERMINAL_FAST_ELEMENTS_KIND, + JSAtomicsMutex::kHeaderSize, 0, TERMINAL_FAST_ELEMENTS_KIND, Builtin::kAtomicsMutexConstructor); mutex_fun->shared().set_internal_formal_parameter_count( JSParameterCount(0)); @@ -4741,7 +4738,7 @@ void Genesis::InitializeGlobal_harmony_struct() { isolate()->factory()->InternalizeUtf8String("Condition"); Handle condition_fun = CreateSharedObjectConstructor( isolate(), condition_str, JS_ATOMICS_CONDITION_TYPE, - JSAtomicsCondition::kHeaderSize, TERMINAL_FAST_ELEMENTS_KIND, + JSAtomicsCondition::kHeaderSize, 0, TERMINAL_FAST_ELEMENTS_KIND, Builtin::kAtomicsConditionConstructor); condition_fun->shared().set_internal_formal_parameter_count( JSParameterCount(0)); diff --git a/src/objects/js-shared-array.h b/src/objects/js-shared-array.h index 2c98c45570..283aba9b28 100644 --- a/src/objects/js-shared-array.h +++ b/src/objects/js-shared-array.h @@ -22,6 +22,18 @@ class JSSharedArray DECL_PRINTER(JSSharedArray) EXPORT_DECL_VERIFIER(JSSharedArray) + // In-object fields. + enum { + // The length field is constant and is equal to elements().length(). + // + // TODO(v8:12547): We can save the space for this field by making it + // possible to put AccessorInfo in shared or RO space. + kLengthFieldIndex = 0, + kInObjectFieldCount, + }; + static constexpr int kSize = + kHeaderSize + (kTaggedSize * kInObjectFieldCount); + class BodyDescriptor; TQ_OBJECT_CONSTRUCTORS(JSSharedArray) diff --git a/test/mjsunit/shared-memory/shared-array-surface.js b/test/mjsunit/shared-memory/shared-array-surface.js index e0df38a029..01f938f4f4 100644 --- a/test/mjsunit/shared-memory/shared-array-surface.js +++ b/test/mjsunit/shared-memory/shared-array-surface.js @@ -88,7 +88,7 @@ let shared_array = new SharedArray(2); shared_array[0] = 42; - assertArrayEquals(shared_array.length, 10); + assertEquals(2, shared_array.length); let propDescs = Object.getOwnPropertyDescriptors(shared_array); let desc = propDescs[0]; diff --git a/tools/v8heapconst.py b/tools/v8heapconst.py index e420fd4d7b..205910457c 100644 --- a/tools/v8heapconst.py +++ b/tools/v8heapconst.py @@ -463,8 +463,8 @@ KNOWN_MAPS = { ("read_only_space", 0x080dd): (138, "StoreHandler1Map"), ("read_only_space", 0x08105): (138, "StoreHandler2Map"), ("read_only_space", 0x0812d): (138, "StoreHandler3Map"), - ("old_space", 0x043a5): (2116, "ExternalMap"), - ("old_space", 0x043d5): (2120, "JSMessageObjectMap"), + ("old_space", 0x0438d): (2116, "ExternalMap"), + ("old_space", 0x043bd): (2120, "JSMessageObjectMap"), } # List of known V8 objects. @@ -524,60 +524,59 @@ KNOWN_OBJECTS = { ("old_space", 0x042e5): "FunctionNameAccessor", ("old_space", 0x042fd): "FunctionLengthAccessor", ("old_space", 0x04315): "FunctionPrototypeAccessor", - ("old_space", 0x0432d): "SharedArrayLengthAccessor", - ("old_space", 0x04345): "StringLengthAccessor", - ("old_space", 0x0435d): "ValueUnavailableAccessor", - ("old_space", 0x04375): "WrappedFunctionLengthAccessor", - ("old_space", 0x0438d): "WrappedFunctionNameAccessor", - ("old_space", 0x043a5): "ExternalMap", - ("old_space", 0x043cd): "InvalidPrototypeValidityCell", - ("old_space", 0x043d5): "JSMessageObjectMap", - ("old_space", 0x043fd): "EmptyScript", - ("old_space", 0x04441): "ManyClosuresCell", - ("old_space", 0x0444d): "ArrayConstructorProtector", - ("old_space", 0x04461): "NoElementsProtector", - ("old_space", 0x04475): "MegaDOMProtector", - ("old_space", 0x04489): "IsConcatSpreadableProtector", - ("old_space", 0x0449d): "ArraySpeciesProtector", - ("old_space", 0x044b1): "TypedArraySpeciesProtector", - ("old_space", 0x044c5): "PromiseSpeciesProtector", - ("old_space", 0x044d9): "RegExpSpeciesProtector", - ("old_space", 0x044ed): "StringLengthProtector", - ("old_space", 0x04501): "ArrayIteratorProtector", - ("old_space", 0x04515): "ArrayBufferDetachingProtector", - ("old_space", 0x04529): "PromiseHookProtector", - ("old_space", 0x0453d): "PromiseResolveProtector", - ("old_space", 0x04551): "MapIteratorProtector", - ("old_space", 0x04565): "PromiseThenProtector", - ("old_space", 0x04579): "SetIteratorProtector", - ("old_space", 0x0458d): "StringIteratorProtector", - ("old_space", 0x045a1): "StringSplitCache", - ("old_space", 0x049a9): "RegExpMultipleCache", - ("old_space", 0x04db1): "BuiltinsConstantsTable", - ("old_space", 0x05215): "AsyncFunctionAwaitRejectSharedFun", - ("old_space", 0x05239): "AsyncFunctionAwaitResolveSharedFun", - ("old_space", 0x0525d): "AsyncGeneratorAwaitRejectSharedFun", - ("old_space", 0x05281): "AsyncGeneratorAwaitResolveSharedFun", - ("old_space", 0x052a5): "AsyncGeneratorYieldWithAwaitResolveSharedFun", - ("old_space", 0x052c9): "AsyncGeneratorReturnResolveSharedFun", - ("old_space", 0x052ed): "AsyncGeneratorReturnClosedRejectSharedFun", - ("old_space", 0x05311): "AsyncGeneratorReturnClosedResolveSharedFun", - ("old_space", 0x05335): "AsyncIteratorValueUnwrapSharedFun", - ("old_space", 0x05359): "PromiseAllResolveElementSharedFun", - ("old_space", 0x0537d): "PromiseAllSettledResolveElementSharedFun", - ("old_space", 0x053a1): "PromiseAllSettledRejectElementSharedFun", - ("old_space", 0x053c5): "PromiseAnyRejectElementSharedFun", - ("old_space", 0x053e9): "PromiseCapabilityDefaultRejectSharedFun", - ("old_space", 0x0540d): "PromiseCapabilityDefaultResolveSharedFun", - ("old_space", 0x05431): "PromiseCatchFinallySharedFun", - ("old_space", 0x05455): "PromiseGetCapabilitiesExecutorSharedFun", - ("old_space", 0x05479): "PromiseThenFinallySharedFun", - ("old_space", 0x0549d): "PromiseThrowerFinallySharedFun", - ("old_space", 0x054c1): "PromiseValueThunkFinallySharedFun", - ("old_space", 0x054e5): "ProxyRevokeSharedFun", - ("old_space", 0x05509): "ShadowRealmImportValueFulfilledSFI", - ("old_space", 0x0552d): "SourceTextModuleExecuteAsyncModuleFulfilledSFI", - ("old_space", 0x05551): "SourceTextModuleExecuteAsyncModuleRejectedSFI", + ("old_space", 0x0432d): "StringLengthAccessor", + ("old_space", 0x04345): "ValueUnavailableAccessor", + ("old_space", 0x0435d): "WrappedFunctionLengthAccessor", + ("old_space", 0x04375): "WrappedFunctionNameAccessor", + ("old_space", 0x0438d): "ExternalMap", + ("old_space", 0x043b5): "InvalidPrototypeValidityCell", + ("old_space", 0x043bd): "JSMessageObjectMap", + ("old_space", 0x043e5): "EmptyScript", + ("old_space", 0x04429): "ManyClosuresCell", + ("old_space", 0x04435): "ArrayConstructorProtector", + ("old_space", 0x04449): "NoElementsProtector", + ("old_space", 0x0445d): "MegaDOMProtector", + ("old_space", 0x04471): "IsConcatSpreadableProtector", + ("old_space", 0x04485): "ArraySpeciesProtector", + ("old_space", 0x04499): "TypedArraySpeciesProtector", + ("old_space", 0x044ad): "PromiseSpeciesProtector", + ("old_space", 0x044c1): "RegExpSpeciesProtector", + ("old_space", 0x044d5): "StringLengthProtector", + ("old_space", 0x044e9): "ArrayIteratorProtector", + ("old_space", 0x044fd): "ArrayBufferDetachingProtector", + ("old_space", 0x04511): "PromiseHookProtector", + ("old_space", 0x04525): "PromiseResolveProtector", + ("old_space", 0x04539): "MapIteratorProtector", + ("old_space", 0x0454d): "PromiseThenProtector", + ("old_space", 0x04561): "SetIteratorProtector", + ("old_space", 0x04575): "StringIteratorProtector", + ("old_space", 0x04589): "StringSplitCache", + ("old_space", 0x04991): "RegExpMultipleCache", + ("old_space", 0x04d99): "BuiltinsConstantsTable", + ("old_space", 0x051fd): "AsyncFunctionAwaitRejectSharedFun", + ("old_space", 0x05221): "AsyncFunctionAwaitResolveSharedFun", + ("old_space", 0x05245): "AsyncGeneratorAwaitRejectSharedFun", + ("old_space", 0x05269): "AsyncGeneratorAwaitResolveSharedFun", + ("old_space", 0x0528d): "AsyncGeneratorYieldWithAwaitResolveSharedFun", + ("old_space", 0x052b1): "AsyncGeneratorReturnResolveSharedFun", + ("old_space", 0x052d5): "AsyncGeneratorReturnClosedRejectSharedFun", + ("old_space", 0x052f9): "AsyncGeneratorReturnClosedResolveSharedFun", + ("old_space", 0x0531d): "AsyncIteratorValueUnwrapSharedFun", + ("old_space", 0x05341): "PromiseAllResolveElementSharedFun", + ("old_space", 0x05365): "PromiseAllSettledResolveElementSharedFun", + ("old_space", 0x05389): "PromiseAllSettledRejectElementSharedFun", + ("old_space", 0x053ad): "PromiseAnyRejectElementSharedFun", + ("old_space", 0x053d1): "PromiseCapabilityDefaultRejectSharedFun", + ("old_space", 0x053f5): "PromiseCapabilityDefaultResolveSharedFun", + ("old_space", 0x05419): "PromiseCatchFinallySharedFun", + ("old_space", 0x0543d): "PromiseGetCapabilitiesExecutorSharedFun", + ("old_space", 0x05461): "PromiseThenFinallySharedFun", + ("old_space", 0x05485): "PromiseThrowerFinallySharedFun", + ("old_space", 0x054a9): "PromiseValueThunkFinallySharedFun", + ("old_space", 0x054cd): "ProxyRevokeSharedFun", + ("old_space", 0x054f1): "ShadowRealmImportValueFulfilledSFI", + ("old_space", 0x05515): "SourceTextModuleExecuteAsyncModuleFulfilledSFI", + ("old_space", 0x05539): "SourceTextModuleExecuteAsyncModuleRejectedSFI", } # Lower 32 bits of first page addresses for various heap spaces.