From 9bfd401ef5f49d23b5a2f83bbc2f0ae35b6cb9f9 Mon Sep 17 00:00:00 2001 From: Jakob Gruber Date: Wed, 9 Jun 2021 08:44:18 +0200 Subject: [PATCH] [compiler] RawFastPropertyAt without serialization This is a step towards making JSObjectRef non-serialized. Change JSObjectRef::RawFastPropertyAt to use a direct load with relaxed semantics. Special handling of `uninitialized` sentinel values is moved to the only use-site. A new lock `boilerplate_migration_access` protects against concurrent boilerplate migrations while we are iterating over properties. Bug: v8:7790 Change-Id: Ic9de54ca16c1f3364d497a77058cfa33d48dd4a4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2928184 Commit-Queue: Jakob Gruber Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#75033} --- src/compiler/access-info.cc | 3 +- src/compiler/heap-refs.cc | 32 ++++++++-------- src/compiler/heap-refs.h | 11 +++++- src/compiler/js-create-lowering.cc | 59 +++++++++++++++++++++++++++--- src/compiler/js-heap-broker.h | 54 +++++++++++++++++---------- src/execution/isolate.h | 14 +++++++ src/objects/js-objects-inl.h | 6 +++ src/objects/js-objects.h | 3 ++ src/runtime/runtime-literals.cc | 2 + 9 files changed, 139 insertions(+), 45 deletions(-) diff --git a/src/compiler/access-info.cc b/src/compiler/access-info.cc index 0e3ccc1948..8e366bbfa2 100644 --- a/src/compiler/access-info.cc +++ b/src/compiler/access-info.cc @@ -719,8 +719,7 @@ PropertyAccessInfo AccessInfoFactory::ComputePropertyAccessInfo( Handle map, Handle name, AccessMode access_mode) const { CHECK(name->IsUniqueName()); - JSHeapBroker::MapUpdaterGuardIfNeeded mumd_scope( - broker(), isolate()->map_updater_access()); + JSHeapBroker::MapUpdaterGuardIfNeeded mumd_scope(broker()); if (access_mode == AccessMode::kHas && !map->IsJSReceiverMap()) { return Invalid(); diff --git a/src/compiler/heap-refs.cc b/src/compiler/heap-refs.cc index 3dde6f4a46..dbbcf5ee4e 100644 --- a/src/compiler/heap-refs.cc +++ b/src/compiler/heap-refs.cc @@ -802,12 +802,15 @@ class HeapNumberData : public HeapObjectData { Handle object, ObjectDataKind kind = ObjectDataKind::kSerializedHeapObject) : HeapObjectData(broker, storage, object, kind), - value_(object->value()) {} + value_(object->value()), + value_as_bits_(object->value_as_bits()) {} double value() const { return value_; } + uint64_t value_as_bits() const { return value_as_bits_; } private: double const value_; + uint64_t const value_as_bits_; }; class ContextData : public HeapObjectData { @@ -1412,8 +1415,7 @@ MapData::MapData(JSHeapBroker* broker, ObjectData** storage, Handle object, // while the lock is held the Map object may not be modified (except in // benign ways). // TODO(jgruber): Consider removing this lock by being smrt. - JSHeapBroker::MapUpdaterGuardIfNeeded mumd_scope( - broker, broker->isolate()->map_updater_access()); + JSHeapBroker::MapUpdaterGuardIfNeeded mumd_scope(broker); // When background serializing the map, we can perform a lite serialization // since the MapRef will read some of the Map's fields can be read directly. @@ -2466,17 +2468,6 @@ void JSObjectData::SerializeRecursiveAsBoilerplate(JSHeapBroker* broker, DCHECK_EQ(field_index.property_index(), static_cast(inobject_fields_.size())); Handle value(boilerplate->RawFastPropertyAt(field_index), isolate); - // In case of double fields we use a sentinel NaN value to mark - // uninitialized fields. A boilerplate value with such a field may migrate - // from its double to a tagged representation. The sentinel value carries - // no special meaning when it occurs in a heap number, so we would like to - // recover the uninitialized value. We check for the sentinel here, - // specifically, since migrations might have been triggered as part of - // boilerplate serialization. - if (!details.representation().IsDouble() && value->IsHeapNumber() && - HeapNumber::cast(*value).value_as_bits() == kHoleNanInt64) { - value = isolate->factory()->uninitialized_value(); - } ObjectData* value_data = broker->GetOrCreateData(value); if (value_data->IsJSObject() && !value_data->should_access_heap()) { value_data->AsJSObject()->SerializeRecursiveAsBoilerplate(broker, @@ -3026,10 +3017,12 @@ FeedbackCellRef FeedbackVectorRef::GetClosureFeedbackCell(int index) const { data()->AsFeedbackVector()->GetClosureFeedbackCell(broker(), index)); } -ObjectRef JSObjectRef::RawFastPropertyAt(FieldIndex index) const { +base::Optional JSObjectRef::RawFastPropertyAt( + FieldIndex index) const { CHECK(index.is_inobject()); - if (data_->should_access_heap()) { - return MakeRef(broker(), object()->RawFastPropertyAt(index)); + if (data_->should_access_heap() || broker()->is_concurrent_inlining()) { + return TryMakeRef(broker(), + object()->RawFastPropertyAt(index, kRelaxedLoad)); } JSObjectData* object_data = data()->AsJSObject(); return ObjectRef(broker(), @@ -3306,6 +3299,7 @@ BIMODAL_ACCESSOR_C(FeedbackVector, double, invocation_count) BIMODAL_ACCESSOR(HeapObject, Map, map) BIMODAL_ACCESSOR_C(HeapNumber, double, value) +BIMODAL_ACCESSOR_C(HeapNumber, uint64_t, value_as_bits) // These JSBoundFunction fields are immutable after initialization. Moreover, // as long as JSObjects are still serialized on the main thread, all @@ -4053,6 +4047,10 @@ base::Optional SourceTextModuleRef::import_meta() const { data()->AsSourceTextModule()->GetImportMeta(broker())); } +base::Optional HeapObjectRef::map_direct_read() const { + return TryMakeRef(broker(), object()->map(kAcquireLoad), kAssumeMemoryFence); +} + namespace { OddballType GetOddballType(Isolate* isolate, Map map) { diff --git a/src/compiler/heap-refs.h b/src/compiler/heap-refs.h index f11aa34fff..6f6f44ef72 100644 --- a/src/compiler/heap-refs.h +++ b/src/compiler/heap-refs.h @@ -279,6 +279,10 @@ class HeapObjectRef : public ObjectRef { MapRef map() const; + // Only for use in special situations where we need to read the object's + // current map (instead of returning the cached map). Use with care. + base::Optional map_direct_read() const; + // See the comment on the HeapObjectType class. HeapObjectType GetHeapObjectType() const; }; @@ -315,7 +319,11 @@ class JSObjectRef : public JSReceiverRef { Handle object() const; - ObjectRef RawFastPropertyAt(FieldIndex index) const; + // Usable only for in-object properties. Only use this if the underlying + // value can be an uninitialized-sentinel, or if HeapNumber construction must + // be avoided for some reason. Otherwise, use the higher-level + // GetOwnFastDataProperty. + base::Optional RawFastPropertyAt(FieldIndex index) const; // Return the element at key {index} if {index} is known to be an own data // property of the object that is non-writable and non-configurable. @@ -439,6 +447,7 @@ class HeapNumberRef : public HeapObjectRef { Handle object() const; double value() const; + uint64_t value_as_bits() const; }; class ContextRef : public HeapObjectRef { diff --git a/src/compiler/js-create-lowering.cc b/src/compiler/js-create-lowering.cc index 11ed0b81cd..f125ed524e 100644 --- a/src/compiler/js-create-lowering.cc +++ b/src/compiler/js-create-lowering.cc @@ -1656,8 +1656,21 @@ Node* JSCreateLowering::AllocateElements(Node* effect, Node* control, base::Optional JSCreateLowering::TryAllocateFastLiteral( Node* effect, Node* control, JSObjectRef boilerplate, AllocationType allocation) { - // Compute the in-object properties to store first (might have effects). + // Prevent concurrent migrations of boilerplate objects. + JSHeapBroker::BoilerplateMigrationGuardIfNeeded boilerplate_access_guard( + broker()); + + // Now that we hold the migration lock, get the current map. MapRef boilerplate_map = boilerplate.map(); + base::Optional current_boilerplate_map = + boilerplate.map_direct_read(); + + if (!current_boilerplate_map.has_value() || + !current_boilerplate_map.value().equals(boilerplate_map)) { + return {}; + } + + // Compute the in-object properties to store first (might have effects). ZoneVector> inobject_fields(zone()); inobject_fields.reserve(boilerplate_map.GetInObjectProperties()); int const boilerplate_nof = boilerplate_map.NumberOfOwnDescriptors(); @@ -1678,14 +1691,48 @@ base::Optional JSCreateLowering::TryAllocateFastLiteral( kFullWriteBarrier, LoadSensitivity::kUnsafe, const_field_info}; - ObjectRef boilerplate_value = boilerplate.RawFastPropertyAt(index); - bool is_uninitialized = - boilerplate_value.IsHeapObject() && + + // Note: the use of RawFastPropertyAt (vs. the higher-level + // GetOwnFastDataProperty) here is necessary, since the underlying value + // may be `uninitialized`, which the latter explicitly does not support. + base::Optional maybe_boilerplate_value = + boilerplate.RawFastPropertyAt(index); + if (!maybe_boilerplate_value.has_value()) return {}; + + // Note: We don't need to take a compilation dependency verifying the value + // of `boilerplate_value`, since boilerplate properties are constant after + // initialization modulo map migration. We protect against concurrent map + // migrations via the boilerplate_migration_access lock. + ObjectRef boilerplate_value = maybe_boilerplate_value.value(); + + // Uninitialized fields are marked through the `uninitialized_value`, or in + // the case of double representation through a HeapNumber containing a + // special sentinel value. The boilerplate value is not updated to keep up + // with the map, thus conversions may be necessary. Specifically: + // + // - Smi representation: uninitialized is converted to Smi 0. + // - Not double representation: the special heap number sentinel is + // converted to `uninitialized_value`. + // + // Note that although we create nodes to write `uninitialized_value` into + // the object, the field should be overwritten immediately with a real + // value, and `uninitialized_value` should never be exposed to JS. + bool is_uninitialized = false; + if (boilerplate_value.IsHeapObject() && boilerplate_value.AsHeapObject().map().oddball_type() == - OddballType::kUninitialized; - if (is_uninitialized) { + OddballType::kUninitialized) { + is_uninitialized = true; + access.const_field_info = ConstFieldInfo::None(); + } else if (!property_details.representation().IsDouble() && + boilerplate_value.IsHeapNumber() && + boilerplate_value.AsHeapNumber().value_as_bits() == + kHoleNanInt64) { + is_uninitialized = true; + boilerplate_value = + MakeRef(broker(), factory()->uninitialized_value()); access.const_field_info = ConstFieldInfo::None(); } + Node* value; if (boilerplate_value.IsJSObject()) { JSObjectRef boilerplate_object = boilerplate_value.AsJSObject(); diff --git a/src/compiler/js-heap-broker.h b/src/compiler/js-heap-broker.h index 59a97ab83a..e089b66d30 100644 --- a/src/compiler/js-heap-broker.h +++ b/src/compiler/js-heap-broker.h @@ -354,30 +354,44 @@ class V8_EXPORT_PRIVATE JSHeapBroker { // Locks {mutex} through the duration of this scope iff it is the first // occurrence. This is done to have a recursive shared lock on {mutex}. - class V8_NODISCARD MapUpdaterGuardIfNeeded final { - public: - explicit MapUpdaterGuardIfNeeded(JSHeapBroker* ptr, - base::SharedMutex* mutex) - : ptr_(ptr), - initial_map_updater_mutex_depth_(ptr->map_updater_mutex_depth_), - shared_mutex(mutex, should_lock()) { - ptr_->map_updater_mutex_depth_++; + class V8_NODISCARD RecursiveSharedMutexGuardIfNeeded { + protected: + RecursiveSharedMutexGuardIfNeeded(base::SharedMutex* mutex, + int* mutex_depth_address) + : mutex_depth_address_(mutex_depth_address), + initial_mutex_depth_(*mutex_depth_address_), + shared_mutex_guard_(mutex, initial_mutex_depth_ == 0) { + (*mutex_depth_address_)++; } - ~MapUpdaterGuardIfNeeded() { - ptr_->map_updater_mutex_depth_--; - DCHECK_EQ(initial_map_updater_mutex_depth_, - ptr_->map_updater_mutex_depth_); + ~RecursiveSharedMutexGuardIfNeeded() { + DCHECK_GE((*mutex_depth_address_), 1); + (*mutex_depth_address_)--; + DCHECK_EQ(initial_mutex_depth_, (*mutex_depth_address_)); } - // Whether the MapUpdater mutex should be physically locked (if not, we - // already hold the lock). - bool should_lock() const { return initial_map_updater_mutex_depth_ == 0; } - private: - JSHeapBroker* const ptr_; - const int initial_map_updater_mutex_depth_; - base::SharedMutexGuardIf shared_mutex; + int* const mutex_depth_address_; + const int initial_mutex_depth_; + base::SharedMutexGuardIf shared_mutex_guard_; + }; + + class MapUpdaterGuardIfNeeded final + : public RecursiveSharedMutexGuardIfNeeded { + public: + explicit MapUpdaterGuardIfNeeded(JSHeapBroker* broker) + : RecursiveSharedMutexGuardIfNeeded( + broker->isolate()->map_updater_access(), + &broker->map_updater_mutex_depth_) {} + }; + + class BoilerplateMigrationGuardIfNeeded final + : public RecursiveSharedMutexGuardIfNeeded { + public: + explicit BoilerplateMigrationGuardIfNeeded(JSHeapBroker* broker) + : RecursiveSharedMutexGuardIfNeeded( + broker->isolate()->boilerplate_migration_access(), + &broker->boilerplate_migration_mutex_depth_) {} }; private: @@ -502,6 +516,8 @@ class V8_EXPORT_PRIVATE JSHeapBroker { // holds the locking depth, i.e. how many times the mutex has been // recursively locked. Only the outermost locker actually locks underneath. int map_updater_mutex_depth_ = 0; + // Likewise for boilerplate migrations. + int boilerplate_migration_mutex_depth_ = 0; static constexpr size_t kMaxSerializedFunctionsCacheSize = 200; static constexpr uint32_t kMinimalRefsBucketCount = 8; diff --git a/src/execution/isolate.h b/src/execution/isolate.h index b3d887620b..1b34f6f89d 100644 --- a/src/execution/isolate.h +++ b/src/execution/isolate.h @@ -672,8 +672,21 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { return &shared_function_info_access_; } + // Protects (most) map update operations, see also MapUpdater. base::SharedMutex* map_updater_access() { return &map_updater_access_; } + // Protects JSObject boilerplate migrations (i.e. calls to MigrateInstance on + // boilerplate objects; elements kind transitions are *not* protected). + // Note this lock interacts with `map_updater_access` as follows + // + // - boilerplate migrations may trigger map updates. + // - if so, `boilerplate_migration_access` is locked before + // `map_updater_access`. + // - backgrounds threads must use the same lock order to avoid deadlocks. + base::SharedMutex* boilerplate_migration_access() { + return &boilerplate_migration_access_; + } + // The isolate's string table. StringTable* string_table() const { return string_table_.get(); } @@ -1936,6 +1949,7 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { base::SharedMutex full_transition_array_access_; base::SharedMutex shared_function_info_access_; base::SharedMutex map_updater_access_; + base::SharedMutex boilerplate_migration_access_; Logger* logger_ = nullptr; StubCache* load_stub_cache_ = nullptr; StubCache* store_stub_cache_ = nullptr; diff --git a/src/objects/js-objects-inl.h b/src/objects/js-objects-inl.h index 34c96b3cfb..8fd3de8650 100644 --- a/src/objects/js-objects-inl.h +++ b/src/objects/js-objects-inl.h @@ -342,6 +342,12 @@ Object JSObject::RawFastPropertyAt(PtrComprCageBase cage_base, } } +Object JSObject::RawFastPropertyAt(FieldIndex index, RelaxedLoadTag tag) const { + PtrComprCageBase cage_base = GetPtrComprCageBase(*this); + CHECK(index.is_inobject()); + return TaggedField::Relaxed_Load(cage_base, *this, index.offset()); +} + void JSObject::RawFastInobjectPropertyAtPut(FieldIndex index, Object value, WriteBarrierMode mode) { DCHECK(index.is_inobject()); diff --git a/src/objects/js-objects.h b/src/objects/js-objects.h index 7ef1ce4218..27bcd4a18b 100644 --- a/src/objects/js-objects.h +++ b/src/objects/js-objects.h @@ -651,6 +651,9 @@ class JSObject : public TorqueGeneratedJSObject { inline Object RawFastPropertyAt(FieldIndex index) const; inline Object RawFastPropertyAt(PtrComprCageBase cage_base, FieldIndex index) const; + // A specialized version of the above for use by TurboFan. Only supports + // in-object properties. + inline Object RawFastPropertyAt(FieldIndex index, RelaxedLoadTag) const; inline void FastPropertyAtPut(FieldIndex index, Object value, WriteBarrierMode mode = UPDATE_WRITE_BARRIER); diff --git a/src/runtime/runtime-literals.cc b/src/runtime/runtime-literals.cc index c4285f2403..8f8b6c6fa3 100644 --- a/src/runtime/runtime-literals.cc +++ b/src/runtime/runtime-literals.cc @@ -84,6 +84,8 @@ MaybeHandle JSObjectWalkVisitor::StructureWalk( } if (object->map(isolate).is_deprecated()) { + base::SharedMutexGuard mutex_guard( + isolate->boilerplate_migration_access()); JSObject::MigrateInstance(isolate, object); }