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

ripsawridge · Commit Bot · commit e1163c14f7e4 · 2018-09-07T15:49:20.000Z
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. TBR=tebbi@chromium.org Bug: v8:8112 Change-Id: I781e8b1bbe2eb56db961cf33b0dca8523868b83d Reviewed-on: https://chromium-review.googlesource.com/1213207 Commit-Queue: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Cr-Commit-Position: refs/heads/master@{#55733}
diff --git a/src/builtins/array-foreach.tq b/src/builtins/array-foreach.tq
@@ -5,20 +5,21 @@
 module array {
   macro ArrayForEachTorqueContinuation(
       context: Context, o: JSReceiver, len: Number, callbackfn: Callable,
-      thisArg: Object, initial_k: Smi): Object {
+      thisArg: Object, initial_k: Number): Object {
     // 5. Let k be 0.
     // 6. Repeat, while k < len
-    for (let k: Smi = initial_k; k < len; k = k + 1) {
+    for (let k: Number = 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);
@@ -60,7 +61,7 @@ module array {
     try {
       const callbackfn: Callable =
           cast<Callable>(callback) otherwise Unexpected;
-      const k: Smi = cast<Smi>(initialK) otherwise Unexpected;
+      const k: Number = cast<Number>(initialK) otherwise Unexpected;
       const number_length: Number = cast<Number>(length) otherwise Unexpected;
 
       return ArrayForEachTorqueContinuation(
@@ -159,7 +160,7 @@ module array {
       const thisArg: Object = arguments.length > 1 ? arguments[1] : Undefined;
 
       // Special cases.
-      let k: Smi = 0;
+      let k: Number = 0;
       try {
         return FastArrayForEach(context, o, len, callbackfn, thisArg)
         otherwise Bailout;
diff --git a/src/builtins/base.tq b/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;
diff --git a/src/code-stub-assembler.h b/src/code-stub-assembler.h
@@ -2739,6 +2739,14 @@ 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<JSReceiver> object,
+                                    TNode<Object> key) {
+    return HasProperty(context, object, key,
+                       HasPropertyLookupMode::kHasProperty);
+  }
+
   Node* Typeof(Node* value);
 
   TNode<Object> GetSuperConstructor(SloppyTNode<Context> context,
