Array.prototype.reverse should call [[HasProperty]] on elements before [[Get]]

This is a change from ES5 to ES6: When reversing an array, first it is checked
whether the element exists, before the element is looked up. The order in ES6
is

[[HasElement]] lower
[[Get]] lower (if present)
[[HasElement]] upper
[[Get]] upper (if present)

In ES5, on the other hand, the order was

[[Get]] lower
[[Get]] upper
[[HasElement]] lower
[[HasElement]] upper

To mitigate the performance impact, this patch implements a new, third copy
of reversing arrays if %_HasPackedElements. This allows us to skip all
membership tests, and a quick and dirty benchmark shows that the new version
is faster:

Over 4 runs, the slowest for the new version:
d8> var start = Date.now(); for (var i = 0; i < 100000000; i++) [1, 2, 3, 4, 5].reverse(); Date.now() - start
4658

Over 3 runs, the fastest for the old version:
d8> var start = Date.now(); for (var i = 0; i < 100000000; i++) [1, 2, 3, 4, 5].reverse(); Date.now() - start
5176

BUG=v8:4223
R=adamk
LOG=Y

Review URL: https://codereview.chromium.org/1238593003

Cr-Commit-Position: refs/heads/master@{#29716}

diff --git a/src/array.js b/src/array.js
index 7baabf83610c3c333eb13b9a5463c652ccaa3839..e0cc0695cc600462546ab55f813f3e3b3a2c87f3 100644 (file)
--- a/src/array.js
+++ b/src/array.js
@@ -587,14 +587,25 @@ function SparseReverse(array, len) {
   }
 }
 
-
-function InnerArrayReverse(array, len) {
+function PackedArrayReverse(array, len) {
   var j = len - 1;
   for (var i = 0; i < j; i++, j--) {
     var current_i = array[i];
-    if (!IS_UNDEFINED(current_i) || i in array) {
-      var current_j = array[j];
-      if (!IS_UNDEFINED(current_j) || j in array) {
+    var current_j = array[j];
+    array[i] = current_j;
+    array[j] = current_i;
+  }
+  return array;
+}
+
+
+function GenericArrayReverse(array, len) {
+  var j = len - 1;
+  for (var i = 0; i < j; i++, j--) {
+    if (i in array) {
+      var current_i = array[i];
+      if (j in array) {
+        var current_j = array[j];
         array[i] = current_j;
         array[j] = current_i;
       } else {
@@ -602,8 +613,8 @@ function InnerArrayReverse(array, len) {
         delete array[i];
       }
     } else {
-      var current_j = array[j];
-      if (!IS_UNDEFINED(current_j) || j in array) {
+      if (j in array) {
+        var current_j = array[j];
         array[i] = current_j;
         delete array[j];
       }
@@ -618,14 +629,17 @@ function ArrayReverse() {
 
   var array = TO_OBJECT_INLINE(this);
   var len = TO_UINT32(array.length);
+  var isArray = IS_ARRAY(array);
 
-  if (UseSparseVariant(array, len, IS_ARRAY(array), len)) {
+  if (UseSparseVariant(array, len, isArray, len)) {
     %NormalizeElements(array);
     SparseReverse(array, len);
     return array;
+  } else if (isArray && %_HasFastPackedElements(array)) {
+    return PackedArrayReverse(array, len);
+  } else {
+    return GenericArrayReverse(array, len);
   }
-
-  return InnerArrayReverse(array, len);
 }
 
 
@@ -1688,10 +1702,10 @@ utils.Export(function(to) {
   to.InnerArrayMap = InnerArrayMap;
   to.InnerArrayReduce = InnerArrayReduce;
   to.InnerArrayReduceRight = InnerArrayReduceRight;
-  to.InnerArrayReverse = InnerArrayReverse;
   to.InnerArraySome = InnerArraySome;
   to.InnerArraySort = InnerArraySort;
   to.InnerArrayToLocaleString = InnerArrayToLocaleString;
+  to.PackedArrayReverse = PackedArrayReverse;
 });
 
 $arrayConcat = ArrayConcatJS;

diff --git a/src/harmony-typedarray.js b/src/harmony-typedarray.js
index b9cc798ad2041b9aa9ab5a0a00d8096974f71952..8518b8571bb360dd267ee165f2fe910d8c700aff 100644 (file)
--- a/src/harmony-typedarray.js
+++ b/src/harmony-typedarray.js
@@ -44,13 +44,13 @@ var InnerArrayIndexOf;
 var InnerArrayJoin;
 var InnerArrayLastIndexOf;
 var InnerArrayMap;
-var InnerArrayReverse;
 var InnerArraySome;
 var InnerArraySort;
 var InnerArrayToLocaleString;
 var IsNaN;
 var MathMax;
 var MathMin;
+var PackedArrayReverse;
 
 utils.Import(function(from) {
   ArrayFrom = from.ArrayFrom;
@@ -68,13 +68,13 @@ utils.Import(function(from) {
   InnerArrayMap = from.InnerArrayMap;
   InnerArrayReduce = from.InnerArrayReduce;
   InnerArrayReduceRight = from.InnerArrayReduceRight;
-  InnerArrayReverse = from.InnerArrayReverse;
   InnerArraySome = from.InnerArraySome;
   InnerArraySort = from.InnerArraySort;
   InnerArrayToLocaleString = from.InnerArrayToLocaleString;
   IsNaN = from.IsNaN;
   MathMax = from.MathMax;
   MathMin = from.MathMin;
+  PackedArrayReverse = from.PackedArrayReverse;
 });
 
 // -------------------------------------------------------------------
@@ -179,7 +179,7 @@ function TypedArrayReverse() {
 
   var length = %_TypedArrayGetLength(this);
 
-  return InnerArrayReverse(this, length);
+  return PackedArrayReverse(this, length);
 }
 
 

diff --git a/test/mjsunit/es6/array-reverse-order.js b/test/mjsunit/es6/array-reverse-order.js
new file mode 100644 (file)
index 0000000..590491c
--- /dev/null
+++ b/test/mjsunit/es6/array-reverse-order.js
@@ -0,0 +1,10 @@
+// Copyright 2015 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.
+
+// ES6 specifically says that elements should be checked with [[HasElement]] before
+// [[Get]]. This is observable in case a getter deletes elements. ES5 put the
+// [[HasElement]] after the [[Get]].
+
+assertTrue(1 in Array.prototype.reverse.call(
+    {length:2, get 0(){delete this[0];}, 1: "b"}))