From 98acfb36e1acf2ab52ab6b6439eb6356c83dcda6 Mon Sep 17 00:00:00 2001 From: jkummerow Date: Mon, 24 Apr 2017 06:27:41 -0700 Subject: [PATCH] [builtins] DeleteProperty: Handle last-added fast properties In general, deleting a property from a fast-properties object requires transitioning the object to dictionary mode. However, when the most-recently-added property is deleted, we can simply roll back the last map transition that the object went through. This is a performance experiment: it should make things faster, but if it turns out to have more negative than positive impact, we will have to revert it. TBR=bmeurer@chromium.org (just adding a comment) Review-Url: https://codereview.chromium.org/2830093002 Cr-Commit-Position: refs/heads/master@{#44799} --- src/builtins/builtins-internal-gen.cc | 83 ++++++++++++++++++- .../js-native-context-specialization.cc | 9 ++ src/ic/accessor-assembler.cc | 29 ++++--- src/ic/accessor-assembler.h | 2 +- src/objects.cc | 9 +- test/mjsunit/dictionary-properties.js | 2 + test/mjsunit/regress/regress-252797.js | 1 + 7 files changed, 119 insertions(+), 16 deletions(-) diff --git a/src/builtins/builtins-internal-gen.cc b/src/builtins/builtins-internal-gen.cc index b152d63bbd..a7057f25f4 100644 --- a/src/builtins/builtins-internal-gen.cc +++ b/src/builtins/builtins-internal-gen.cc @@ -169,6 +169,85 @@ class DeletePropertyBaseAssembler : public CodeStubAssembler { explicit DeletePropertyBaseAssembler(compiler::CodeAssemblerState* state) : CodeStubAssembler(state) {} + void DeleteFastProperty(Node* receiver, Node* receiver_map, Node* properties, + Node* name, Label* dont_delete, Label* not_found, + Label* slow) { + // This builtin 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 current map must not be marked stable. Otherwise there could + // be optimized code that depends on the assumption that no object that + // reached this map transitions away from it (without triggering the + // "deoptimize dependent code" mechanism). + Node* bitfield3 = LoadMapBitField3(receiver_map); + GotoIfNot(IsSetWord32(bitfield3), slow); + // (2) The property to be deleted must be the last property. + Node* descriptors = LoadMapDescriptors(receiver_map); + Node* nof = DecodeWord32(bitfield3); + GotoIf(Word32Equal(nof, Int32Constant(0)), not_found); + Node* descriptor_number = Int32Sub(nof, Int32Constant(1)); + Node* key_index = DescriptorArrayToKeyIndex(descriptor_number); + Node* actual_key = LoadFixedArrayElement(descriptors, key_index); + // TODO(jkummerow): We could implement full descriptor search in order + // to avoid the runtime call for deleting nonexistent properties, but + // that's probably a rare case. + GotoIf(WordNotEqual(actual_key, name), slow); + // (3) The property to be deleted must be deletable. + Node* details = + LoadDetailsByKeyIndex(descriptors, key_index); + GotoIf(IsSetWord32(details, PropertyDetails::kAttributesDontDeleteMask), + dont_delete); + // (4) The map must have a back pointer. + Node* backpointer = + LoadObjectField(receiver_map, Map::kConstructorOrBackPointerOffset); + GotoIfNot(IsMap(backpointer), slow); + // (5) The last transition must have been caused by adding a property + // (and not any kind of special transition). + Node* previous_nof = DecodeWord32( + LoadMapBitField3(backpointer)); + GotoIfNot(Word32Equal(previous_nof, descriptor_number), slow); + + // Preconditions successful, perform the map rollback! + // Zap the property to avoid keeping objects alive. + // Zapping is not necessary for properties stored in the descriptor array. + Label zapping_done(this); + GotoIf(Word32NotEqual(DecodeWord32(details), + Int32Constant(kField)), + &zapping_done); + Node* field_index = + DecodeWordFromWord32(details); + Node* inobject_properties = LoadMapInobjectProperties(receiver_map); + Label inobject(this), backing_store(this); + // We don't need to special-case inobject slack tracking here (by using + // the one_pointer_filler_map as filler), because it'll trim objects to + // the size of the largest known map anyway, so rolled-back properties + // can be zapped with |undefined|. + Node* filler = UndefinedConstant(); + DCHECK(Heap::RootIsImmortalImmovable(Heap::kUndefinedValueRootIndex)); + Branch(UintPtrLessThan(field_index, inobject_properties), &inobject, + &backing_store); + BIND(&inobject); + { + Node* field_offset = + IntPtrMul(IntPtrSub(LoadMapInstanceSize(receiver_map), + IntPtrSub(inobject_properties, field_index)), + IntPtrConstant(kPointerSize)); + StoreObjectFieldNoWriteBarrier(receiver, field_offset, filler); + Goto(&zapping_done); + } + BIND(&backing_store); + { + Node* backing_store_index = IntPtrSub(field_index, inobject_properties); + StoreFixedArrayElement(properties, backing_store_index, filler, + SKIP_WRITE_BARRIER); + Goto(&zapping_done); + } + BIND(&zapping_done); + StoreMap(receiver, backpointer); + Return(TrueConstant()); + } + void DeleteDictionaryProperty(Node* receiver, Node* properties, Node* name, Node* context, Label* dont_delete, Label* notfound) { @@ -250,8 +329,8 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) { Node* properties_map = LoadMap(properties); GotoIf(WordEqual(properties_map, LoadRoot(Heap::kHashTableMapRootIndex)), &dictionary); - // TODO(jkummerow): Implement support for fast properties? - Goto(&slow); + DeleteFastProperty(receiver, receiver_map, properties, unique, &dont_delete, + &if_notfound, &slow); BIND(&dictionary); { diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index 88d0fba216..661b025d0a 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -2310,6 +2310,15 @@ Node* JSNativeContextSpecialization::BuildCheckMaps( Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore( Handle map, Node* properties, Node* effect, Node* control) { + // TODO(bmeurer/jkummerow): Property deletions can undo map transitions + // while keeping the backing store around, meaning that even though the + // map might believe that objects have no unused property fields, there + // might actually be some. It would be nice to not create a new backing + // store in that case (i.e. when properties->length() >= new_length). + // However, introducing branches and Phi nodes here would make it more + // difficult for escape analysis to get rid of the backing stores used + // for intermediate states of chains of property additions. That makes + // it unclear what the best approach is here. DCHECK_EQ(0, map->unused_property_fields()); // Compute the length of the old {properties} and the new properties. int length = map->NextFreePropertyIndex() - map->GetInObjectProperties(); diff --git a/src/ic/accessor-assembler.cc b/src/ic/accessor-assembler.cc index c048c7ec81..34b3402dba 100644 --- a/src/ic/accessor-assembler.cc +++ b/src/ic/accessor-assembler.cc @@ -984,15 +984,7 @@ void AccessorAssembler::HandleStoreFieldAndReturn(Node* handler_word, BIND(&if_out_of_object); { if (transition_to_field) { - Label storage_extended(this); - GotoIfNot(IsSetWord(handler_word), - &storage_extended); - Comment("[ Extend storage"); - ExtendPropertiesBackingStore(holder); - Comment("] Extend storage"); - Goto(&storage_extended); - - BIND(&storage_extended); + ExtendPropertiesBackingStore(holder, handler_word); } StoreNamedField(handler_word, holder, false, representation, prepared_value, @@ -1053,7 +1045,12 @@ Node* AccessorAssembler::PrepareValueForStore(Node* handler_word, Node* holder, return value; } -void AccessorAssembler::ExtendPropertiesBackingStore(Node* object) { +void AccessorAssembler::ExtendPropertiesBackingStore(Node* object, + Node* handler_word) { + Label done(this); + GotoIfNot(IsSetWord(handler_word), &done); + Comment("[ Extend storage"); + ParameterMode mode = OptimalParameterMode(); Node* properties = LoadProperties(object); @@ -1061,6 +1058,14 @@ void AccessorAssembler::ExtendPropertiesBackingStore(Node* object) { ? LoadAndUntagFixedArrayBaseLength(properties) : LoadFixedArrayBaseLength(properties); + // Previous property deletion could have left behind unused backing store + // capacity even for a map that think it doesn't have any unused fields. + // Perform a bounds check to see if we actually have to grow the array. + Node* offset = DecodeWord(handler_word); + Node* size = ElementOffsetFromIndex(length, FAST_ELEMENTS, mode, + FixedArray::kHeaderSize); + GotoIf(UintPtrLessThan(offset, size), &done); + Node* delta = IntPtrOrSmiConstant(JSObject::kFieldsAdded, mode); Node* new_capacity = IntPtrOrSmiAdd(length, delta, mode); @@ -1088,6 +1093,10 @@ void AccessorAssembler::ExtendPropertiesBackingStore(Node* object) { SKIP_WRITE_BARRIER, mode); StoreObjectField(object, JSObject::kPropertiesOffset, new_properties); + Comment("] Extend storage"); + Goto(&done); + + BIND(&done); } void AccessorAssembler::StoreNamedField(Node* handler_word, Node* object, diff --git a/src/ic/accessor-assembler.h b/src/ic/accessor-assembler.h index f2cafdb128..5644fa8ae8 100644 --- a/src/ic/accessor-assembler.h +++ b/src/ic/accessor-assembler.h @@ -193,7 +193,7 @@ class AccessorAssembler : public CodeStubAssembler { Node* value, Label* bailout); // Extends properties backing store by JSObject::kFieldsAdded elements. - void ExtendPropertiesBackingStore(Node* object); + void ExtendPropertiesBackingStore(Node* object, Node* handler_word); void StoreNamedField(Node* handler_word, Node* object, bool is_inobject, Representation representation, Node* value, diff --git a/src/objects.cc b/src/objects.cc index 4974f4f178..53c476407a 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -3458,11 +3458,15 @@ void MigrateFastToFast(Handle object, Handle new_map) { } PropertyDetails details = new_map->GetLastDescriptorDetails(); + int target_index = details.field_index() - new_map->GetInObjectProperties(); + bool have_space = old_map->unused_property_fields() > 0 || + (details.location() == kField && target_index >= 0 && + object->properties()->length() > target_index); // Either new_map adds an kDescriptor property, or a kField property for // which there is still space, and which does not require a mutable double // box (an out-of-object double). if (details.location() == kDescriptor || - (old_map->unused_property_fields() > 0 && + (have_space && ((FLAG_unbox_double_fields && object->properties()->length() == 0) || !details.representation().IsDouble()))) { object->synchronized_set_map(*new_map); @@ -3471,7 +3475,7 @@ void MigrateFastToFast(Handle object, Handle new_map) { // If there is still space in the object, we need to allocate a mutable // double box. - if (old_map->unused_property_fields() > 0) { + if (have_space) { FieldIndex index = FieldIndex::ForDescriptor(*new_map, new_map->LastAdded()); DCHECK(details.representation().IsDouble()); @@ -3498,7 +3502,6 @@ void MigrateFastToFast(Handle object, Handle new_map) { } DCHECK_EQ(kField, details.location()); DCHECK_EQ(kData, details.kind()); - int target_index = details.field_index() - new_map->GetInObjectProperties(); DCHECK(target_index >= 0); // Must be a backing store index. new_storage->set(target_index, *value); diff --git a/test/mjsunit/dictionary-properties.js b/test/mjsunit/dictionary-properties.js index 33360d7f52..cffa48547e 100644 --- a/test/mjsunit/dictionary-properties.js +++ b/test/mjsunit/dictionary-properties.js @@ -11,6 +11,7 @@ function SlowObject() { this.foo = 1; this.bar = 2; this.qux = 3; + this.z = 4; delete this.qux; assertFalse(%HasFastProperties(this)); } @@ -38,6 +39,7 @@ function SlowPrototype() { } SlowPrototype.prototype.bar = 2; SlowPrototype.prototype.baz = 3; +SlowPrototype.prototype.z = 4; delete SlowPrototype.prototype.baz; assertFalse(%HasFastProperties(SlowPrototype.prototype)); var slow_proto = new SlowPrototype; diff --git a/test/mjsunit/regress/regress-252797.js b/test/mjsunit/regress/regress-252797.js index c3bb139965..900073bdbb 100644 --- a/test/mjsunit/regress/regress-252797.js +++ b/test/mjsunit/regress/regress-252797.js @@ -45,6 +45,7 @@ assertFalse(%HasFastProperties(holder)); // Create a receiver into dictionary mode. var receiver = Object.create(holder, { killMe: {value: 0, configurable: true}, + keepMe: {value: 0, configurable: true} }); delete receiver.killMe; assertFalse(%HasFastProperties(receiver));