Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: cherry-pick 389ea9be7d68 from v8 #40970

Merged
merged 2 commits into from
Jan 12, 2024
Merged

chore: cherry-pick 389ea9be7d68 from v8 #40970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffee277
chore: cherry-pick 389ea9be7d68 from v8
MarshallOfSound Jan 11, 2024
96290c2
chore: update patches
patchup[bot] Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/v8/.patches
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ build_gn.patch
do_not_export_private_v8_symbols_on_windows.patch
fix_build_deprecated_attribute_for_older_msvc_versions.patch
chore_allow_customizing_microtask_policy_per_context.patch
cherry-pick-389ea9be7d68.patch
335 changes: 335 additions & 0 deletions patches/v8/cherry-pick-389ea9be7d68.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,335 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Toon Verwaest <verwaest@chromium.org>
Date: Thu, 11 Jan 2024 10:47:17 +0100
Subject: Drop fast last-property deletion

This interacts badly with other optimizations and isn't particularly
common.

Bug: chromium:1517354
Change-Id: I7adb51a8fc0ec47eaeb911ca2a4cbc517088e416
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5185340
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Auto-Submit: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/main@{#91782}

diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc
index 94af9625f372c09b6cf10aead05f71832c274a6b..87b20def865db3cd1f50e35c70f8aedf226bdede 100644
--- a/src/runtime/runtime-object.cc
+++ b/src/runtime/runtime-object.cc
@@ -71,186 +71,10 @@ MaybeHandle<Object> Runtime::HasProperty(Isolate* isolate,
return ReadOnlyRoots(isolate).boolean_value_handle(maybe.FromJust());
}

-namespace {
-
-// This function sets the sentinel value in a deleted field. Thes sentinel has
-// to look like a proper standalone object because the slack tracking may
-// complete at any time. For this reason we use the filler map word.
-// If V8_MAP_PACKING is enabled, then the filler map word is a packed filler
-// map. Otherwise, the filler map word is the same as the filler map.
-inline void ClearField(Isolate* isolate, Tagged<JSObject> object,
- FieldIndex index) {
- if (index.is_inobject()) {
- MapWord filler_map_word =
- ReadOnlyRoots(isolate).one_pointer_filler_map_word();
-#ifndef V8_MAP_PACKING
- DCHECK_EQ(filler_map_word.ToMap(),
- ReadOnlyRoots(isolate).one_pointer_filler_map());
-#endif
- int offset = index.offset();
- TaggedField<MapWord>::Release_Store(object, offset, filler_map_word);
- } else {
- object->property_array()->set(
- index.outobject_array_index(),
- ReadOnlyRoots(isolate).one_pointer_filler_map());
- }
-}
-
-void GeneralizeAllTransitionsToFieldAsMutable(Isolate* isolate, Handle<Map> map,
- Handle<Name> name) {
- InternalIndex descriptor(map->NumberOfOwnDescriptors());
-
- Handle<Map> target_maps[kPropertyAttributesCombinationsCount];
- int target_maps_count = 0;
-
- // Collect all outgoing field transitions.
- {
- DisallowGarbageCollection no_gc;
- TransitionsAccessor transitions(isolate, *map);
- transitions.ForEachTransitionTo(
- *name,
- [&](Tagged<Map> target) {
- DCHECK_EQ(descriptor, target->LastAdded());
- DCHECK_EQ(*name, target->GetLastDescriptorName(isolate));
- PropertyDetails details = target->GetLastDescriptorDetails(isolate);
- // Currently, we track constness only for fields.
- if (details.kind() == PropertyKind::kData &&
- details.constness() == PropertyConstness::kConst) {
- target_maps[target_maps_count++] = handle(target, isolate);
- }
- DCHECK_IMPLIES(details.kind() == PropertyKind::kAccessor,
- details.constness() == PropertyConstness::kConst);
- },
- &no_gc);
- CHECK_LE(target_maps_count, kPropertyAttributesCombinationsCount);
- }
-
- for (int i = 0; i < target_maps_count; i++) {
- Handle<Map> target = target_maps[i];
- PropertyDetails details =
- target->instance_descriptors(isolate)->GetDetails(descriptor);
- Handle<FieldType> field_type(
- target->instance_descriptors(isolate)->GetFieldType(descriptor),
- isolate);
- MapUpdater::GeneralizeField(isolate, target, descriptor,
- PropertyConstness::kMutable,
- details.representation(), field_type);
- DCHECK_EQ(PropertyConstness::kMutable, target->instance_descriptors(isolate)
- ->GetDetails(descriptor)
- .constness());
- }
-}
-
-bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
- Handle<Object> raw_key) {
- // This implements a special case for fast property deletion: when the
- // last property in an object is deleted, then instead of normalizing
- // the properties, we can undo the last map transition, with a few
- // prerequisites:
- // (1) The receiver must be a regular object and the key a unique name.
- Handle<Map> receiver_map(receiver->map(), isolate);
- if (IsSpecialReceiverMap(*receiver_map)) return false;
- DCHECK(IsJSObjectMap(*receiver_map));
-
- if (!IsUniqueName(*raw_key)) return false;
- Handle<Name> key = Handle<Name>::cast(raw_key);
- // (2) The property to be deleted must be the last property.
- int nof = receiver_map->NumberOfOwnDescriptors();
- if (nof == 0) return false;
- InternalIndex descriptor(nof - 1);
- Handle<DescriptorArray> descriptors(
- receiver_map->instance_descriptors(isolate), isolate);
- if (descriptors->GetKey(descriptor) != *key) return false;
- // (3) The property to be deleted must be deletable.
- PropertyDetails details = descriptors->GetDetails(descriptor);
- if (!details.IsConfigurable()) return false;
- // (4) The map must have a back pointer.
- Handle<Object> backpointer(receiver_map->GetBackPointer(), isolate);
- if (!IsMap(*backpointer)) return false;
- Handle<Map> parent_map = Handle<Map>::cast(backpointer);
- // (5) The last transition must have been caused by adding a property
- // (and not any kind of special transition).
- if (parent_map->NumberOfOwnDescriptors() != nof - 1) return false;
-
- // Preconditions successful. No more bailouts after this point.
-
- // Zap the property to avoid keeping objects alive. Zapping is not necessary
- // for properties stored in the descriptor array.
- if (details.location() == PropertyLocation::kField) {
- DisallowGarbageCollection no_gc;
-
- // Invalidate slots manually later in case we delete an in-object tagged
- // property. In this case we might later store an untagged value in the
- // recorded slot.
- isolate->heap()->NotifyObjectLayoutChange(
- *receiver, no_gc, InvalidateRecordedSlots::kNo,
- InvalidateExternalPointerSlots::kNo);
- FieldIndex index =
- FieldIndex::ForPropertyIndex(*receiver_map, details.field_index());
- // Special case deleting the last out-of object property.
- if (!index.is_inobject() && index.outobject_array_index() == 0) {
- DCHECK(!parent_map->HasOutOfObjectProperties());
- // Clear out the properties backing store.
- receiver->SetProperties(ReadOnlyRoots(isolate).empty_fixed_array());
- } else {
- ClearField(isolate, JSObject::cast(*receiver), index);
- if (index.is_inobject()) {
- // We need to clear the recorded slot in this case because in-object
- // slack tracking might not be finished. This ensures that we don't
- // have recorded slots in free space.
- isolate->heap()->ClearRecordedSlot(*receiver,
- receiver->RawField(index.offset()));
- }
- }
- }
- // If the {receiver_map} was marked stable before, then there could be
- // optimized code that depends on the assumption that no object that
- // reached this {receiver_map} transitions away from it without triggering
- // the "deoptimize dependent code" mechanism.
- receiver_map->NotifyLeafMapLayoutChange(isolate);
- // Finally, perform the map rollback.
- receiver->set_map(*parent_map, kReleaseStore);
-#if VERIFY_HEAP
- if (v8_flags.verify_heap) {
- receiver->HeapObjectVerify(isolate);
- receiver->property_array()->PropertyArrayVerify(isolate);
- }
-#endif
-
- // If the {descriptor} was "const" so far, we need to update the
- // {receiver_map} here, otherwise we could get the constants wrong, i.e.
- //
- // o.x = 1;
- // [change o.x's attributes or reconfigure property kind]
- // delete o.x;
- // o.x = 2;
- //
- // could trick V8 into thinking that `o.x` is still 1 even after the second
- // assignment.
-
- // Step 1: Migrate object to an up-to-date shape.
- if (parent_map->is_deprecated()) {
- JSObject::MigrateInstance(isolate, Handle<JSObject>::cast(receiver));
- parent_map = handle(receiver->map(), isolate);
- }
-
- // Step 2: Mark outgoing transitions from the up-to-date version of the
- // parent_map to same property name of any kind or attributes as mutable.
- // Also migrate object to the up-to-date map to make the object shapes
- // converge sooner.
- GeneralizeAllTransitionsToFieldAsMutable(isolate, parent_map, key);
-
- return true;
-}
-
-} // namespace
-
Maybe<bool> Runtime::DeleteObjectProperty(Isolate* isolate,
Handle<JSReceiver> receiver,
Handle<Object> key,
LanguageMode language_mode) {
- if (DeleteObjectPropertyFast(isolate, receiver, key)) return Just(true);
-
bool success = false;
PropertyKey lookup_key(isolate, key, &success);
if (!success) return Nothing<bool>();
diff --git a/test/cctest/test-field-type-tracking.cc b/test/cctest/test-field-type-tracking.cc
index 41500ea14a571d3a081c79ed10f0d37430df852c..65193aa2308764cdf64c1c75d5a8ada4e6315c30 100644
--- a/test/cctest/test-field-type-tracking.cc
+++ b/test/cctest/test-field-type-tracking.cc
@@ -3030,122 +3030,6 @@ TEST(RepresentationPredicatesAreInSync) {
}
}

