Revert of [builtins] DeleteProperty: Handle last-added fast properties...

Revert of [builtins] DeleteProperty: Handle last-added fast properties (patchset #2 id:20001 of https://codereview.chromium.org/2830093002/ )

Reason for revert:
Breaks:
https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds/12920
and
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/builds/10281

Original issue's description:
> [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}
> Committed: https://chromium.googlesource.com/v8/v8/+/98acfb36e1acf2ab52ab6b6439eb6356c83dcda6

TBR=ishell@chromium.org,jkummerow@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2843473002
Cr-Commit-Position: refs/heads/master@{#44806}

src/builtins/builtins-internal-gen.cc

@@ -169,85 +169,6 @@ 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<Map::IsUnstable>(bitfield3), slow);
-    // (2) The property to be deleted must be the last property.
-    Node* descriptors = LoadMapDescriptors(receiver_map);
-    Node* nof = DecodeWord32<Map::NumberOfOwnDescriptorsBits>(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<DescriptorArray>(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<Map::NumberOfOwnDescriptorsBits>(
-        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<PropertyDetails::LocationField>(details),
-                          Int32Constant(kField)),
-           &zapping_done);
-    Node* field_index =
-        DecodeWordFromWord32<PropertyDetails::FieldIndexField>(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) {
@@ -329,8 +250,8 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) {
     Node* properties_map = LoadMap(properties);
     GotoIf(WordEqual(properties_map, LoadRoot(Heap::kHashTableMapRootIndex)),
            &dictionary);
-    DeleteFastProperty(receiver, receiver_map, properties, unique, &dont_delete,
-                       &if_notfound, &slow);
+    // TODO(jkummerow): Implement support for fast properties?
+    Goto(&slow);
 
     BIND(&dictionary);
     {

src/compiler/js-native-context-specialization.cc

@@ -2310,15 +2310,6 @@ Node* JSNativeContextSpecialization::BuildCheckMaps(
 
 Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore(
     Handle<Map> 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();

src/ic/accessor-assembler.cc

@@ -984,7 +984,15 @@ void AccessorAssembler::HandleStoreFieldAndReturn(Node* handler_word,
   BIND(&if_out_of_object);
   {
     if (transition_to_field) {
-      ExtendPropertiesBackingStore(holder, handler_word);
+      Label storage_extended(this);
+      GotoIfNot(IsSetWord<StoreHandler::ExtendStorageBits>(handler_word),
+                &storage_extended);
+      Comment("[ Extend storage");
+      ExtendPropertiesBackingStore(holder);
+      Comment("] Extend storage");
+      Goto(&storage_extended);
+
+      BIND(&storage_extended);
     }
 
     StoreNamedField(handler_word, holder, false, representation, prepared_value,
@@ -1045,12 +1053,7 @@ Node* AccessorAssembler::PrepareValueForStore(Node* handler_word, Node* holder,
   return value;
 }
 
-void AccessorAssembler::ExtendPropertiesBackingStore(Node* object,
-                                                     Node* handler_word) {
-  Label done(this);
-  GotoIfNot(IsSetWord<StoreHandler::ExtendStorageBits>(handler_word), &done);
-  Comment("[ Extend storage");
-
+void AccessorAssembler::ExtendPropertiesBackingStore(Node* object) {
   ParameterMode mode = OptimalParameterMode();
 
   Node* properties = LoadProperties(object);
@@ -1058,14 +1061,6 @@ 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<StoreHandler::FieldOffsetBits>(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);
 
@@ -1093,10 +1088,6 @@ 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,

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, Node* handler_word);
+  void ExtendPropertiesBackingStore(Node* object);
 
   void StoreNamedField(Node* handler_word, Node* object, bool is_inobject,
                        Representation representation, Node* value,

src/objects.cc

@@ -3458,15 +3458,11 @@ void MigrateFastToFast(Handle<JSObject> object, Handle<Map> 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 ||
-        (have_space &&
+        (old_map->unused_property_fields() > 0 &&
          ((FLAG_unbox_double_fields && object->properties()->length() == 0) ||
           !details.representation().IsDouble()))) {
       object->synchronized_set_map(*new_map);
@@ -3475,7 +3471,7 @@ void MigrateFastToFast(Handle<JSObject> object, Handle<Map> new_map) {
 
     // If there is still space in the object, we need to allocate a mutable
     // double box.
-    if (have_space) {
+    if (old_map->unused_property_fields() > 0) {
       FieldIndex index =
           FieldIndex::ForDescriptor(*new_map, new_map->LastAdded());
       DCHECK(details.representation().IsDouble());
@@ -3502,6 +3498,7 @@ void MigrateFastToFast(Handle<JSObject> object, Handle<Map> 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);
 

test/mjsunit/dictionary-properties.js

@@ -11,7 +11,6 @@ function SlowObject() {
   this.foo = 1;
   this.bar = 2;
   this.qux = 3;
-  this.z = 4;
   delete this.qux;
   assertFalse(%HasFastProperties(this));
 }
@@ -39,7 +38,6 @@ 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;

test/mjsunit/regress/regress-252797.js

@@ -45,7 +45,6 @@ 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));