Port all Promise code to C++

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: till, Assigned: till)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
The new Promise implementation started out being almost entirely in JS. Then complications in the form of xray wrappers and debugging entered the picture, requiring parts to be ported to C++, and additional supporting C++ code to be introduced. The result is somewhat of a messy intertwined mixture, which makes it hard to follow what's going on and makes things slower than they ought to be.

Because of that, and to enable further optimizations down the line, I ported the remaining JS parts to C++. While the amount of code didn't increase all that much, by about 250 lines, it's C++ instead of JS, so the complexity did increase, I'm afraid. I do think the result is still easier to follow, though.

The one optimization that this has over the current implementation is that NewPromiseCapability omits the creation of resolve/reject functions in cases where we can be sure they don't escape to content code. This alone improves performance of Promise#then by ~40%.
Comment hidden (mozreview-request)
Comment on attachment 8804639 [details]
Bug 1313049 - Port Promise-related functions from self-hosted JS to C++. , f=bz

The UncheckedUnwrap() calls look fine, modulo the comment fixups we talked about.
Attachment #8804639 - Flags: feedback+

Comment 3

2 years ago
mozreview-review
Comment on attachment 8804639 [details]
Bug 1313049 - Port Promise-related functions from self-hosted JS to C++. , f=bz

https://reviewboard.mozilla.org/r/88556/#review87618

Gread :D

