[array] Refactor sort pre-processing into a single runtime function.

This CL consolidates CopyFromPrototype and RemoveArrayHoles into a
single runtime function. It also creates two small helper functions
that are needed in both pre-processing steps.

Additionally it removes the return value from CopyFromPrototype since
it is no longer needed (it was previously used by a sort post-
processing step that no longer exists).

Bug: v8:7382
Change-Id: I7f9b00c1bc639d2118fdecef9c3b45c2cf010310
Reviewed-on: https://chromium-review.googlesource.com/1051887
Commit-Queue: Simon Zünd <szuend@google.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53199}

src/debug/debug-evaluate.cc

@@ -345,14 +345,13 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
   V(LoadLookupSlotForCall)               \
   /* Arrays */                           \
   V(ArraySpeciesConstructor)             \
-  V(CopyFromPrototype)                   \
   V(EstimateNumberOfElements)            \
   V(GetArrayKeys)                        \
   V(HasComplexElements)                  \
   V(HasFastPackedElements)               \
   V(NewArray)                            \
   V(NormalizeElements)                   \
-  V(RemoveArrayHoles)                    \
+  V(PrepareElementsForSort)              \
   V(TrySliceSimpleNonFastElements)       \
   V(TypedArrayGetBuffer)                 \
   /* Errors */                           \

src/js/array.js

@@ -803,23 +803,19 @@ function InnerArraySort(array, length, comparefn) {
 
   if (length < 2) return array;
 
-  var is_array = IS_ARRAY(array);
-  var max_prototype_element;
-  if (!is_array) {
-    // For compatibility with JSC, we also sort elements inherited from
-    // the prototype chain on non-Array objects.
-    // We do this by copying them to this object and sorting only
-    // own elements. This is not very efficient, but sorting with
-    // inherited elements happens very, very rarely, if at all.
-    // The specification allows "implementation dependent" behavior
-    // if an element on the prototype chain has an element that
-    // might interact with sorting.
-    max_prototype_element = %CopyFromPrototype(array, length);
-  }
-
-  // %RemoveArrayHoles moves all non-undefined elements to the front of the
-  // array and moves the undefineds after that. Holes are removed.
-  var num_non_undefined = %RemoveArrayHoles(array, length);
+  // For compatibility with JSC, we also sort elements inherited from
+  // the prototype chain on non-Array objects.
+  // We do this by copying them to this object and sorting only
+  // own elements. This is not very efficient, but sorting with
+  // inherited elements happens very, very rarely, if at all.
+  // The specification allows "implementation dependent" behavior
+  // if an element on the prototype chain has an element that
+  // might interact with sorting.
+  //
+  // We also move all non-undefined elements to the front of the
+  // array and move the undefineds after that. Holes are removed.
+  // This happens for Array as well as non-Array objects.
+  var num_non_undefined = %PrepareElementsForSort(array, length);
 
   QuickSort(array, 0, num_non_undefined);
 

src/objects.cc

@@ -8896,6 +8896,18 @@ MaybeHandle<FixedArray> JSReceiver::GetOwnEntries(Handle<JSReceiver> object,
                                try_fast_path, true);
 }
 
