Hack forEach to use HasFastPackedElements

src/array.js

Index: src/array.js
diff --git a/src/array.js b/src/array.js
index 5f89ebb7a6b6c4cfea2b66df051e918e95a74c0d..a01efd0c6c57d57aba59afed5b9105bbb1179915 100644
--- a/src/array.js
+++ b/src/array.js
@@ -1227,6 +1227,100 @@ function ArrayForEach(f, receiver) {
 }
 
 
+function ArrayForEach2(f, receiver) {
+  if (IS_NULL_OR_UNDEFINED(this) && !IS_UNDETECTABLE(this)) {
+    throw MakeTypeError("called_on_null_or_undefined",
+                        ["Array.prototype.forEach"]);
+  }
+
+  // Pull out the length so that modifications to the length in the
+  // loop will not affect the looping and side effects are visible.
+  var array = ToObject(this);
+  var length = TO_UINT32(array.length);
+
+  if (!IS_SPEC_FUNCTION(f)) {
+    throw MakeTypeError('called_non_callable', [ f ]);
+  }
+  if (IS_NULL_OR_UNDEFINED(receiver)) {
+    receiver = %GetDefaultReceiver(f) || receiver;
+  } else if (!IS_SPEC_OBJECT(receiver) && %IsClassicModeFunction(f)) {
+    receiver = ToObject(receiver);
+  }
+
+  if (%DebugCallbackSupportsStepping(f)) {
+    for (var i = 0; i < length; i++) {
+      if (i in array) {
+        var element = array[i];
+        // Prepare break slots for debugger step in.
+        %DebugPrepareStepInIfStepping(f);
+        %_CallFunction(receiver, element, i, array, f);
+      }
+    }
+  } else {
+    // This is mostly a duplicate of the previous loop sans debug stepping.
+    var isProxy = %IsJSProxy(array);
+    for (var i = 0; i < length; i++) {
+      if (!isProxy && %HasFastPackedElements(array) || i in array) {

[Michael Starzinger, 2013/08/13 14:10:22]

It is _really_ surprising that the runtime call into
Runtime_HasFastPackedElements is cheaper than the "in" keyword. This suggests
that there is a lot of potential in actually improving "in" in general. This
needs investigation of the generated code!

[Michael Starzinger, 2013/08/13 15:47:16]

Now that I think about it, I don't actually think that this is a valid
optimization either. What happens if the callback actually changes the array in
a way so that it remains in fast-packed mode, but "i" still runs out of bounds.
Imagine the following callback.

var array = [1,2,3,4,5,6];
Object.defineProperty(array, '1', {
  get: function () { array.length = 3; },
  configurable: true
});
array.forEach(f);

[Toon Verwaest, 2013/08/13 20:47:42]

I don't think you can have indexed accessors while being packed. I think we
always go dictionary mode when accessors are installed. Holey arrays would have
this problem; but they are already excluded explicitly.

On 2013/08/13 15:47:16, Michael Starzinger wrote:

[Michael Starzinger, 2013/08/14 07:48:47]

OK, admittedly, my example does not trigger the problem and is unnecessarily
complex. But you don't need accessors to reproduce this problem ...

var array = [1,2,3,4,5,6];
function f(value, index, array) {
  array.length = 3;
}
array.forEach(f);

+        var element = array[i];
+        %_CallFunction(receiver, element, i, array, f);
+      }
+    }
+    // End of duplicate.
+  }
+}
+
+
+function ArrayForEach3(f, receiver) {
+  if (IS_NULL_OR_UNDEFINED(this) && !IS_UNDETECTABLE(this)) {
+    throw MakeTypeError("called_on_null_or_undefined",
+                        ["Array.prototype.forEach"]);
+  }
+
+  // Pull out the length so that modifications to the length in the
+  // loop will not affect the looping and side effects are visible.
+  var array = ToObject(this);
+  var length = TO_UINT32(array.length);
+
+  if (!IS_SPEC_FUNCTION(f)) {
+    throw MakeTypeError('called_non_callable', [ f ]);
+  }
+  if (IS_NULL_OR_UNDEFINED(receiver)) {
+    receiver = %GetDefaultReceiver(f) || receiver;
+  } else if (!IS_SPEC_OBJECT(receiver) && %IsClassicModeFunction(f)) {
+    receiver = ToObject(receiver);
+  }
+
+  if (%DebugCallbackSupportsStepping(f)) {
+    for (var i = 0; i < length; i++) {
+      if (i in array) {
+        var element = array[i];
+        // Prepare break slots for debugger step in.
+        %DebugPrepareStepInIfStepping(f);
+        %_CallFunction(receiver, element, i, array, f);
+      }
+    }
+  } else if (%IsJSProxy(array)) {
+    // This is a duplicate of the previous loop sans debug stepping.
+    for (var i = 0; i < length; i++) {
+      if (i in array) {
+        var element = array[i];
+        %_CallFunction(receiver, element, i, array, f);
+      }
+    }
+  } else {
+    // This is mostly a duplicate of the previous loop but since the order is
+    // not observable we can optimize further.
+    for (var i = 0; i < length; i++) {
+      var element = array[i];
+      if (!IS_UNDEFINED(element) || i in array) {

[Michael Starzinger, 2013/08/13 14:10:22]

This is actually wrong as the i-th element might have a getter that removes the
element upon first access but still returns undefined. This will cause us to do
the "in" check on the property after the element has been removed.

We had this "optimization" for a while and it was removed. The test262 suite
should actually cover this case.

[Michael Starzinger, 2013/08/13 14:18:58]

This is the issue tracking exactly this:

https://code.google.com/p/v8/issues/detail?id=1956

+        %_CallFunction(receiver, element, i, array, f);
+      }
+    }
+    // End of duplicate.
+  }
+}
+
+
 // Executes the function once for each element present in the
 // array until it finds one where callback returns true.
 function ArraySome(f, receiver) {
@@ -1555,7 +1649,6 @@ function ArrayReduceRight(callback, current) {
   }
 
   var receiver = %GetDefaultReceiver(callback);
-
   if (%DebugCallbackSupportsStepping(callback)) {
     for (; i >= 0; i--) {
       if (i in array) {
@@ -1632,6 +1725,8 @@ function SetUpArray() {
     "sort", getFunction("sort", ArraySort),
     "filter", getFunction("filter", ArrayFilter, 1),
     "forEach", getFunction("forEach", ArrayForEach, 1),
+    "forEach2", getFunction("forEach2", ArrayForEach2, 1),
+    "forEach3", getFunction("forEach3", ArrayForEach3, 1),
     "some", getFunction("some", ArraySome, 1),
     "every", getFunction("every", ArrayEvery, 1),
     "map", getFunction("map", ArrayMap, 1),