-TEST(DeletePropertyGeneralizesConstness) {
- CcTest::InitializeVM();
- v8::HandleScope scope(CcTest::isolate());
- Isolate* isolate = CcTest::i_isolate();
- Handle<FieldType> any_type = FieldType::Any(isolate);
-
- // Create a map with some properties.
- Handle<Map> initial_map = Map::Create(isolate, kPropCount + 3);
- Handle<Map> map = initial_map;
- for (int i = 0; i < kPropCount; i++) {
- Handle<String> name = CcTest::MakeName("prop", i);
- map = Map::CopyWithField(isolate, map, name, any_type, NONE,
- PropertyConstness::kConst, Representation::Smi(),
- INSERT_TRANSITION)
- .ToHandleChecked();
- }
- Handle<Map> parent_map = map;
- CHECK(!map->is_deprecated());
-
- Handle<String> name_x = CcTest::MakeString("x");
- Handle<String> name_y = CcTest::MakeString("y");
-
- map = Map::CopyWithField(isolate, parent_map, name_x, any_type, NONE,
- PropertyConstness::kConst, Representation::Smi(),
- INSERT_TRANSITION)
- .ToHandleChecked();
-
- // Create an object, initialize its properties and add a couple of clones.
- Handle<JSObject> object1 = isolate->factory()->NewJSObjectFromMap(map);
- for (int i = 0; i < kPropCount; i++) {
- FieldIndex index = FieldIndex::ForDescriptor(*map, InternalIndex(i));
- object1->FastPropertyAtPut(index, Smi::FromInt(i));
- }
- Handle<JSObject> object2 = isolate->factory()->CopyJSObject(object1);
-
- CHECK(!map->is_deprecated());
- CHECK(!parent_map->is_deprecated());
-
- // Transition to Double must deprecate m1.
- CHECK(!Representation::Smi().CanBeInPlaceChangedTo(Representation::Double()));
-
- // Reconfigure one of the first properties to make the whole transition tree
- // deprecated (including |parent_map| and |map|).
- Handle<Map> new_map =
- ReconfigureProperty(isolate, map, InternalIndex(0), PropertyKind::kData,
- NONE, Representation::Double(), any_type);
- CHECK(map->is_deprecated());
- CHECK(parent_map->is_deprecated());
- CHECK(!new_map->is_deprecated());
- // The "x" property is still kConst.
- CHECK_EQ(new_map->GetLastDescriptorDetails(isolate).constness(),
- PropertyConstness::kConst);
-
- Handle<Map> new_parent_map = Map::Update(isolate, parent_map);
- CHECK(!new_parent_map->is_deprecated());
-
- // |new_parent_map| must have exactly one outgoing transition to |new_map|.
- {
- TransitionsAccessor ta(isolate, *new_parent_map);
- CHECK_EQ(ta.NumberOfTransitions(), 1);
- CHECK_EQ(ta.GetTarget(0), *new_map);
- }
-
- // Deletion of the property from |object1| must migrate it to |new_parent_map|
- // which is an up-to-date version of the |parent_map|. The |new_map|'s "x"
- // property should be marked as mutable.
- CHECK_EQ(object1->map(isolate), *map);
- CHECK(Runtime::DeleteObjectProperty(isolate, object1, name_x,
- LanguageMode::kSloppy)
- .ToChecked());
- CHECK_EQ(object1->map(isolate), *new_parent_map);
- CHECK_EQ(new_map->GetLastDescriptorDetails(isolate).constness(),
- PropertyConstness::kMutable);
-
- // Now add transitions to "x" and "y" properties from |new_parent_map|.
- std::vector<Handle<Map>> transitions;
- Handle<Object> value = handle(Smi::FromInt(0), isolate);
- for (int i = 0; i < kPropertyAttributesCombinationsCount; i++) {
- auto attributes = PropertyAttributesFromInt(i);
-
- Handle<Map> tmp;
- // Add some transitions to "x" and "y".
- tmp = Map::TransitionToDataProperty(isolate, new_parent_map, name_x, value,
- attributes, PropertyConstness::kConst,
- StoreOrigin::kNamed);
- CHECK(!tmp->map(isolate)->is_dictionary_map());
- transitions.push_back(tmp);
-
- tmp = Map::TransitionToDataProperty(isolate, new_parent_map, name_y, value,
- attributes, PropertyConstness::kConst,
- StoreOrigin::kNamed);
- CHECK(!tmp->map(isolate)->is_dictionary_map());
- transitions.push_back(tmp);
- }
-
- // Deletion of the property from |object2| must migrate it to |new_parent_map|
- // which is an up-to-date version of the |parent_map|.
- // All outgoing transitions from |new_map| that add "x" must be marked as
- // mutable, transitions to other properties must remain const.
- CHECK_EQ(object2->map(isolate), *map);
- CHECK(Runtime::DeleteObjectProperty(isolate, object2, name_x,
- LanguageMode::kSloppy)
- .ToChecked());
- CHECK_EQ(object2->map(isolate), *new_parent_map);
- for (Handle<Map> m : transitions) {
- if (m->GetLastDescriptorName(isolate) == *name_x) {
- CHECK_EQ(m->GetLastDescriptorDetails(isolate).constness(),
- PropertyConstness::kMutable);
-
- } else {
- CHECK_EQ(m->GetLastDescriptorDetails(isolate).constness(),
- PropertyConstness::kConst);
- }
- }
-}
-
#define CHECK_SAME(object, rep, expected) \
CHECK_EQ(Object::FitsRepresentation(*object, rep, true), \
Object::FitsRepresentation(*object, rep, false)); \