[Promise.all] Use FixedArray for "values"

There's no need for it to be a JSArray. In the spec it's a List
which roughly corresponds to FixedArray (but not JSArray).

Gotchas:
- There's no good point in time where we know the array length, since
resolving might be interleaved with iteration.
- Using ExtractFixedArray in places where we don't need to extract,
since it takes care of things like allocating the resulting array
in the right space etc.

Drive-by fix: Previously we pre-allocated an array 1 elements too big,
but never noticed it since the last element was the hole.

Bug: v8:10506
Change-Id: I6a72fbf1fc0cc031f2c8bad9314c4ed21d544a0f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2202905Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67919}

src/builtins/builtins-promise.h

@@ -35,7 +35,7 @@ class PromiseBuiltins {
     kPromiseAllResolveElementCapabilitySlot,
 
     // Values array from Promise.all
-    kPromiseAllResolveElementValuesArraySlot,
+    kPromiseAllResolveElementValuesSlot,
 
     kPromiseAllResolveElementLength
   };

src/builtins/promise-all-element-closure.tq

@@ -66,7 +66,7 @@ extern enum PromiseAllResolveElementContextSlots extends int31
 constexpr 'PromiseBuiltins::PromiseAllResolveElementContextSlots' {
   kPromiseAllResolveElementRemainingSlot,
   kPromiseAllResolveElementCapabilitySlot,
-  kPromiseAllResolveElementValuesArraySlot,
+  kPromiseAllResolveElementValuesSlot,
   kPromiseAllResolveElementLength
 }
 extern operator '[]=' macro StoreContextElement(
@@ -109,45 +109,22 @@ transitioning macro PromiseAllResolveElementClosure<F: type>(
   assert(identityHash > 0);
   const index = identityHash - 1;
 
-  // Check if we need to grow the [[ValuesArray]] to store {value} at {index}.
-  const valuesArray = UnsafeCast<JSArray>(
-      context[PromiseAllResolveElementContextSlots::
-                  kPromiseAllResolveElementValuesArraySlot]);
-  const elements = UnsafeCast<FixedArray>(valuesArray.elements);
-  const valuesLength = Convert<intptr>(valuesArray.length);
-  if (index < valuesLength) {
-    // The {index} is in bounds of the {values_array},
-    // just store the {value} and continue.
-    elements.objects[index] = updatedValue;
-  } else {
-    // Check if we need to grow the backing store.
-    const newLength = index + 1;
-    const elementsLength = elements.length_intptr;
-    if (index < elementsLength) {
-      // The {index} is within bounds of the {elements} backing store, so
-      // just store the {value} and update the "length" of the {values_array}.
-      valuesArray.length = Convert<Smi>(newLength);
-      elements.objects[index] = updatedValue;
-    } else
-      deferred {
-        // We need to grow the backing store to fit the {index} as well.
-        const newElementsLength = IntPtrMin(
-            CalculateNewElementsCapacity(newLength),
-            kPropertyArrayHashFieldMax + 1);
-        assert(index < newElementsLength);
-        assert(elementsLength < newElementsLength);
-        const newElements =
-            ExtractFixedArray(elements, 0, elementsLength, newElementsLength);
-        newElements.objects[index] = updatedValue;
-
-        // Update backing store and "length" on {values_array}.
-        valuesArray.elements = newElements;
-        valuesArray.length = Convert<Smi>(newLength);
-      }
-  }
   let remainingElementsCount =
       UnsafeCast<Smi>(context[PromiseAllResolveElementContextSlots::
                                   kPromiseAllResolveElementRemainingSlot]);
+
+  let values =
+      UnsafeCast<FixedArray>(context[PromiseAllResolveElementContextSlots::
+                                         kPromiseAllResolveElementValuesSlot]);
+  const newCapacity = index + 1;
+  if (newCapacity > values.length_intptr) deferred {
+      // This happens only when the promises are resolved during iteration.
+      values = ExtractFixedArray(values, 0, values.length_intptr, newCapacity);
+      context[PromiseAllResolveElementContextSlots::
+                  kPromiseAllResolveElementValuesSlot] = values;
+    }
+  values.objects[index] = updatedValue;
+
   remainingElementsCount = remainingElementsCount - 1;
   context[PromiseAllResolveElementContextSlots::
               kPromiseAllResolveElementRemainingSlot] = remainingElementsCount;
@@ -156,6 +133,9 @@ transitioning macro PromiseAllResolveElementClosure<F: type>(
         context[PromiseAllResolveElementContextSlots::
                     kPromiseAllResolveElementCapabilitySlot]);
     const resolve = UnsafeCast<JSAny>(capability.resolve);
+    const arrayMap = UnsafeCast<Map>(
+        nativeContext[NativeContextSlot::JS_ARRAY_PACKED_ELEMENTS_MAP_INDEX]);
+    const valuesArray = NewJSArray(arrayMap, values);
     Call(context, resolve, Undefined, valuesArray);
   }
   return Undefined;

src/builtins/promise-all.tq

@@ -18,12 +18,6 @@ const kPromiseBuiltinsPromiseContextLength: constexpr int31
 // case to mark it's done).
 macro CreatePromiseAllResolveElementContext(implicit context: Context)(
     capability: PromiseCapability, nativeContext: NativeContext): Context {
-  // TODO(bmeurer): Manually fold this into a single allocation.
-  const arrayMap = UnsafeCast<Map>(
-      nativeContext[NativeContextSlot::JS_ARRAY_PACKED_ELEMENTS_MAP_INDEX]);
-  const valuesArray = AllocateJSArray(
-      ElementsKind::PACKED_ELEMENTS, arrayMap, IntPtrConstant(0),
-      SmiConstant(0));
   const resolveContext = AllocateSyntheticFunctionContext(
       nativeContext,
       PromiseAllResolveElementContextSlots::kPromiseAllResolveElementLength);
@@ -32,7 +26,7 @@ macro CreatePromiseAllResolveElementContext(implicit context: Context)(
   resolveContext[PromiseAllResolveElementContextSlots::
                      kPromiseAllResolveElementCapabilitySlot] = capability;
   resolveContext[PromiseAllResolveElementContextSlots::
-                     kPromiseAllResolveElementValuesArraySlot] = valuesArray;
+                     kPromiseAllResolveElementValuesSlot] = kEmptyFixedArray;
   return resolveContext;
 }
 
@@ -283,30 +277,36 @@ Reject(Object) {
                             kPromiseAllResolveElementRemainingSlot] =
       remainingElementsCount;
   if (remainingElementsCount > 0) {
-    // Pre-allocate the backing store for the {values_array} to the desired
-    // capacity here. We may already have elements here in case of some
-    // fancy Thenable that calls the resolve callback immediately, so we need
-    // to handle that correctly here.
-    const valuesArray = UnsafeCast<JSArray>(
+    // Pre-allocate the backing store for the {values} to the desired
+    // capacity. We may already have elements in "values" - this happens
+    // when the Thenable calls the resolve callback immediately.
+    let values = UnsafeCast<FixedArray>(
         resolveElementContext[PromiseAllResolveElementContextSlots::
-                                  kPromiseAllResolveElementValuesArraySlot]);
-    const oldElements = UnsafeCast<FixedArray>(valuesArray.elements);
-    const oldCapacity = oldElements.length_intptr;
-    const newCapacity = SmiUntag(index);
+                                  kPromiseAllResolveElementValuesSlot]);
+    // 'index' is a 1-based index and incremented after every Promise. Later we
+    // use 'values' as a 0-based array, so capacity 'index - 1' is enough.
+    const newCapacity = SmiUntag(index) - 1;
+
+    const oldCapacity = values.length_intptr;
     if (oldCapacity < newCapacity) {
-      valuesArray.elements =
-          ExtractFixedArray(oldElements, 0, oldCapacity, newCapacity);
+      values = ExtractFixedArray(values, 0, oldCapacity, newCapacity);
+      resolveElementContext[PromiseAllResolveElementContextSlots::
+                                kPromiseAllResolveElementValuesSlot] = values;
     }
   } else
     deferred {
+      assert(remainingElementsCount == 0);
       // If remainingElementsCount.[[Value]] is 0, then
       //     Let valuesArray be CreateArrayFromList(values).
       //     Perform ? Call(resultCapability.[[Resolve]], undefined,
       //                    « valuesArray »).
-      assert(remainingElementsCount == 0);
-      const valuesArray = UnsafeCast<JSAny>(
+
+      const values = UnsafeCast<FixedArray>(
           resolveElementContext[PromiseAllResolveElementContextSlots::
-                                    kPromiseAllResolveElementValuesArraySlot]);
+                                    kPromiseAllResolveElementValuesSlot]);
+      const arrayMap = UnsafeCast<Map>(
+          nativeContext[NativeContextSlot::JS_ARRAY_PACKED_ELEMENTS_MAP_INDEX]);
+      const valuesArray = NewJSArray(arrayMap, values);
       Call(nativeContext, UnsafeCast<JSAny>(resolve), Undefined, valuesArray);
     }