[Builtins] Array.prototype.forEach perf regression on dictionaries.

An unnecessary call to ToString() on the array index caused trips to
the runtime. The fix also includes performance micro-benchmarks so
we'll have a harder time regressing this case in future.

Bug: v8:8112
Change-Id: Iada5bd2e3c6d2246fb1225e7094f3d9c66ddafbd
Reviewed-on: https://chromium-review.googlesource.com/1206355
Commit-Queue: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55653}

src/builtins/array-foreach.tq

@@ -10,15 +10,16 @@ module array {
     // 6. Repeat, while k < len
     for (let k: Smi = initial_k; k < len; k = k + 1) {
       // 6a. Let Pk be ! ToString(k).
-      const pK: String = ToString_Inline(context, k);
+      // k is guaranteed to be a positive integer, hence ToString is
+      // side-effect free and HasProperty/GetProperty do the conversion inline.
 
       // 6b. Let kPresent be ? HasProperty(O, Pk).
-      const kPresent: Boolean = HasProperty(context, o, pK);
+      const kPresent: Boolean = HasProperty_Inline(context, o, k);
 
       // 6c. If kPresent is true, then
       if (kPresent == True) {
         // 6c. i. Let kValue be ? Get(O, Pk).
-        const kValue: Object = GetProperty(context, o, pK);
+        const kValue: Object = GetProperty(context, o, k);
 
         // 6c. ii. Perform ? Call(callbackfn, T, <kValue, k, O>).
         Call(context, callbackfn, thisArg, kValue, k, o);

src/builtins/base.tq

@@ -191,6 +191,7 @@ extern macro GetProperty(Context, Object, Object): Object;
 extern builtin SetProperty(Context, Object, Object, Object);
 extern builtin DeleteProperty(Context, Object, Object, LanguageMode);
 extern builtin HasProperty(Context, JSReceiver, Object): Boolean;
+extern macro HasProperty_Inline(Context, JSReceiver, Object): Boolean;
 
 extern macro ThrowRangeError(Context, constexpr MessageTemplate): never;
 extern macro ThrowTypeError(Context, constexpr MessageTemplate): never;

src/code-stub-assembler.h

@@ -2738,6 +2738,13 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
                              SloppyTNode<Object> key,
                              HasPropertyLookupMode mode);
 
+  // Due to naming conflict with the builtin function namespace.
+  TNode<Oddball> HasProperty_Inline(TNode<Context> context,
+                                    TNode<Object> object, TNode<Object> key) {
+    return HasProperty(context, object, key,
+                       HasPropertyLookupMode::kHasProperty);
+  }
+
   Node* Typeof(Node* value);
 
   TNode<Object> GetSuperConstructor(SloppyTNode<Context> context,

test/js-perf-test/Array/every.js

@@ -30,6 +30,7 @@ DefineHigherOrderTests([
   ['DoubleEvery', newClosure('every'), DoubleSetup, v => v > 0.0],
   ['SmiEvery', newClosure('every'), SmiSetup, v => v != 34343],
   ['FastEvery', newClosure('every'), FastSetup, v => v !== 'hi'],
+  ['DictionaryEvery', newClosure('every'), DictionarySetup, v => v != 34343],
   ['OptFastEvery', OptFastEvery, FastSetup, v => true],
   ['OptUnreliableEvery', OptUnreliableEvery, FastSetup, v => true]
 ]);

test/js-perf-test/Array/filter.js

@@ -61,6 +61,10 @@ DefineHigherOrderTests([
   ],
   ['SmiFilter', newClosure('filter'), SmiSetup, v => v % 2 === 0],
   ['FastFilter', newClosure('filter'), FastSetup, (_, i) => i % 2 === 0],
+  [
+    'DictionaryFilter', newClosure('filter'), DictionarySetup,
+     v => true
+  ],
   [
     'GenericFilter', newClosure('filter', true), ObjectSetup,
     (_, i) => i % 2 === 0

test/js-perf-test/Array/for-each.js

@@ -60,6 +60,10 @@ DefineHigherOrderTests([
     'FastForEach', newClosure('forEach'), FastSetup,
     v => v === `value ${max_index}`
   ],
+  [
+    'DictionaryForEach', newClosure('forEach'), DictionarySetup,
+     v => v === max_index
+  ],
   [
     'GenericForEach', newClosure('forEach', true), ObjectSetup,
     v => v === max_index

test/js-perf-test/Array/map.js

@@ -55,6 +55,7 @@ DefineHigherOrderTests([
   ['FastMap', newClosure('map'), FastSetup, v => v],
   ['SmallSmiToDoubleMap', newClosure('map'), SmiSetup, v => v + 0.5],
   ['SmallSmiToFastMap', newClosure('map'), SmiSetup, v => 'hi' + v],
+  ['DictionaryMap', newClosure('map'), DictionarySetup, v => v],
   ['GenericMap', newClosure('map', true), ObjectSetup, v => v],
   ['OptFastMap', OptFastMap, FastSetup, undefined],
   ['OptUnreliableMap', OptUnreliableMap, FastSetup, v => v]

test/js-perf-test/Array/run.js

@@ -67,10 +67,12 @@ function HoleyFastSetup() {
 function DictionarySetup() {
   array = [];
   // Add a large index to force dictionary elements.
-  array[2**30] = 10;
-  // Spread out {array_size} elements.
-  for (var i = 0; i < array_size-1; i++) {
-    array[i*101] = i;
+  array[1000000] = 0;
+  var len = array.length;
+  for (var i = 0; i < len; i++) {
+    if (i % 100 === 0) {
+      array[i] = i;
+    }
   }
   assert(%HasDictionaryElements(array));
 }

test/js-perf-test/Array/some.js

@@ -30,6 +30,7 @@ DefineHigherOrderTests([
   ['DoubleSome', newClosure('some'), DoubleSetup, v => v < 0.0],
   ['SmiSome', newClosure('some'), SmiSetup, v => v === 34343],
   ['FastSome', newClosure('some'), FastSetup, v => v === 'hi'],
+  ['DictionarySome', newClosure('some'), DictionarySetup, v => v > 0],
   ['OptFastSome', OptFastSome, FastSetup, undefined],
   ['OptUnreliableSome', OptUnreliableSome, FastSetup, v => v === 'hi']
 ]);

test/js-perf-test/JSTests.json

@@ -695,6 +695,7 @@
         {"name": "DoubleForEach"},
         {"name": "SmiForEach"},
         {"name": "FastForEach"},
+        {"name": "DictionaryForEach"},
         {"name": "GenericForEach"},
         {"name": "OptFastForEach"},
         {"name": "OptUnreliableForEach"},
@@ -702,6 +703,7 @@
         {"name": "DoubleFilter"},
         {"name": "SmiFilter"},
         {"name": "FastFilter"},
+        {"name": "DictionaryFilter"},
         {"name": "GenericFilter"},
         {"name": "OptFastFilter"},
         {"name": "OptUnreliableFilter"},
@@ -709,12 +711,14 @@
         {"name": "DoubleMap"},
         {"name": "SmiMap"},
         {"name": "FastMap"},
+        {"name": "DictionaryMap"},
         {"name": "GenericMap"},
         {"name": "OptFastMap"},
         {"name": "OptUnreliableMap"},
         {"name": "DoubleEvery"},
         {"name": "SmiEvery"},
         {"name": "FastEvery"},
+        {"name": "DictionaryEvery"},
         {"name": "OptFastEvery"},
         {"name": "OptUnreliableEvery"},
         {"name": "SmiJoin"},
@@ -724,6 +728,7 @@
         {"name": "DoubleSome"},
         {"name": "SmiSome"},
         {"name": "FastSome"},
+        {"name": "DictionarySome"},
         {"name": "OptFastSome"},
         {"name": "OptUnreliableSome"},
         {"name": "DoubleReduce"},