Cleanup Delete backend implementation.

src/elements.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 #include "src/arguments.h"
 #include "src/conversions.h"
 #include "src/elements.h"
 #include "src/messages.h"
@@ skipping 713 matching lines @@
       JSObject::PrintElementsTransition(stdout, object, from_kind, old_elements,
                                         to_kind, elements);
     }
   }
 
   virtual void GrowCapacityAndConvert(Handle<JSObject> object,
                                       uint32_t capacity) final {
     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,
                                uint32_t to_start, int packed_size,
                                int copy_size) {
     UNREACHABLE();
   }
 
   virtual void CopyElements(Handle<FixedArrayBase> from, uint32_t from_start,
                             ElementsKind from_kind, Handle<FixedArrayBase> to,
@@ skipping 236 matching lines @@
   }
 
   static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start,
                                FixedArrayBase* to, ElementsKind from_kind,
                                uint32_t to_start, int packed_size,
                                int copy_size) {
     UNREACHABLE();
   }
 
 
-  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);
+  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 (is_arguments) {
-        FixedArray::cast(obj->elements())->set(1, *new_elements);
-      } else {
-        obj->set_elements(*new_elements);
-      }
+    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,
                                 Handle<FixedArrayBase> store) {
     Handle<SeededNumberDictionary> backing_store =
         Handle<SeededNumberDictionary>::cast(store);
     Isolate* isolate = backing_store->GetIsolate();
     int entry = backing_store->FindEntry(key);
     if (entry != SeededNumberDictionary::kNotFound) {
       return handle(backing_store->ValueAt(entry), isolate);
@@ skipping 87 matching lines @@
          typename KindTraits>
 class FastElementsAccessor
     : public ElementsAccessorBase<FastElementsAccessorSubclass, KindTraits> {
  public:
   explicit FastElementsAccessor(const char* name)
       : ElementsAccessorBase<FastElementsAccessorSubclass,
                              KindTraits>(name) {}
 
   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(store);
+    backing_store->set_the_hole(index);
 
-    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);
+    // 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);
       }
     }
   }
 
   static void ReconfigureImpl(Handle<JSObject> object,
                               Handle<FixedArrayBase> store, uint32_t index,
                               Handle<Object> value,
                               PropertyAttributes attributes) {
     Handle<SeededNumberDictionary> dictionary =
         JSObject::NormalizeElements(object);
@@ skipping 20 matching lines @@
         JSObject::TransitionElementsKind(object, to_kind);
       }
       if (IsFastSmiOrObjectElementsKind(from_kind)) {
         DCHECK(IsFastSmiOrObjectElementsKind(to_kind));
         JSObject::EnsureWritableFastElements(object);
       }
     }
     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) {
     return !BackingStore::cast(backing_store)->is_the_hole(index);
   }
 
   static void ValidateContents(Handle<JSObject> holder, int length) {
 #if DEBUG
     Isolate* isolate = holder->GetIsolate();
     HandleScope scope(isolate);
@@ skipping 220 matching lines @@
                                         uint32_t index) {
     return PropertyDetails(DONT_DELETE, DATA, 0, PropertyCellType::kNoCell);
   }
 
   static void SetLengthImpl(Handle<JSArray> array, uint32_t length,
                             Handle<FixedArrayBase> backing_store) {
     // External arrays do not support changing their length.
     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,
                                      FixedArrayBase* backing_store,
                                      uint32_t key) {
     return key < AccessorClass::GetCapacityImpl(holder, backing_store)
                ? key
                : kMaxUInt32;
   }
 
@@ skipping 55 matching lines @@
         Context* context = Context::cast(parameter_map->get(0));
         int context_index = entry->aliased_context_slot();
         DCHECK(!context->get(context_index)->IsTheHole());
         return handle(context->get(context_index), isolate);
       } else {
         return result;
       }
     }
   }
 
-  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);
     }
   }
 
   static void GrowCapacityAndConvertImpl(Handle<JSObject> object,
                                          uint32_t capacity) {
     UNREACHABLE();
   }
 
   static void SetImpl(FixedArrayBase* store, uint32_t key, Object* value) {
     FixedArray* parameter_map = FixedArray::cast(store);
@@ skipping 388 matching lines @@
       break;
   }
 
   array->set_elements(*elms);
   array->set_length(Smi::FromInt(number_of_elements));
   return array;
 }
 
 }  // namespace internal
 }  // namespace v8