+Handle<FixedArray> JSReceiver::GetOwnElementIndices(Isolate* isolate,
+                                                    Handle<JSReceiver> receiver,
+                                                    Handle<JSObject> object) {
+  KeyAccumulator accumulator(isolate, KeyCollectionMode::kOwnOnly,
+                             ALL_PROPERTIES);
+  accumulator.CollectOwnElementIndices(receiver, object);
+  Handle<FixedArray> keys =
+      accumulator.GetKeys(GetKeysConversion::kKeepNumbers);
+  DCHECK(keys->ContainsSortedNumbers());
+  return keys;
+}
+
 bool Map::DictionaryElementsInPrototypeChainOnly() {
   if (IsDictionaryElementsKind(elements_kind())) {
     return false;
@@ -10092,6 +10104,20 @@ Handle<FixedArray> FixedArray::SetAndGrow(Handle<FixedArray> array, int index,
   return new_array;
 }
 
+bool FixedArray::ContainsSortedNumbers() {
+  for (int i = 1; i < length(); ++i) {
+    Object* a_obj = get(i - 1);
+    Object* b_obj = get(i);
+    if (!a_obj->IsNumber() || !b_obj->IsNumber()) return false;
+
+    uint32_t a = NumberToUint32(a_obj);
+    uint32_t b = NumberToUint32(b_obj);
+
+    if (a > b) return false;
+  }
+  return true;
+}
+
 void FixedArray::Shrink(int new_length) {
   DCHECK(0 <= new_length && new_length <= length());
   if (new_length < length()) {

src/objects.h

@@ -2262,6 +2262,9 @@ class JSReceiver: public HeapObject {
       Handle<JSReceiver> object, PropertyFilter filter,
       bool try_fast_path = true);
 
+  V8_WARN_UNUSED_RESULT static Handle<FixedArray> GetOwnElementIndices(
+      Isolate* isolate, Handle<JSReceiver> receiver, Handle<JSObject> object);
+
   static const int kHashMask = PropertyArray::HashField::kMask;
 
   // Layout description.

src/objects/fixed-array.h

@@ -127,6 +127,8 @@ class FixedArray : public FixedArrayBase {
 
   inline Object** GetFirstElementAddress();
   inline bool ContainsOnlySmisOrHoles();
+  // Returns true iff the elements are Numbers and sorted ascending.
+  bool ContainsSortedNumbers();
 
   // Gives access to raw memory which stores the array's data.
   inline Object** data_start();

src/runtime/runtime-array.cc

@@ -31,6 +31,7 @@ RUNTIME_FUNCTION(Runtime_TransitionElementsKind) {
 namespace {
 // Find the next free position. undefined and holes are both considered
 // free spots. Returns "Nothing" if an exception occurred.
+V8_WARN_UNUSED_RESULT
 Maybe<uint32_t> FindNextFreePosition(Isolate* isolate,
                                      Handle<JSReceiver> receiver,
                                      uint32_t current_pos) {
@@ -47,12 +48,12 @@ Maybe<uint32_t> FindNextFreePosition(Isolate* isolate,
   }
 }
 
-// As PrepareElementsForSort, but also handles Dictionary elements that stay
+// As RemoveArrayHoles, but also handles Dictionary elements that stay
 // Dictionary (requires_slow_elements() is true), proxies and objects that
 // might have accessors.
-Object* PrepareElementsForSortGeneric(Handle<JSReceiver> receiver,
-                                      uint32_t limit) {
-  Isolate* isolate = receiver->GetIsolate();
+V8_WARN_UNUSED_RESULT
+Object* RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
+                                uint32_t limit) {
   HandleScope scope(isolate);
 
   // For proxies, we do not collect the keys, instead we use all indices in
@@ -65,21 +66,8 @@ Object* PrepareElementsForSortGeneric(Handle<JSReceiver> receiver,
       keys->set(i, Smi::FromInt(i));
     }
   } else {
-    KeyAccumulator accumulator(isolate, KeyCollectionMode::kOwnOnly,
-                               ALL_PROPERTIES);
-    accumulator.CollectOwnElementIndices(receiver,
-                                         Handle<JSObject>::cast(receiver));
-    keys = accumulator.GetKeys(GetKeysConversion::kKeepNumbers);
-
-#ifdef DEBUG
-    // Check that keys are sorted.
-    for (int i = 1; i < keys->length(); ++i) {
-      uint32_t a = NumberToUint32(keys->get(i - 1));
-      uint32_t b = NumberToUint32(keys->get(i));
-
-      DCHECK_LE(a, b);
-    }
-#endif
+    keys = JSReceiver::GetOwnElementIndices(isolate, receiver,
+                                            Handle<JSObject>::cast(receiver));
   }
 
   uint32_t num_undefined = 0;
@@ -168,10 +156,11 @@ Object* PrepareElementsForSortGeneric(Handle<JSReceiver> receiver,
 // start of the elements array.  If the object is in dictionary mode, it is
 // converted to fast elements mode.  Undefined values are placed after
 // non-undefined values.  Returns the number of non-undefined values.
-Object* PrepareElementsForSort(Handle<JSReceiver> receiver, uint32_t limit) {
-  Isolate* isolate = receiver->GetIsolate();
+V8_WARN_UNUSED_RESULT
+Object* RemoveArrayHoles(Isolate* isolate, Handle<JSReceiver> receiver,
+                         uint32_t limit) {
   if (receiver->IsJSProxy()) {
-    return PrepareElementsForSortGeneric(receiver, limit);
+    return RemoveArrayHolesGeneric(isolate, receiver, limit);
   }
 
   Handle<JSObject> object = Handle<JSObject>::cast(receiver);
@@ -181,7 +170,7 @@ Object* PrepareElementsForSort(Handle<JSReceiver> receiver, uint32_t limit) {
   }
 
   if (object->HasSloppyArgumentsElements() || !object->map()->is_extensible()) {
-    return PrepareElementsForSortGeneric(receiver, limit);
+    return RemoveArrayHolesGeneric(isolate, receiver, limit);
   }
 
   JSObject::ValidateElements(*object);
@@ -191,7 +180,7 @@ Object* PrepareElementsForSort(Handle<JSReceiver> receiver, uint32_t limit) {
     Handle<NumberDictionary> dict(object->element_dictionary());
     if (object->IsJSArray() || dict->requires_slow_elements() ||
         dict->max_number_key() >= limit) {
-      return PrepareElementsForSortGeneric(receiver, limit);
+      return RemoveArrayHolesGeneric(isolate, receiver, limit);
     }
     // Convert to fast elements.
     Handle<Map> new_map =
@@ -303,47 +292,10 @@ Object* PrepareElementsForSort(Handle<JSReceiver> receiver, uint32_t limit) {
   return *isolate->factory()->NewNumberFromUint(result);
 }
 
-}  // namespace
-
-// Moves all own elements of an object, that are below a limit, to positions
-// starting at zero. All undefined values are placed after non-undefined values,
-// and are followed by non-existing element. Does not change the length
-// property.
-// Returns the number of non-undefined elements collected.
-// Returns -1 if hole removal is not supported by this method.
-RUNTIME_FUNCTION(Runtime_RemoveArrayHoles) {
-  HandleScope scope(isolate);
-  DCHECK_EQ(2, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, object, 0);
-  CONVERT_NUMBER_CHECKED(uint32_t, limit, Uint32, args[1]);
-  if (isolate->debug_execution_mode() == DebugInfo::kSideEffects) {
-    if (!isolate->debug()->PerformSideEffectCheckForObject(object)) {
-      return isolate->heap()->exception();
-    }
-  }
-
-  // Counter for sorting arrays that have non-packed elements and where either
-  // the ElementsProtector is invalid or the prototype does not match
-  // Array.prototype.
-  if (object->IsJSArray() &&
-      !Handle<JSArray>::cast(object)->HasFastPackedElements()) {
-    JSObject* initial_array_proto = JSObject::cast(
-        isolate->native_context()->get(Context::INITIAL_ARRAY_PROTOTYPE_INDEX));
-    if (!isolate->IsNoElementsProtectorIntact() ||
-        object->map()->prototype() != initial_array_proto) {
-      isolate->CountUsage(
-          v8::Isolate::kArrayPrototypeSortJSArrayModifiedPrototype);
-    }
-  }
-
-  return PrepareElementsForSort(object, limit);
-}
-
-namespace {
-
 // Copy element at index from source to target only if target does not have the
 // element on its own. Returns true if a copy occurred, false if not
 // and Nothing if an exception occurred.
+V8_WARN_UNUSED_RESULT
 Maybe<bool> ConditionalCopy(Isolate* isolate, Handle<JSReceiver> source,
                             Handle<JSReceiver> target, uint32_t index) {
   Maybe<bool> source_has_prop = JSReceiver::HasOwnProperty(source, index);
@@ -369,52 +321,24 @@ Maybe<bool> ConditionalCopy(Isolate* isolate, Handle<JSReceiver> source,
   return Just(true);
 }
 
-}  // namespace
-
 // Copy elements in the range 0..length from objects prototype chain
-// to object itself, if object has holes. Return one more than the maximal
-// index of a prototype property.
-RUNTIME_FUNCTION(Runtime_CopyFromPrototype) {
-  HandleScope scope(isolate);
-  DCHECK_EQ(2, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, object, 0);
-  DCHECK(!object->IsJSArray());
-  CONVERT_NUMBER_CHECKED(uint32_t, length, Uint32, args[1]);
-
-  if (isolate->debug_execution_mode() == DebugInfo::kSideEffects) {
-    if (!isolate->debug()->PerformSideEffectCheckForObject(object)) {
-      return isolate->heap()->exception();
-    }
-  }
-
-  uint32_t max = 0;
+// to object itself, if object has holes. Returns null on error and undefined on
+// success.
+V8_WARN_UNUSED_RESULT
+MaybeHandle<Object> CopyFromPrototype(Isolate* isolate,
+                                      Handle<JSReceiver> object,
+                                      uint32_t length) {
   for (PrototypeIterator iter(isolate, object, kStartAtPrototype);
        !iter.IsAtEnd(); iter.Advance()) {
     Handle<JSReceiver> current(PrototypeIterator::GetCurrent<JSReceiver>(iter));
 
     if (current->IsJSProxy()) {
       for (uint32_t i = 0; i < length; ++i) {
-        Maybe<bool> did_copy = ConditionalCopy(isolate, current, object, i);
-        MAYBE_RETURN(did_copy, isolate->heap()->exception());
-        if (did_copy.FromJust() && i >= max) max = i + 1;
+        MAYBE_RETURN_NULL(ConditionalCopy(isolate, current, object, i));
       }
     } else {
-      KeyAccumulator accumulator(isolate, KeyCollectionMode::kOwnOnly,
-                                 ALL_PROPERTIES);
-      accumulator.CollectOwnElementIndices(object,
-                                           Handle<JSObject>::cast(current));
-      Handle<FixedArray> keys =
-          accumulator.GetKeys(GetKeysConversion::kKeepNumbers);
-
-#ifdef DEBUG
-      // Check that keys are sorted.
-      for (int i = 1; i < keys->length(); ++i) {
-        uint32_t a = NumberToUint32(keys->get(i - 1));
-        uint32_t b = NumberToUint32(keys->get(i));
-
-        DCHECK_LE(a, b);
-      }
-#endif
+      Handle<FixedArray> keys = JSReceiver::GetOwnElementIndices(
+          isolate, object, Handle<JSObject>::cast(current));
 
       uint32_t num_indices = keys->length();
       for (uint32_t i = 0; i < num_indices; ++i) {
@@ -424,13 +348,46 @@ RUNTIME_FUNCTION(Runtime_CopyFromPrototype) {
         // interested in the range [0, length).
         if (idx >= length) break;
 
-        Maybe<bool> did_copy = ConditionalCopy(isolate, current, object, idx);
-        MAYBE_RETURN(did_copy, isolate->heap()->exception());
-        if (did_copy.FromJust() && idx >= max) max = idx + 1;
+        MAYBE_RETURN_NULL(ConditionalCopy(isolate, current, object, idx));
       }
     }
   }
-  return *isolate->factory()->NewNumberFromUint(max);
+  return isolate->factory()->undefined_value();
+}
+
+}  // namespace
+
+RUNTIME_FUNCTION(Runtime_PrepareElementsForSort) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(2, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, object, 0);
+  CONVERT_NUMBER_CHECKED(uint32_t, length, Uint32, args[1]);
+
+  if (isolate->debug_execution_mode() == DebugInfo::kSideEffects) {
+    if (!isolate->debug()->PerformSideEffectCheckForObject(object)) {
+      return isolate->heap()->exception();
+    }
+  }
+
+  // Counter for sorting arrays that have non-packed elements and where either
+  // the ElementsProtector is invalid or the prototype does not match
+  // Array.prototype.
+  if (object->IsJSArray() &&
+      !Handle<JSArray>::cast(object)->HasFastPackedElements()) {
+    JSObject* initial_array_proto = JSObject::cast(
+        isolate->native_context()->get(Context::INITIAL_ARRAY_PROTOTYPE_INDEX));
+    if (!isolate->IsNoElementsProtectorIntact() ||
+        object->map()->prototype() != initial_array_proto) {
+      isolate->CountUsage(
+          v8::Isolate::kArrayPrototypeSortJSArrayModifiedPrototype);
+    }
+  }
+
+  if (!object->IsJSArray()) {
+    RETURN_FAILURE_ON_EXCEPTION(isolate,
+                                CopyFromPrototype(isolate, object, length));
+  }
+  return RemoveArrayHoles(isolate, object, length);
 }
 
 // Move contents of argument 0 (an array) to argument 1 (an array)

src/runtime/runtime.h

@@ -49,8 +49,7 @@ namespace internal {
   F(MoveArrayContents, 2, 1)        \
   F(NewArray, -1 /* >= 3 */, 1)     \
   F(NormalizeElements, 1, 1)        \
-  F(RemoveArrayHoles, 2, 1)         \
-  F(CopyFromPrototype, 2, 1)        \
+  F(PrepareElementsForSort, 2, 1)   \
   F(TransitionElementsKind, 2, 1)   \
   F(TrySliceSimpleNonFastElements, 3, 1)