Cleanup Delete backend implementation.

src/elements.cc

Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
index 2082543a2473a7ccde8db3f562669cd38e6102f7..72c8bfd8cb80455f892c9bf9b9676fb067b1f244 100644
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -731,8 +731,7 @@ class ElementsAccessorBase : public ElementsAccessor {
     ElementsAccessorSubclass::GrowCapacityAndConvertImpl(object, capacity);
   }
 
-  virtual void Delete(Handle<JSObject> obj, uint32_t key,
-                      LanguageMode language_mode) override = 0;
+  virtual void Delete(Handle<JSObject> obj, uint32_t index) override = 0;
 
   static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
                                FixedArrayBase* to, ElementsKind from_kind,
@@ -989,37 +988,27 @@ class DictionaryElementsAccessor
   }
 
 
-  static void DeleteCommon(Handle<JSObject> obj, uint32_t key,
-                           LanguageMode language_mode) {
-    Isolate* isolate = obj->GetIsolate();
-    Handle<FixedArray> backing_store(FixedArray::cast(obj->elements()),
-                                     isolate);
-    bool is_arguments = obj->HasSloppyArgumentsElements();
-    if (is_arguments) {
-      backing_store = handle(FixedArray::cast(backing_store->get(1)), isolate);
-    }
-    Handle<SeededNumberDictionary> dictionary =
-        Handle<SeededNumberDictionary>::cast(backing_store);
-    int entry = dictionary->FindEntry(key);
-    if (entry != SeededNumberDictionary::kNotFound) {
-      Handle<Object> result =
-          SeededNumberDictionary::DeleteProperty(dictionary, entry);
-      USE(result);
-      DCHECK(result->IsTrue());
-      Handle<FixedArray> new_elements =
-          SeededNumberDictionary::Shrink(dictionary, key);
-
-      if (is_arguments) {
-        FixedArray::cast(obj->elements())->set(1, *new_elements);
-      } else {
-        obj->set_elements(*new_elements);
-      }
+  static void DeleteImpl(Handle<JSObject> obj, uint32_t index,
+                         Handle<FixedArrayBase> store) {
+    Handle<SeededNumberDictionary> dict =
+        Handle<SeededNumberDictionary>::cast(store);
+    // TODO(verwaest): Remove reliance on key in Shrink.
+    uint32_t key = GetKeyForIndexImpl(*dict, index);
+    Handle<Object> result = SeededNumberDictionary::DeleteProperty(dict, index);
+    USE(result);
+    DCHECK(result->IsTrue());
+    Handle<FixedArray> new_elements = SeededNumberDictionary::Shrink(dict, key);
+
+    if (obj->elements() != *store) {

[Igor Sheludko, 2015/07/03 15:09:01]

It looks like you can do the same trick to unify AddImpl() for
DictionaryElementsAccessor and SlowSloppyArgumentsElementsAccessor.

Or even better if you split the addition/deletion into parts - update the
elements dictionary and then set it into the object.

+      DCHECK(obj->HasSlowArgumentsElements());
+      FixedArray::cast(obj->elements())->set(1, *new_elements);
+    } else {
+      obj->set_elements(*new_elements);
     }
   }
 
-  virtual void Delete(Handle<JSObject> obj, uint32_t key,
-                      LanguageMode language_mode) final {
-    DeleteCommon(obj, key, language_mode);
+  virtual void Delete(Handle<JSObject> obj, uint32_t index) final {

[Igor Sheludko, 2015/07/03 15:09:01]

Could you please move this virtual Delete() implementation to
ElementsAccessorBase class and in all the successors replace virtual Delete()
with static DeleteImpl()? Like we do for virtual Get()/static GetImpl() and etc.
Just to be uniform.

+    DeleteImpl(obj, index, handle(obj->elements()));
   }
 
   static Handle<Object> GetImpl(Handle<JSObject> obj, uint32_t key,
@@ -1127,58 +1116,38 @@ class FastElementsAccessor
 
   typedef typename KindTraits::BackingStore BackingStore;
 
-  static void DeleteCommon(Handle<JSObject> obj, uint32_t key,
-                           LanguageMode language_mode) {
+  static void DeleteImpl(Handle<JSObject> obj, uint32_t index,
+                         Handle<FixedArrayBase> store) {
     DCHECK(obj->HasFastSmiOrObjectElements() ||
            obj->HasFastDoubleElements() ||
            obj->HasFastArgumentsElements());
-    Isolate* isolate = obj->GetIsolate();
-    Heap* heap = obj->GetHeap();
-    Handle<FixedArrayBase> elements(obj->elements());
-    if (*elements == heap->empty_fixed_array()) return;
-
-    Handle<BackingStore> backing_store = Handle<BackingStore>::cast(elements);
-    bool is_sloppy_arguments_elements_map =
-        backing_store->map() == heap->sloppy_arguments_elements_map();
-    if (is_sloppy_arguments_elements_map) {
-      backing_store = handle(
-          BackingStore::cast(Handle<FixedArray>::cast(backing_store)->get(1)),
-          isolate);
+    Handle<BackingStore> backing_store = Handle<BackingStore>::cast(store);
+    backing_store->set_the_hole(index);
+
+    // TODO(verwaest): Move this out of elements.cc.
+    // If an old space backing store is larger than a certain size and
+    // has too few used values, normalize it.
+    // To avoid doing the check on every delete we require at least
+    // one adjacent hole to the value being deleted.
+    const int kMinLengthForSparsenessCheck = 64;
+    if (backing_store->length() < kMinLengthForSparsenessCheck) return;
+    if (backing_store->GetHeap()->InNewSpace(*backing_store)) return;
+    uint32_t length = 0;
+    if (obj->IsJSArray()) {
+      JSArray::cast(*obj)->length()->ToArrayLength(&length);
+    } else {
+      length = static_cast<uint32_t>(store->length());
     }
-    uint32_t length = static_cast<uint32_t>(
-        obj->IsJSArray()
-        ? Smi::cast(Handle<JSArray>::cast(obj)->length())->value()
-        : backing_store->length());
-    if (key < length) {
-      if (!is_sloppy_arguments_elements_map) {
-        ElementsKind kind = KindTraits::Kind;
-        if (IsFastPackedElementsKind(kind)) {
-          JSObject::TransitionElementsKind(obj, GetHoleyElementsKind(kind));
-        }
-        if (IsFastSmiOrObjectElementsKind(KindTraits::Kind)) {
-          Handle<Object> writable = JSObject::EnsureWritableFastElements(obj);
-          backing_store = Handle<BackingStore>::cast(writable);
-        }
+    if ((index > 0 && backing_store->is_the_hole(index - 1)) ||
+        (index + 1 < length && backing_store->is_the_hole(index + 1))) {
+      int num_used = 0;
+      for (int i = 0; i < backing_store->length(); ++i) {
+        if (!backing_store->is_the_hole(i)) ++num_used;
+        // Bail out early if more than 1/4 is used.
+        if (4 * num_used > backing_store->length()) break;
       }
-      backing_store->set_the_hole(key);
-      // If an old space backing store is larger than a certain size and
-      // has too few used values, normalize it.
-      // To avoid doing the check on every delete we require at least
-      // one adjacent hole to the value being deleted.
-      const int kMinLengthForSparsenessCheck = 64;
-      if (backing_store->length() >= kMinLengthForSparsenessCheck &&
-          !heap->InNewSpace(*backing_store) &&
-          ((key > 0 && backing_store->is_the_hole(key - 1)) ||
-           (key + 1 < length && backing_store->is_the_hole(key + 1)))) {
-        int num_used = 0;
-        for (int i = 0; i < backing_store->length(); ++i) {
-          if (!backing_store->is_the_hole(i)) ++num_used;
-          // Bail out early if more than 1/4 is used.
-          if (4 * num_used > backing_store->length()) break;
-        }
-        if (4 * num_used <= backing_store->length()) {
-          JSObject::NormalizeElements(obj);
-        }
+      if (4 * num_used <= backing_store->length()) {
+        JSObject::NormalizeElements(obj);
       }
     }
   }
@@ -1219,9 +1188,15 @@ class FastElementsAccessor
     FastElementsAccessorSubclass::SetImpl(object->elements(), index, *value);
   }
 
-  virtual void Delete(Handle<JSObject> obj, uint32_t key,
-                      LanguageMode language_mode) final {
-    DeleteCommon(obj, key, language_mode);
+  virtual void Delete(Handle<JSObject> obj, uint32_t index) final {
+    ElementsKind kind = KindTraits::Kind;
+    if (IsFastPackedElementsKind(kind)) {
+      JSObject::TransitionElementsKind(obj, GetHoleyElementsKind(kind));
+    }
+    if (IsFastSmiOrObjectElementsKind(KindTraits::Kind)) {
+      JSObject::EnsureWritableFastElements(obj);
+    }
+    DeleteImpl(obj, index, handle(obj->elements()));
   }
 
   static bool HasIndexImpl(FixedArrayBase* backing_store, uint32_t index) {
@@ -1462,9 +1437,8 @@ class TypedElementsAccessor
     UNREACHABLE();
   }
 
-  virtual void Delete(Handle<JSObject> obj, uint32_t key,
-                      LanguageMode language_mode) final {
-    // External arrays always ignore deletes.
+  virtual void Delete(Handle<JSObject> obj, uint32_t index) final {
+    UNREACHABLE();
   }
 
   static uint32_t GetIndexForKeyImpl(JSObject* holder,
@@ -1540,16 +1514,17 @@ class SloppyArgumentsElementsAccessor
     }
   }
 
-  virtual void Delete(Handle<JSObject> obj, uint32_t key,
-                      LanguageMode language_mode) final {
+  virtual void Delete(Handle<JSObject> obj, uint32_t index) final {
     FixedArray* parameter_map = FixedArray::cast(obj->elements());
-    if (!GetParameterMapArg(parameter_map, key)->IsTheHole()) {
+    uint32_t length = static_cast<uint32_t>(parameter_map->length()) - 2;
+    if (index < length) {
       // TODO(kmillikin): We could check if this was the last aliased
       // parameter, and revert to normal elements in that case.  That
       // would enable GC of the context.
-      parameter_map->set_the_hole(key + 2);
+      parameter_map->set_the_hole(index + 2);
     } else {
-      ArgumentsAccessor::DeleteCommon(obj, key, language_mode);
+      Handle<FixedArray> arguments(FixedArray::cast(parameter_map->get(1)));
+      ArgumentsAccessor::DeleteImpl(obj, index - length, arguments);
     }
   }