[builtins] Reduce resolve element closure overhead in Promise.all.

In Promise.all we used to allocate a fresh closure plus a fresh context
for each individual element, which is quite a lot of overhead, especially
since this could be shared in a single context for all elements. The only
bit of information that is needed for each resolve element closure is the
index under which to store the resulting value. With this change we move
this index to the "identity hash" field of the JSFunction, which doesn't
care about the concrete value anyways, as long as it's not zero (the "no
hash" sentinel), and share the rest of the fields in a single outer
context for all resolve element closures.

This limits the maximum number of elements for Promise.all to 2^21 for
now, but that should be fine. Shall we ever see the need for more than
this, we can add machinery to overflow to separate context for indices
larger than 2^21.

This significantly reduces the overhead due to Promise.all on the
parallel-async-es2017-native test, with execution time dropping from
around 148ms to 133ms, so overall a steady 10% improvement on this
benchmark.

Bug: v8:7253
Change-Id: I1092da771c4919f3db7129d2b0a244fc26a7b144
Reviewed-on: https://chromium-review.googlesource.com/973283Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52134}

src/builtins/builtins-promise-gen.h

@@ -30,11 +30,8 @@ class PromiseBuiltinsAssembler : public CodeStubAssembler {
 
  protected:
   enum PromiseAllResolveElementContextSlots {
-    // Index into the values array, or -1 if the callback was already called
-    kPromiseAllResolveElementIndexSlot = Context::MIN_CONTEXT_SLOTS,
-
-    // Remaining elements count (mutable HeapNumber)
-    kPromiseAllResolveElementRemainingElementsSlot,
+    // Remaining elements count
+    kPromiseAllResolveElementRemainingSlot = Context::MIN_CONTEXT_SLOTS,
 
     // Promise capability from Promise.all
     kPromiseAllResolveElementCapabilitySlot,
@@ -105,6 +102,18 @@ class PromiseBuiltinsAssembler : public CodeStubAssembler {
 
   Node* PromiseHasHandler(Node* promise);
 
+  // Creates the context used by all Promise.all resolve element closures,
+  // together with the values array. Since all closures for a single Promise.all
+  // call use the same context, we need to store the indices for the individual
+  // closures somewhere else (we put them into the identity hash field of the
+  // closures), and we also need to have a separate marker for when the closure
+  // was called already (we slap the native context onto the closure in that
+  // case to mark it's done).
+  Node* CreatePromiseAllResolveElementContext(Node* promise_capability,
+                                              Node* native_context);
+  Node* CreatePromiseAllResolveElementFunction(Node* context, Node* index,
+                                               Node* native_context);
+
   Node* CreatePromiseResolvingFunctionsContext(Node* promise, Node* debug_event,
                                                Node* native_context);
 
@@ -170,9 +179,6 @@ class PromiseBuiltinsAssembler : public CodeStubAssembler {
                           const IteratorRecord& record, Label* if_exception,
                           Variable* var_exception);
 
-  Node* IncrementSmiCell(Node* cell, Label* if_overflow = nullptr);
-  Node* DecrementSmiCell(Node* cell);
-
   void SetForwardingHandlerIfTrue(Node* context, Node* condition,
                                   const NodeGenerator& object);
   inline void SetForwardingHandlerIfTrue(Node* context, Node* condition,

src/builtins/builtins.cc

@@ -496,7 +496,6 @@ bool Builtins::IsIsolateIndependent(int index) {
     case kOrdinaryHasInstance:
     case kOrdinaryToPrimitive_Number:
     case kOrdinaryToPrimitive_String:
-    case kPromiseAll:
     case kPromiseCapabilityDefaultReject:
     case kPromiseCapabilityDefaultResolve:
     case kPromiseCatchFinally:

src/messages.h

@@ -695,6 +695,7 @@ class ErrorUtils : public AllStatic {
   T(TooManySpreads,                                                            \
     "Literal containing too many nested spreads (up to 65534 allowed)")        \
   T(TooManyVariables, "Too many variables declared (only 4194303 allowed)")    \
+  T(TooManyElementsInPromiseAll, "Too many elements passed to Promise.all")    \
   T(TypedArrayTooShort,                                                        \
     "Derived TypedArray constructor created an array which was too small")     \
   T(UnexpectedEOS, "Unexpected end of input")                                  \

test/mjsunit/es6/promise-all-overflow-1.js

+// Copyright 2018 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.
+
+// Flags: --allow-natives-syntax
+
+// Make sure we properly throw a RangeError when overflowing the maximum
+// number of elements for Promise.all, which is capped at 2^21 bits right
+// now, since we store the indices as identity hash on the resolve element
+// closures.
+const a = new Array(2 ** 21 - 1);
+const p = Promise.resolve(1);
+for (let i = 0; i < a.length; ++i) a[i] = p;
+testAsync(assert => {
+  assert.plan(1);
+  Promise.all(a).then(assert.unreachable, reason => {
+    assert.equals(true, reason instanceof RangeError);
+  });
+});

test/mjsunit/es6/promise-all-overflow-2.js

+// Copyright 2018 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.
+
+// Flags: --allow-natives-syntax
+
+// Test that pre-allocation of the result array works even if it needs to be
+// allocated in large object space.
+const a = new Array(64 * 1024);
+a.fill(Promise.resolve(1));
+testAsync(assert => {
+  assert.plan(1);
+  Promise.all(a).then(b => {
+    assert.equals(a.length, b.length);
+  });
+});

test/mjsunit/es6/promise-all.js

+// Copyright 2018 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.
+
+// Flags: --allow-natives-syntax
+
+// We store the index in the hash code field of the Promise.all resolve
+// element closures, so make sure we properly handle the cases where this
+// magical field turns into a PropertyArray later.
+(function() {
+  class MyPromise extends Promise {
+    then(resolve, reject) {
+      this.resolve = resolve;
+    }
+  };
+  const myPromise = new MyPromise(() => {});
+  MyPromise.all([myPromise]);
+  myPromise.resolve.x = 1;
+  myPromise.resolve(1);
+})();
+
+// Same test as above, but for PropertyDictionary.
+(function() {
+  class MyPromise extends Promise {
+    then(resolve, reject) {
+      this.resolve = resolve;
+    }
+  };
+  const myPromise = new MyPromise(() => {});
+  MyPromise.all([myPromise]);
+  for (let i = 0; i < 1025; ++i) {
+    myPromise.resolve[`x${i}`] = i;
+  }
+  myPromise.resolve(1);
+})();
+
+// Test that we return a proper array even if (custom) "then" invokes the
+// resolve callbacks right away.
+(function() {
+  class MyPromise extends Promise {
+    constructor(executor, id) {
+      super(executor);
+      this.id = id;
+    }
+
+    then(resolve, reject) {
+      if (this.id) return resolve(this.id);
+      return super.then(resolve, reject)
+    }
+  };
+  const a = new MyPromise(() => {}, 'a');
+  const b = new MyPromise(() => {}, 'b');
+  testAsync(assert => {
+    assert.plan(1);
+    MyPromise.all([a, b]).then(
+        v => assert.equals(['a', 'b'], v),
+        assert.unexpectedRejection());
+  });
+})();
+
+// Test that we properly handle holes introduced into the resulting array
+// by resolving some late elements immediately.
+(function() {
+  class MyPromise extends Promise {
+    then(resolve, reject) {
+      if (this.immediately) {
+        resolve(42);
+      } else {
+        super.then(resolve, reject);
+      }
+    }
+  };
+  const a = new Array(1024);
+  a.fill(MyPromise.resolve(1));
+  const p = MyPromise.resolve(0);
+  p.immediately = true;
+  a.push(p);
+  testAsync(assert => {
+    assert.plan(1);
+    MyPromise.all(a).then(
+        b => assert.equals(42, b[1024]),
+        assert.unexpectedRejection());
+  });
+})();

test/mjsunit/mjsunit.status

@@ -105,6 +105,7 @@
   'migrations': [SKIP],
   'array-functions-prototype-misc': [PASS, SLOW, ['mode == debug', SKIP]],
   'compiler/regress-808472': [PASS, ['mode == debug', SKIP]],
+  'es6/promise-all-overflow-*': [PASS, SLOW, ['mode == debug', SKIP]],
 
   ##############################################################################
   # This test sets the umask on a per-process basis and hence cannot be