::: js/src/builtin/Promise.cpp:69
(Diff revision 1)
> -        promise = val.toObjectOrNull();
> +};
>  
> -        if (!GetProperty(cx, reaction, reaction, handlerName, &handler))
> -            return false;
> +enum ThenableJobDataSlots {
> +    ThenableJobDataSlot_Promise = 0,
> +    ThenableJobDataSlot_Thenable,
> +    ThenableJobDataSlotsCount,

this is pre-existing tho, here "Slot" sounds inappropriate,
as it's actually dense element, and used for setDenseElement/getDenseElement parameter.
"Index" might be better.
(of course in followup patch or followup bug)

::: js/src/builtin/Promise.cpp:139
(Diff revision 1)
> -    // Also null out the resolve/reject functions so they can be GC'd.
> -    promise->setFixedSlot(PROMISE_RESOLVE_FUNCTION_SLOT, UndefinedValue());
> +#define REACTION_FLAG_RESOLVED                  0x1
> +#define REACTION_FLAG_FULFILLED                 0x2
> +#define REACTION_FLAG_IGNORE_DEFAULT_RESOLUTION 0x4
>  
> -    // Now that everything else is done, do the things the debugger needs.
> -    // Step 7 of RejectPromise implemented in onSettled.
> +static int32_t
> +ReactionFlags(PromiseReactionRecord* reaction)

Can this be `PromiseReactionRecord::flags()` method?
(maybe in followup patch)

Same for other functions that receives `PromiseReactionRecord` and returns value from slot.
(`SetReactionTargetState` and `ReactionTargetState`)

That would match other object classes style, and it's clear what I can do with `PromiseReactionRecord` class.


maybe, PromiseAllDataHolder would also better having accessor for each slot.
either in followup patch or followup bug.

::: js/src/builtin/Promise.cpp:170
(Diff revision 1)
>  }
>  
> -// ES2016, 25.4.1.4.
> -static MOZ_MUST_USE bool
> -FulfillMaybeWrappedPromise(JSContext *cx, HandleObject promiseObj, HandleValue value_) {
> -    Rooted<PromiseObject*> promise(cx);
> +const Class PromiseReactionRecord::class_ = {
> +    "PromiseReactionRecord",
> +    JSCLASS_HAS_RESERVED_SLOTS(ReactionRecordSlots),
> +    JS_NULL_CLASS_OPS

trailing `JS_NULL_CLASS_OPS` can be omitted.

::: js/src/builtin/Promise.cpp:174
(Diff revision 1)
> -FulfillMaybeWrappedPromise(JSContext *cx, HandleObject promiseObj, HandleValue value_) {
> -    Rooted<PromiseObject*> promise(cx);
> -    RootedValue value(cx, value_);
> +    JSCLASS_HAS_RESERVED_SLOTS(ReactionRecordSlots),
> +    JS_NULL_CLASS_OPS
> +};
>  
> -    mozilla::Maybe<AutoCompartment> ac;
> -    if (!IsProxy(promiseObj)) {
> +static void
> +SetPromiseFlags(PromiseObject& promise, int32_t flag)

"Set" sounds different than what this function does,
since passing 0 doesn't clear it.
maybe "AddPromiseFlags" ?

::: js/src/builtin/Promise.cpp:383
(Diff revision 1)
> -        return false;
>  
> -    RootedFunction reject(cx, NewNativeFunction(cx, RejectPromiseFunction, 1, funName,
> -                                                 gc::AllocKind::FUNCTION_EXTENDED));
> -    if (!reject)
> +/**
> + * Tells the embedding to enqueue a Promise reaction job, based on
> + * three parameters:
> + * reaction - The reaction record.

reactionObj

::: js/src/builtin/Promise.cpp:384
(Diff revision 1)
>  
> -    RootedFunction reject(cx, NewNativeFunction(cx, RejectPromiseFunction, 1, funName,
> -                                                 gc::AllocKind::FUNCTION_EXTENDED));
> -    if (!reject)
> +/**
> + * Tells the embedding to enqueue a Promise reaction job, based on
> + * three parameters:
> + * reaction - The reaction record.
> + * handlerArg - The first and only argument to pass to the handler invoked by

handlerArg_

::: js/src/builtin/Promise.cpp:385
(Diff revision 1)
> -    RootedFunction reject(cx, NewNativeFunction(cx, RejectPromiseFunction, 1, funName,
> -                                                 gc::AllocKind::FUNCTION_EXTENDED));
> -    if (!reject)
> +/**
> + * Tells the embedding to enqueue a Promise reaction job, based on
> + * three parameters:
> + * reaction - The reaction record.
> + * handlerArg - The first and only argument to pass to the handler invoked by
> + *              the job. This is stored on the reaction record.

won't "This will be stored ..." be better?
"This is stored ..." sounds like it's already stored before calling this function, for me.

::: js/src/builtin/Promise.cpp:422
(Diff revision 1)
>  
> -    resolveVal.setObject(*resolve);
> +    RootedValue reactionVal(cx, ObjectValue(*reaction));
> -    rejectVal.setObject(*reject);
>  
> -    return true;
> -}
> +    uint32_t handlerSlot = targetState == JS::PromiseState::Fulfilled
> +                                          ? ReactionRecordSlot_OnFulfilled

"?" and ":" should be aligned at the beginning of "targetState".

also, this conditional expression appears twice in this patch.
it would be nice to have helper function.

::: js/src/builtin/Promise.cpp:517
(Diff revision 1)
> -}
> +    // instead of getting the right list of reactions, we determine the
> +    // resolution type to retrieve the right information from the
> +    // reaction records.
> +    RootedValue reactionsVal(cx, promise->getFixedSlot(PromiseSlot_ReactionsOrResult));
>  
> -/**
> +    // Step 3-5.

Steps

::: js/src/builtin/Promise.cpp:661
(Diff revision 1)
> -    }
> -    RootedValue cVal(cx, args.thisv());
> -    RootedObject C(cx, &cVal.toObject());
>  
> -    // Step 3 of Resolve.
> -    if (mode == ResolveMode && x.isObject()) {
> +    // Steps 3-4.
> +    if (!F->getExtendedSlot(0).isUndefined() || !F->getExtendedSlot(1).isUndefined()) {

Would be nice to name those 0 and 1 like other slots,
 maybe `CapabilitiesExecutorSlot_Resolve` and `CapabilitiesExecutorSlot_Reject`, or something like that, in followup.

::: js/src/builtin/Promise.cpp:746
(Diff revision 1)
> +    }
> +
> +    return true;
>  }
>  
> +// ES6, 25.4.2.1.

I don't see any difference between ES6 and ES2016 for this section, so it would be nice to align ES2016, also for the URL.

::: js/src/builtin/Promise.cpp:782
(Diff revision 1)
> +        ac.emplace(cx, reactionObj);
> -}
> +    }
>  
> -// ES2016, February 12 draft, 25.4.3.1. steps 3-11.
> -/* static */
> -PromiseObject*
> +    Rooted<PromiseReactionRecord*> reaction(cx, &reactionObj->as<PromiseReactionRecord>());
> +
> +    JS:: PromiseState targetState = ReactionTargetState(reaction);

Please remove a space after JS::

::: js/src/builtin/Promise.cpp:783
(Diff revision 1)
> -}
> +    }
>  
> -// ES2016, February 12 draft, 25.4.3.1. steps 3-11.
> -/* static */
> -PromiseObject*
> -PromiseObject::create(JSContext* cx, HandleObject executor, HandleObject proto /* = nullptr */)
> +    Rooted<PromiseReactionRecord*> reaction(cx, &reactionObj->as<PromiseReactionRecord>());
> +
> +    JS:: PromiseState targetState = ReactionTargetState(reaction);
> +    uint32_t handlerSlot = JS::PromiseState(targetState) == JS::PromiseState::Fulfilled

this cast is unnecessary
  targetState == JS::PromiseState::Fulfilled

::: js/src/builtin/Promise.cpp:789
(Diff revision 1)
> -{
> -    MOZ_ASSERT(executor->isCallable());
> +                           ? ReactionRecordSlot_OnFulfilled
> +                           : ReactionRecordSlot_OnRejected;
> +    RootedValue handlerVal(cx, reaction->getFixedSlot(handlerSlot));
> +    RootedValue argument(cx, reaction->getFixedSlot(ReactionRecordSlot_HandlerArg));
>  
> -    RootedObject usedProto(cx, proto);
> +    // Step 1 (omitted).

maybe we could move "// Step 1" comment before `Rooted<PromiseReactionRecord*> reaction(cx, &reactionObj->as<PromiseReactionRecord>());` line.

::: js/src/builtin/Promise.cpp:791
(Diff revision 1)
> +    RootedValue handlerVal(cx, reaction->getFixedSlot(handlerSlot));
> +    RootedValue argument(cx, reaction->getFixedSlot(ReactionRecordSlot_HandlerArg));
>  
> -    RootedObject usedProto(cx, proto);
> -    bool wrappedProto = false;
> -    // If the proto is wrapped, that means the current function is running
> +    // Step 1 (omitted).
> +
> +    // Steps 2-3.

Please move "Step 3" comment before `uint32_t handlerSlot = JS::PromiseState(targetState) == JS::PromiseState::Fulfilled` line above.

also "Step 2" comment before `RootedObject promiseObj(cx, reaction->getFixedSlot(ReactionRecordSlot_Promise)` line below,
with maybe trailing " (reordered)".

::: js/src/builtin/Promise.cpp:834
(Diff revision 1)
> -    // Steps 3-7.
> -    Rooted<PromiseObject*> promise(cx, CreatePromiseObjectInternal(cx, usedProto, wrappedProto));
> -    if (!promise)
> +    args.rval().setUndefined();
> +    return true;
> +}
> -        return nullptr;
>  
> -    RootedValue promiseVal(cx, ObjectValue(*promise));
> +// ES6, 25.4.2.2.

here too, ES2016, and URL for 7.0

::: js/src/builtin/Promise.cpp:905
(Diff revision 1)
> -    }
> +}
>  
> -    // Step 9.
> -    bool success;
> +/**
> + * Tells the embedding to enqueue a Promise resolve thenable job, based on
> + * three parameters:
> + * promiseToResolve - The promise to resolve, obviously.

promiseToResolve_

::: js/src/builtin/Promise.cpp:906
(Diff revision 1)
>  
> -    // Step 9.
> -    bool success;
> +/**
> + * Tells the embedding to enqueue a Promise resolve thenable job, based on
> + * three parameters:
> + * promiseToResolve - The promise to resolve, obviously.
> + * thenable - The thenable to resolve the Promise with.

thenable_

::: js/src/builtin/Promise.cpp:907
(Diff revision 1)
> -    // Step 9.
> -    bool success;
> +/**
> + * Tells the embedding to enqueue a Promise resolve thenable job, based on
> + * three parameters:
> + * promiseToResolve - The promise to resolve, obviously.
> + * thenable - The thenable to resolve the Promise with.
> + * then - The `then` function to invoke with the `thenable` as the receiver.

thenVal

::: js/src/builtin/Promise.cpp:1028
(Diff revision 1)
> +
> +    resolutionFun->setExtendedSlot(0, UndefinedValue());
> +    resolutionFun->setExtendedSlot(1, UndefinedValue());
> +
> +    otherFun->setExtendedSlot(0, UndefinedValue());
> +    otherFun->setExtendedSlot(1, UndefinedValue());

it would be nice to have some comment or assertion what those 0 and 1 are.

or, maybe, assert that `ResolveFunctionSlot_Promise == 
RejectFunctionSlot_Promise` and `ResolveFunctionSlot_RejectFunction == RejectFunctionSlot_ResolveFunction`, and use one of them instead of literal 0 and 1?

::: js/src/builtin/Promise.cpp:1078
(Diff revision 1)
> +        JS::dbg::onNewPromise(cx, promise);
> +
> +    return promise;
> +}
> +
> +// ES6, 25.4.3.1.

ES2016

::: js/src/builtin/Promise.cpp:1257
(Diff revision 1)
> +Promise_static_all(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedValue iterable(cx, args.get(0));
> +
> +    // Step 2.

// Step 2 (reordered).

::: js/src/builtin/Promise.cpp:1301
(Diff revision 1)
> +        // Step 8.b.
> +        return AbruptRejectPromise(cx, args, resultPromise, reject);
> +    }
> +
> +    // Step 9.
> +    args.rval().set(ObjectValue(*resultPromise));

args.rval().setObject(*resultPromise);

::: js/src/builtin/Promise.cpp:1341
(Diff revision 1)
> +        return nullptr;
> +
> +    // Steps 4-6 (omitted).
> +
> +    // Step 7.
> +    // Implemented as an inlined, simplied version of PerformPromiseAll.

it would be nice to say "ES2016 25.4.4.1.1" in the comment, so that the step numbers are clear.

::: js/src/builtin/Promise.cpp:1377
(Diff revision 1)
> +        // Sub-step 5 (inline in loop-header below).
> +
> +        // Sub-step 6.
> +        for (uint32_t index = 0; index < promiseCount; index++) {
> +            // Steps a-c (omitted).
> +            // Step d (implemented after the loop.

please add closing ")"

::: js/src/builtin/Promise.cpp:1383
(Diff revision 1)
> +            // Steps e-g (omitted).
> +
> +            // Step h.
> +            valuesArray->setDenseElement(index, UndefinedHandleValue);
> +
> +            // Steps i-j, vastly simplified.

Step i

::: js/src/builtin/Promise.cpp:1386
(Diff revision 1)
> +            valuesArray->setDenseElement(index, UndefinedHandleValue);
> +
> +            // Steps i-j, vastly simplified.
> +            RootedObject nextPromiseObj(cx, promises[index]);
> +
> +            // Step k.

Step j

::: js/src/builtin/Promise.cpp:1393
(Diff revision 1)
> +                                                             1, nullptr,
> +                                                             gc::AllocKind::FUNCTION_EXTENDED));
> +            if (!resolveFunc)
> +                return nullptr;
> +
> +            // Steps l-p.

Steps k-o

::: js/src/builtin/Promise.cpp:1398
(Diff revision 1)
> +            // Steps l-p.
> +            resolveFunc->setExtendedSlot(PromiseAllResolveElementFunctionSlot_Data, dataHolderVal);
> +            resolveFunc->setExtendedSlot(PromiseAllResolveElementFunctionSlot_ElementIndex,
> +                                         Int32Value(index));
> +
> +            // Step q.

Step p

::: js/src/builtin/Promise.cpp:1403
(Diff revision 1)
> +            // Step q.
> +            remainingCount++;
> +            dataHolder->setFixedSlot(PromiseAllDataHolderSlot_RemainingElements,
> +                                     Int32Value(remainingCount));
> +
> +            // Steps r-s, very roughly.

Step q

::: js/src/builtin/Promise.cpp:1405
(Diff revision 1)
> +            dataHolder->setFixedSlot(PromiseAllDataHolderSlot_RemainingElements,
> +                                     Int32Value(remainingCount));
> +
> +            // Steps r-s, very roughly.
> +            RootedValue resolveFunVal(cx, ObjectValue(*resolveFunc));
> +            RootedValue rejectFunVal(cx, ObjectOrNullValue(reject));

`reject` should be non-null here.
so `ObjectValue` can be used instead.

::: js/src/builtin/Promise.cpp:1422
(Diff revision 1)
> +                                    resultPromise, nullptr, nullptr))
> +            {
> +                return nullptr;
> +            }
> +
> +            // Step t (inline in loop-header).

Step r

::: js/src/builtin/Promise.cpp:1474
(Diff revision 1)
> +        return true;
> +    return RejectMaybeWrappedPromise(cx, promiseObj, result);
> +
> +}
> +
> +// ES2016, 25.4.4.1.1

25.4.4.1.1.
(trailing period)

::: js/src/builtin/Promise.cpp:1561
(Diff revision 1)
> +        // Step d.
> +        if (done) {
> +            // Step d.i (implicit).
> +            // Step d.ii.
> +            remainingCount = dataHolder->getFixedSlot(PromiseAllDataHolderSlot_RemainingElements)
> +                             .toInt32();

would be nice to have accessor methods on PromiseAllDataHolder class, for all slots.
maybe in followup bug.

::: js/src/builtin/Promise.cpp:1676
(Diff revision 1)
> +        // See comment for PerformPromiseAll, step 3 for why we unwrap here.
> +        valuesObj = UncheckedUnwrap(valuesObj);
> +    }
> +    RootedNativeObject values(cx, &valuesObj->as<NativeObject>());
> +
> +    // Step 6 (moved unter step 10).

under

::: js/src/builtin/Promise.cpp:1731
(Diff revision 1)
> +Promise_static_race(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedValue iterable(cx, args.get(0));
> +
> +    // Step 2.

Step 2 (reordered)

::: js/src/builtin/Promise.cpp:1779
(Diff revision 1)
> +    // Step 9.
> +    args.rval().set(ObjectValue(*resultPromise));
> +    return true;
> +}
> +
> +// ES2016, 25.4.4.3.1

trailing period

::: js/src/builtin/Promise.cpp:1790
(Diff revision 1)
> +    MOZ_ASSERT(C->isConstructor());
> +    RootedValue CVal(cx, ObjectValue(*C));
> +
> +    // Step 2 (omitted).
> +
> +    // Step 3.

I don't see correspondins steps 1-3 in http://www.ecma-international.org/ecma-262/7.0/#sec-performpromiserace

can you remove those comments?

::: js/src/builtin/Promise.cpp:2009
(Diff revision 1)
> +    if (promise->compartment() != cx->compartment()) {
> +        if (!cx->compartment()->wrap(cx, &promiseObj))
> +            return nullptr;
> +    }
>  
> -    if (keys.length() == 0)
> +    RootedValue ctorVal(cx);

// Step 3.

::: js/src/builtin/Promise.cpp:2014
(Diff revision 1)
> -    if (keys.length() == 0)
> -        return true;
> +    RootedValue ctorVal(cx);
> +    if (!SpeciesConstructor(cx, promiseObj, JSProto_Promise, &ctorVal))
> +        return nullptr;
> +    RootedObject C(cx, &ctorVal.toObject());
>  
> -    if (!values.growBy(keys.length()))
> +    // Steps 5-6.

Step 4.

::: js/src/builtin/Promise.cpp:2021
(Diff revision 1)
> +    RootedObject resolve(cx);
> +    RootedObject reject(cx);
> +    if (!NewPromiseCapability(cx, C, &resultPromise, &resolve, &reject, true))
> +        return nullptr;
>  
> -    // Each reaction is an internally-created object with the structure:
> +    // Step 7.

Step 5.

::: js/src/builtin/Promise.cpp:2163
(Diff revision 1)
> -    RootedNativeObject jobData(cx, &jobDataObj->as<NativeObject>());
> -    RootedValue argument(cx, jobData->getDenseElement(ReactionJobDataSlot_HandlerArg));
>  
> -    // Step 1 (omitted).
> +    // By default, the blocked promise is added as an extra entry to the
> +    // rejected promises list.
> +    bool addToDependent = true;

this can be inside the then-branch of the following `if`.

::: js/src/builtin/Promise.cpp:2167
(Diff revision 1)
> +    // rejected promises list.
> +    bool addToDependent = true;
> +    if (promiseObj->is<PromiseObject>() && IsNativeFunction(thenVal, Promise_then)) {
> +        // |promise| is an unwrapped Promise, and |then| is the original
> +        // |Promise.prototype.then|, inline it here.
> +        // 25.4.5.3., steps 3-4.

step 3

::: js/src/builtin/Promise.cpp:2184
(Diff revision 1)
> -        int32_t handlerNum = int32_t(handlerVal.toNumber());
> -        // Step 4.
> -        if (handlerNum == PROMISE_HANDLER_IDENTITY) {
> -            handlerResult = argument;
>          } else {
> -            // Step 5.
> +            // 25.4.5.3., steps 5-6.

step 4

::: js/src/builtin/Promise.cpp:2189
(Diff revision 1)
> -            MOZ_ASSERT(handlerNum == PROMISE_HANDLER_THROWER);
> -            shouldReject = true;
> +            if (!NewPromiseCapability(cx, C, &resultPromise, &resolveFun, &rejectFun, true))
> +                return false;
> -            handlerResult = argument;
>          }
> -    } else {
> -        // Step 6.
> +
> +        // 25.4.5.3., step 7.

step 5

::: js/src/builtin/Promise.cpp:2204
(Diff revision 1)
> +        RootedValue rval(cx);
> +        if (!Call(cx, thenVal, promiseVal, onFulfilled, onRejected, &rval))
> +            return false;
>      }
>  
> -    // Steps 7-9.
> +    if (!addToDependent)

this can be in the then-branch of the `if` above.

::: js/src/builtin/Promise.cpp:2257
(Diff revision 1)
> +        ac.emplace(cx, promise);
> +        if (!cx->compartment()->wrap(cx, &reactionVal))
> +            return false;
> +    }
>  
> -    // Re-rooting because we might need to unwrap it.
> +    // Steps 7.a,b.

25.4.5.3.1 steps 7.1,b.

::: js/src/builtin/Promise.cpp:2279
(Diff revision 1)
> +
> +    if (reactionsObj->is<PromiseReactionRecord>()) {
> +        // If a single reaction existed so far, create a list and store the
> +        // old and the new reaction in it.
> +        reactions = NewDenseFullyAllocatedArray(cx, 2);
> +        if (!reactions || reactions->ensureDenseElements(cx, 0, 2) != DenseElementResult::Success)

I'd prefer separating these 2 checks into 2 if's.

::: js/src/builtin/Promise.cpp:2327
(Diff revision 1)
>  }
>  
> -enum ThenableJobSlots {
> -    ThenableJobSlot_Handler = 0,
> -    ThenableJobSlot_JobData,
> -};
> +uint64_t
> +PromiseObject::getID()
> +{
> +    Value idVal(getReservedSlot(PromiseSlot_Id));

it's pre-existing tho
I don't find any reason to use getReservedSlot instead of getFixedSlot, since getFixedSlot is used in most of other places.

::: js/src/builtin/Promise.cpp:2352
(Diff revision 1)
> -PromiseResolveThenableJob(JSContext* cx, unsigned argc, Value* vp)
> +PromiseObject::dependentPromises(JSContext* cx, MutableHandle<GCVector<Value>> values)
>  {
> -    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (state() != JS::PromiseState::Pending)
> +        return true;
>  
> -    RootedFunction job(cx, &args.callee().as<JSFunction>());
> +    RootedValue reactionsVal(cx, getReservedSlot(PromiseSlot_ReactionsOrResult));

here too, especially for PromiseSlot_ReactionsOrResult, it's accessed with getFixedSlot in some other place.

::: js/src/builtin/Promise.cpp:2365
(Diff revision 1)
> -    RootedValue rejectVal(cx);
> -    if (!CreateResolvingFunctions(cx, promise, &resolveVal, &rejectVal))
> +    // If only a single reaction is pending, it's stored directly.
> +    if (reactions->is<PromiseReactionRecord>()) {
> +        if (!values.growBy(1))
> +            return false;
> +        RootedValue promiseVal(cx, reactions->getFixedSlot(ReactionRecordSlot_Promise));
> +        Rooted<PromiseObject*> promise(cx, &promiseVal.toObject().as<PromiseObject>());

those 2 lines are unnecessary.

::: js/src/builtin/Promise.cpp:2385
(Diff revision 1)
> +    }
>  
> -    RootedValue rval(cx);
> +    return true;
> +}
>  
> -    // In difference to the usual pattern, we return immediately on success.
> +namespace js {

I don't see any reason to put namespace here.
it's only for EnqueuePromiseReactions.
can you remove this namespace block and just add js:: to EnqueuePromiseReactions ?

::: js/src/builtin/Promise.cpp:2481
(Diff revision 1)
>      }
>      js_delete(this);
>  }
>  
>  void
> -PromiseTask::cancel(JSContext* cx)
> +js::PromiseTask::cancel(JSContext* cx)

js:: can be removed, even after removing namespace block

::: js/src/builtin/TestingFunctions.cpp:1470
(Diff revision 1)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (!args.requireAtLeast(cx, "getWaitForAllPromise", 1))
> +        return false;
> +    if (!args[0].isObject() || !args[0].toObject().is<NativeObject>()) {
> +        JS_ReportErrorASCII(cx, "first argument must be a Promise object");

this message sounds wrong.
first argument must be an array of promise objects.

::: js/src/builtin/TestingFunctions.cpp:1475
(Diff revision 1)
> +        JS_ReportErrorASCII(cx, "first argument must be a Promise object");
> +        return false;
> +    }
> +    RootedNativeObject list(cx, &args[0].toObject().as<NativeObject>());
> +    AutoObjectVector promises(cx);
> +    uint32_t count = list->getDenseInitializedLength();

it would be nice to assert that it a dense array.
so wrong usage will be caught on debug build.

::: js/src/jsapi.cpp:4796
(Diff revision 1)
>      return promise->as<PromiseObject>().reject(cx, rejectionValue);
>  }
>  
>  JS_PUBLIC_API(JSObject*)
> -JS::CallOriginalPromiseThen(JSContext* cx, JS::HandleObject promise,
> +JS::CallOriginalPromiseThen(JSContext* cx, JS::HandleObject promiseObj,
>                              JS::HandleObject onResolve, JS::HandleObject onReject)

Appending "Obj" to onResolve and onReject might be nice, to align with JS::AddPromiseReactions below.

::: js/src/jsapi.cpp:4879
(Diff revision 1)
> +//
> +//    if (remainingElementsCount.value === 0)
> +//        callFunction(resultCapability.resolve, undefined, values);
> +//
> +//    return allPromise;
> +//}

please remove this comment block
Attachment #8804639 - Flags: review?(arai.unmht) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8805568 [details]
Bug 1313049 - Review feedback.

https://reviewboard.mozilla.org/r/89334/#review88524

Thanks!
Attachment #8805568 - Flags: review?(arai.unmht) → review+

Comment 7

2 years ago
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72764ba31b81
Port Promise-related functions from self-hosted JS to C++. r=arai, f=bz

Comment 8

2 years ago
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c08e1aca9ea
Follow-up to fix bustage on 32bit, responsible for a CLOSED TREE. r=me

Comment 10

2 years ago
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309ecb16acfe
Port Promise-related functions from self-hosted JS to C++. r=arai, f=bz

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/309ecb16acfe
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
See Also: → bug 1314055
Depends on: 1314392
Depends on: 1314386
(Assignee)

Updated

2 years ago
Flags: needinfo?(till)

Updated

2 years ago
Depends on: 1315751
Depends on: 1394530
You need to log in before you can comment on or make changes to this bug.