1475678 - Promise clean-up and optimisation work

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[André Bargull [:anba]]

I have a stack of fourteen patches to clean-up and optimise Promise built-ins.


Some performance numbers after the patches:

// Promise.race
//  2:  75ms ->  50ms
//  4: 120ms ->  70ms
// 10: 260ms -> 145ms
//
// Promise.all
//  2: 150ms ->  75ms
//  4: 220ms -> 110ms
// 10: 415ms -> 220ms
function testAllOrRace() {
    var t = 0;
    for (var i = 0; i < 40000; ++i) {
        var a = Array(2).fill(null).map(() => newPromiseCapability());
        var xs = a.map(c => c.promise);

        var u = dateNow()
        Promise.race(cx);
        // Promise.all(xs);
        t += dateNow() - u;

        a.forEach(c => c.resolve());
        drainJobQueue();
    }
    print(t);
}


// Promise.then
//  125ms -> 95ms
//
// Promise.catch
//   140ms -> 95ms
function testThenOrCatch() {
    var t = 0;
    for (var i = 0; i < 200000; ++i) {
        var c = newPromiseCapability();

        var u = dateNow()
        c.promise.then();
        // c.promise.catch();
        t += dateNow() - u;

        c.resolve();
        drainJobQueue();
    }
    print(t);
}

Comment 1

[André Bargull [:anba]]

Attachment: bug1475678-part1-async-gen-promise-args.patch

Changes the parameter type for async-generator code from JSObject* to PromiseObject* in preparation for the next patch.

Comment 2

[André Bargull [:anba]]

Attachment: bug1475678-part2-reject-with-unwrapped-promise.patch

We don't need to go through |RejectMaybeWrappedPromise| when the promise object is definitely not a wrapped promise object. 

I've named the new helper method "RejectPromiseInternal", so it matches the already existing "ResolvePromiseInternal".

Comment 3

[André Bargull [:anba]]

Attachment: bug1475678-part3-double-rooted.patch

We don't need to root already rooted values.

Drive-by fix: Add a missing |ReportAccessDenied| call in |PromiseObject::create|.

Comment 4

[André Bargull [:anba]]

Attachment: bug1475678-part4-uint32-variables.patch

Changes variables from size_t to uint32_t when used for slots/dense indices, because slot and dense elements methods all expect uint32_t values.

Drive-by fix: Correct indentation in PromiseReactionRecord.

Comment 5

[André Bargull [:anba]]

Attachment: bug1475678-part5-dense-array-init.patch

Use more efficient functions to initialize dense elements.

EnqueuePromiseResolveThenableJob:

JSCompartment::wrap(...) can GC, so if we move both wrap() calls before the call to NewDenseFullyAllocatedArray(), we can safely use NativeObject::setDenseInitializedLength() and NativeObject::initDenseElement() to fill the array.


GetWaitForAllPromise:

We can use NativeObject::ensureDenseInitializedLength() to directly initialize all dense elements in an empty array. This is more efficient than calling NativeObject::ensureDenseElements().


AddPromiseReaction:

Similar to EnqueuePromiseResolveThenableJob, we can also use NativeObject::setDenseInitializedLength() and NativeObject::initDenseElement() to fill the first two elements.
And change the NativeObject::ensureDenseElements() call to use the "correct" invocation, so we fall into this fast path [1].


[1] https://searchfox.org/mozilla-central/rev/448f792f9652d29daebdad21bf50b03405e40a45/js/src/vm/NativeObject-inl.h#393-404

Comment 6

[André Bargull [:anba]]

Attachment: bug1475678-part6-unnecessary-rooting.patch

Remove unnecessary rooting where either no GC can happen or the value is directly stored in another rooted location.

Also moves rooted variables outside of loops in Promise.all and Promise.race for efficiency reasons.

Comment 7

[André Bargull [:anba]]

Attachment: bug1475678-part7-unwrapping-calls.patch

Avoid duplicate UncheckedUnwrap() calls and change some JSCompartment::wrap() calls to access the compartment through the context instead of using an object. (The latter change is just for consistency with other calls to wrap() in this file and probably also other parts of the engine.)

Also change |JSAutoRealm| to |AutoRealm| in PerformPromiseAll to avoid going through JSAPI for engine internal code.

And I've changed PromiseAllResolveElementFunction to use |IsProxy| instead of |IsWrapper| to match other code in this file. (I assume Promise code uses IsProxy instead of IsWrapper in some places for performance reasons only.)

Comment 8

[André Bargull [:anba]]

Attachment: bug1475678-part8-promise-capability-struct.patch

Add a PromiseCapability struct to avoid passing the PromiseCapability tuple ([[Promise]], [[Resolve]], and [[Reject]]) as three separate arguments in multiple functions.

This change also allows us to make the code a bit more similar to the spec. For example we can have a AbruptRejectPromise function which accepts a PromiseCapability <https://tc39.github.io/ecma262/#sec-ifabruptrejectpromise>.

The "resultCapability" resp. "promiseCapability" variable names were chosen to match the names used in the spec algorithms.

Comment 9

[André Bargull [:anba]]

Attachment: bug1475678-part9-promise-all-elem-array-init.patch

Another small change to improve filling the values array in PerformPromiseAll:

- Avoid the slowish JSCompartment::wrap() call when the promise object is not wrapped.
- Use NewDenseEmptyArray(cx) instead of NewDenseFullyAllocatedArray(cx, 0), to skip a few extra checks in NewDenseFullyAllocatedArray.
- Call NewbornArrayPush instead of DefineDataProperty to initialize the values array with undefined-values. We can use NewbornArrayPush here, because the values array is only exposed to user code when we've finished the iteration.
- Remove the no longer necessary magic-value check in NativeObject::initDenseElementWithType() so it matches NativeObject::setDenseElementWithType(). And also avoid calling AddTypePropertyId() when it's not necessary, similar to the optimisation in setDenseElementWithType().

Comment 10

[André Bargull [:anba]]

Attachment: bug1475678-part10-combine-race-all-loop.patch

Combine the loops from PerformPromiseAll and PerformPromiseRace into a single helper function CommonPerformPromiseAllRace.

This change is in preparation for the next patch, which adds more code into the loop and by having only a single implementation of this loop we can avoid quite a bit of code duplication.

And also inline BlockOnPromise() into the new CommonPerformPromiseAllRace(), because (a) BlockOnPromise() is now only called from CommonPerformPromiseAllRace() and (b) this'll also make the next patch easier to integrate.

Combining everything into a single function requires quite a bit of rooted values and because we normally try to define rooted variables outside of loop statements (in performance sensitive code), I've tried to reuse rooted variables where possible to improve readability and for a tiny bit of additional performance. When a rooted variable is reused, the variable is accordingly named to reflect all use sites, e.g. |RootedObject thenSpeciesOrBlockedPromise| for a rooted object which is first used for |Rooted thenSpecies| and then for |RootedObject blockedPromise|.

Comment 11

[André Bargull [:anba]]

Attachment: bug1475678-part11-property-lookup-cache.patch

Add a property lookup cache similar to the existing lookup cache for Array[@@species]. Because Promise operations are not as performance sensitive as Array[@@species] code, the lookup doesn't store pointers to the |Promise| and |Promise.prototype|. This also reduces the sizeof(PromiseLookup), so it should still be acceptable to directly store PromiseLookup as a member of js::Realm instead of malloc'ing it dynamically. Apart from that the cache matches more or less ArraySpeciesLookup.

The new PromiseLookup class only supports Promise object without own properties and whose prototype is |Promise.prototype|. I hope this covers the majority of current use cases for Promise objects, but if we ever want to optimise Promise sub-classes, we'll need to revisit this decision. 


ResolvePromiseInternal
- It looks like |promise| and |resolutionVal| are always from the same compartment, which enables us to remove some of the compartment checks I've recently added for the EnqueuePromiseResolveThenableBuiltinJob fast path. (But also asserts this to be sure that this is really the case.)
- Use the new promise lookup cache to try to avoid the GetProperty() call.


CommonPerformPromiseAllRace
- Uses the lookup cache to skip up to five GetProperty() calls per each iteration ([[Get]] for "resolve", in |Promise.resolve| the "constructor" lookup, then getting the  "then" property, in |Promise.prototype.then| another lookup for "constructor" and also for @@species).
- Additionally inspect the JS::ForOfIterator (through the new PromiseForOfIterator subclass) to detect the case when the iteration itself doesn't produce any side-effects. This allows us to skip revalidating the promise lookup cache for each iteration.


Promise_catch_impl and Promise_then_impl:
- Use the lookup cache to directly call into PerformPromiseThen.

Comment 12

[André Bargull [:anba]]

Attachment: bug1475678-part12-reaction-record-alloc-size.patch

We don't need to keep both handlers alive in PromiseReactionRecord when we've determined the target state. Instead we can reuse the slot of the no longer used handler to store the handler-argument. This change has two benefits:

1. It makes the no longer used handler object eligible for GC.
2. It reduces the slot size of PromiseReactionRecord by one, which means PromiseReactionRecord now has "only" eights slots, so it fits into the OBJECT8 allocation kind.

Comment 13

[André Bargull [:anba]]

Attachment: bug1475678-part13-call-cleanup.patch

Another clean-up patch to use the Call() functions from Interpreter.h, so we don't need to create FixedInvokeArgs objects ourselves.

Comment 14

[André Bargull [:anba]]

Attachment: bug1475678-part14-clear-incumbent.patch

Clear the "IncumbentGlobalObject" slot when it's no longer needed to avoid an unnecessary GC thing reference in PromiseReactionRecord objects. (I don't know if this change makes a real difference for the GC, but at least theoretically it should be better to remove unnecessary references to GC things.)

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8992071 [details] [diff] [review]
bug1475678-part6-unnecessary-rooting.patch

Review of attachment 8992071 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.cpp
@@ +628,5 @@
>          args.rval().setUndefined();
>          return true;
>      }
>  
> +    RootedObject promise(cx, &promiseVal.toObject());

this needs some comment about the ordering.
iiuc, this is done before ClearResolutionFunctionSlots in order to keep the following reference graph, right?
  args.callee() -> reject -> promiseVal -> promise

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8992074 [details] [diff] [review]
bug1475678-part8-promise-capability-struct.patch

Review of attachment 8992074 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup :D

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8992077 [details] [diff] [review]
bug1475678-part9-promise-all-elem-array-init.patch

Review of attachment 8992077 [details] [diff] [review]:
-----------------------------------------------------------------

I'll review remaining patches this afternoon.

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8992081 [details] [diff] [review]
bug1475678-part10-combine-race-all-loop.patch

Review of attachment 8992081 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.cpp
@@ +2283,5 @@
> +IsPromiseSpecies(JSContext* cx, JSFunction* species);
> +
> +template <typename T>
> +static MOZ_MUST_USE bool
> +CommonPerformPromiseAllRace(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,

can you put the following comment above this definition?
  // ES2018 PerformPromiseAll 25.6.4.1.1 step 6.
  // ES2018 PerformPromiseRace 25.6.4.3.1 step 3.
so that "step a" etc make sense.

(or ES2016 25.4.4.x for each)

@@ +2296,5 @@
> +    HandleObject resultPromise = resultCapability.promise();
> +    RootedValue resolveFunVal(cx);
> +    RootedValue rejectFunVal(cx, ObjectValue(*resultCapability.reject()));
> +
> +    RootedValue nextValue(cx);

nextValueOrNextPromise ?

@@ +2300,5 @@
> +    RootedValue nextValue(cx);
> +    RootedObject nextPromiseObj(cx);
> +
> +    RootedValue resolveOrThen(cx);
> +    RootedObject thenSpeciesOrBlockedPromise(cx);

they need a comment about reuse/optimization.

@@ +2317,5 @@
> +        // Step d.
> +        if (*done)
> +            return true;
> +
> +        // Step i.

it's different step for each function.

// 25.6.4.1.1 step 6.i.
// 25.6.4.3.1 step 3.h.

@@ +2330,5 @@
> +        if (!Call(cx, staticResolve, CVal, nextValue, &nextPromise))
> +            return false;
> +
> +        // Get the resolve function for this iteration.
> +        // Steps j-q.

this step exists only in PerformPromiseAll.

// 25.6.4.1.1 steps 6.j-q.

@@ +2345,5 @@
> +        // |Promise| constructor to create the resulting promise, we skip the
> +        // call to |nextPromise.then| and thus creating a new promise that
> +        // would not be observable by content.
> +
> +        // Step r.

// 25.6.4.1.1 step 6.r.
// 25.6.4.3.1 step 3.i.

@@ +2362,5 @@
> +        if (nextPromiseObj->is<PromiseObject>() && IsNativeFunction(thenVal, Promise_then)) {
> +            // |nextPromise| is an unwrapped Promise, and |then| is the
> +            // original |Promise.prototype.then|, inline it here.
> +
> +            // 25.4.5.3., step 3.

if you use ES 2018 above, "25.6.5.4 step 3".

@@ +2535,1 @@
>          // Step h.

// Step 6.h.
same for other steps in this lambda, since isn't inside a block with "// Step 6." comment.

@@ +2570,5 @@
> +    // Step 6.
> +    if (!CommonPerformPromiseAllRace(cx, iterator, C, resultCapability, done, true, getResolve))
> +        return false;
> +
> +    // Step d.ii.

// Step 6.d.ii.
same for the following, since this is not inside a block which has "// Step 6."

Comment 19

[Tooru Fujisawa [:arai]]

Comment on attachment 8992090 [details] [diff] [review]
bug1475678-part11-property-lookup-cache.patch

Review of attachment 8992090 [details] [diff] [review]:
-----------------------------------------------------------------

I have some concern about GC related part.
:pbone, can you review GC part of this patch?
especially weak pointer of shape.

::: js/src/builtin/Promise.cpp
@@ +783,5 @@
> +        // through the resolving functions. In that case the normal bookkeeping to
> +        // ensure only pending promises can be resolved doesn't apply and we need
> +        // to manually check for already settled promises. The exception is simply
> +        // dropped when this case happens.
> +        if (IsSettledMaybeWrappedPromise(promise))

shouldn't we keep this for built-in "then"?

@@ +2316,5 @@
> +    // during the iteration.
> +    bool iterationMayHaveSideEffects = !iterator.isOptimizedDenseArrayIteration();
> +
> +    // Only try to optimize when C is the builtin Promise constructor.
> +    bool isDefaultPromiseState = C == promiseCtor;

can you add comment that explains this is updated to the correct value inside the loop?
because, "C == promiseCtor" itself doesn't mean "default promise state", but it's just a requirement.

@@ +2366,5 @@
> +
> +            if (nextValuePromise &&
> +                promiseLookup.isDefaultInstanceWhenPromiseStateIsSane(cx, nextValuePromise))
> +            {
> +                // BlockOnPromiseBuiltinThen doesn't produce any side-effects,

BlockOnPromiseBuiltinThen doesn't exist I think?
(or perhaps it's in other bug's patch I overlooked/forgot ?)

@@ +4173,5 @@
> +           getter->as<JSFunction>().realm() == cx->realm();
> +}
> +
> +void
> +js::PromiseLookup::initialize(JSContext* cx)

can you enumerate the short summary of requirement at the top?
like https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/js/src/vm/RegExpShared.h#304-312

maybe with index, and refer index in initialize and isPromiseStateStillSane methods,
so that it's clear we're not missing some requirement, and also when some other requirement is added in the future, we won't miss it.

@@ +4257,5 @@
> +
> +void
> +js::PromiseLookup::reset()
> +{
> +    JS_POISON(this, 0xBB, sizeof(this), MemCheckKind::MakeUndefined);

now 0xBB is used in 2 places, and I think it's better adding it to something like following (please ask review from GC people), and maybe teach it to mrgiggles :)

https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/js/src/jsutil.h#254-269
https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/js/src/gc/Marking.cpp#131-143

@@ +4371,5 @@
> +    // Ensure |promise| doesn't define any own properties. This serves as a
> +    // quick check to make sure |promise| doesn't define an own "constructor"
> +    // or "then" property which may shadow Promise.prototype.constructor or
> +    // Promise.prototype.then.
> +    return promise->lastProperty()->isEmptyShape();

it's lucky that Promise object doesn't have property by default :)
(otherwise we should cache its shape, like RegExp)

::: js/src/builtin/Promise.h
@@ +209,5 @@
> +
> +    // Shape of matching Promise, slot containing the @@species property.
> +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseConstructorShape_;
> +#ifdef DEBUG
> +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseSpeciesShape_;

it would be better pointing the comment in isPromiseStateStillSane to clarify why this is debug-only, which a bit contradicts with the explanation above.

@@ +213,5 @@
> +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseSpeciesShape_;
> +#endif
> +
> +    // Shape of matching Promise.prototype object.
> +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseProtoShape_;

given it's weak pointer, don't we have to use ReadBarriered<Shape*> instead ?
RegExp's cache uses it https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/js/src/vm/RegExpShared.h#314

I'd forward the review.

@@ +250,5 @@
> +    using CheckStateFn = decltype(&PromiseLookup::isPromiseStateStillSane);
> +
> +    enum class Reinitialize : bool {
> +        Allowed,
> +        Disallowed

can you add some comment what they mean, especially, reinitialization of what?
iiuc, this allows/disallows reinitializing the lookup cache when failure, right?

@@ +267,5 @@
> +    bool ensureThenInitialized(JSContext* cx) {
> +        return ensureInitialized<&PromiseLookup::isPromiseThenStateStillSane>(cx, Reinitialize::Allowed);
> +    }
> +
> +    // Return true if the prototype is Promise.prototype and the object

if the prototype *of the given Promise object* is

Comment 20

[Tooru Fujisawa [:arai]]

Comment on attachment 8992092 [details] [diff] [review]
bug1475678-part12-reaction-record-alloc-size.patch

Review of attachment 8992092 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.cpp
@@ +476,1 @@
>      ReactionRecordSlot_OnFulfilled,

can we rename this to OnFulfilledHandlerOrOnRejectedArg or define one more enum entry like this?
  ReactionRecordSlot_OnRejectedArg = ReactionRecordSlot_OnFulfilled,
same for OnRejected.

or add explicit comment in handlerArgSlot method to clarify the name mismatch is intentional.

Comment 21

[Paul Bone [:pbone]]

Comment on attachment 8992090 [details] [diff] [review]
bug1475678-part11-property-lookup-cache.patch

Review of attachment 8992090 [details] [diff] [review]:
-----------------------------------------------------------------

r+ pending fixes.

::: js/src/builtin/Promise.cpp
@@ +4257,5 @@
> +
> +void
> +js::PromiseLookup::reset()
> +{
> +    JS_POISON(this, 0xBB, sizeof(this), MemCheckKind::MakeUndefined);

Arai is right, this should be a new arbitrary bit pattern and added to the lists on those two places.

::: js/src/builtin/Promise.h
@@ +213,5 @@
> +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseSpeciesShape_;
> +#endif
> +
> +    // Shape of matching Promise.prototype object.
> +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseProtoShape_;

Yes. but I don't see where it says it's a weak ref.  If it is perhaps use WeakRef<Shape*> instead?

Comment 22

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #19)
> ::: js/src/builtin/Promise.cpp
> @@ +783,5 @@
> > +        // through the resolving functions. In that case the normal bookkeeping to
> > +        // ensure only pending promises can be resolved doesn't apply and we need
> > +        // to manually check for already settled promises. The exception is simply
> > +        // dropped when this case happens.
> > +        if (IsSettledMaybeWrappedPromise(promise))
> 
> shouldn't we keep this for built-in "then"?

I'll just remove the change for this function for now, because all it saves is a single property lookup, which currently may improve performance in µ-benchmarks by 1%, if at all.


> @@ +2366,5 @@
> > +
> > +            if (nextValuePromise &&
> > +                promiseLookup.isDefaultInstanceWhenPromiseStateIsSane(cx, nextValuePromise))
> > +            {
> > +                // BlockOnPromiseBuiltinThen doesn't produce any side-effects,
> 
> BlockOnPromiseBuiltinThen doesn't exist I think?
> (or perhaps it's in other bug's patch I overlooked/forgot ?)

Oh, that's just a leftover from a previous version of these patches. 


> @@ +4257,5 @@
> > +
> > +void
> > +js::PromiseLookup::reset()
> > +{
> > +    JS_POISON(this, 0xBB, sizeof(this), MemCheckKind::MakeUndefined);
> 
> now 0xBB is used in 2 places, and I think it's better adding it to something
> like following (please ask review from GC people), and maybe teach it to
> mrgiggles :)
> 

I don't really know the rules when to add patterns to the list in jsutil.h. jsutil.h refers to IsThingPoisoned, which makes it sound like only things which are GC-things should be in jsutil.h? Furthermore there are other callers to JS_POISON which directly pass a hex-value [1], including a reuse for 0xCC (NativeIterator::NativeIterator and TrailingNamesArray).

[1] https://searchfox.org/mozilla-central/search?q=JS_POISON%5C(%5B%5E%2C%5D%2B%2C%5Cs%2B0&case=true&regexp=true&path=


> @@ +213,5 @@
> > +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseSpeciesShape_;
> > +#endif
> > +
> > +    // Shape of matching Promise.prototype object.
> > +    MOZ_INIT_OUTSIDE_CTOR Shape* promiseProtoShape_;
> 
> given it's weak pointer, don't we have to use ReadBarriered<Shape*> instead ?
> RegExp's cache uses it
> https://searchfox.org/mozilla-central/rev/
> 6f86cc3479f80ace97f62634e2c82a483d1ede40/js/src/vm/RegExpShared.h#314
> 
> I'd forward the review.
> 

The complete cache is purged on GC (see bug 1376572, comment #3 and bug 1376572, comment #7 for the design), so unless things have changed, ReadBarriered shouldn't be necessary here.

Comment 23

[Tooru Fujisawa [:arai]]

(In reply to André Bargull [:anba] from comment #22)
> > @@ +4257,5 @@
> > > +
> > > +void
> > > +js::PromiseLookup::reset()
> > > +{
> > > +    JS_POISON(this, 0xBB, sizeof(this), MemCheckKind::MakeUndefined);
> > 
> > now 0xBB is used in 2 places, and I think it's better adding it to something
> > like following (please ask review from GC people), and maybe teach it to
> > mrgiggles :)
> > 
> 
> I don't really know the rules when to add patterns to the list in jsutil.h.
> jsutil.h refers to IsThingPoisoned, which makes it sound like only things
> which are GC-things should be in jsutil.h? Furthermore there are other
> callers to JS_POISON which directly pass a hex-value [1], including a reuse
> for 0xCC (NativeIterator::NativeIterator and TrailingNamesArray).
> 
> [1]
> https://searchfox.org/mozilla-central/
> search?q=JS_POISON%5C(%5B%5E%2C%5D%2B%2C%5Cs%2B0&case=true&regexp=true&path=

IMO everything should be listed in single place there, in order to give name and unique value :)
(so, we should fix those callsites as well)


> The complete cache is purged on GC (see bug 1376572, comment #3 and bug
> 1376572, comment #7 for the design), so unless things have changed,
> ReadBarriered shouldn't be necessary here.

oh, indeed.
I overlooked the difference of the requirements between RegExp's cache and this cache.
https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/js/src/vm/Realm.cpp#433-437

Comment 24

[Paul Bone [:pbone]]

> > @@ +4257,5 @@
> > > +
> > > +void
> > > +js::PromiseLookup::reset()
> > > +{
> > > +    JS_POISON(this, 0xBB, sizeof(this), MemCheckKind::MakeUndefined);
> > 
> > now 0xBB is used in 2 places, and I think it's better adding it to something
> > like following (please ask review from GC people), and maybe teach it to
> > mrgiggles :)
> > 
> 
> I don't really know the rules when to add patterns to the list in jsutil.h.
> jsutil.h refers to IsThingPoisoned, which makes it sound like only things
> which are GC-things should be in jsutil.h? Furthermore there are other
> callers to JS_POISON which directly pass a hex-value [1], including a reuse
> for 0xCC (NativeIterator::NativeIterator and TrailingNamesArray).
> 
> [1]
> https://searchfox.org/mozilla-central/
> search?q=JS_POISON%5C(%5B%5E%2C%5D%2B%2C%5Cs%2B0&case=true&regexp=true&path=
> 

I don't know the rules either, but it seems like a good idea.  If you're still not sure then leave it ass is and we'll fix it in Bug 1476845.

Comment 25

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #23)
> > The complete cache is purged on GC (see bug 1376572, comment #3 and bug
> > 1376572, comment #7 for the design), so unless things have changed,
> > ReadBarriered shouldn't be necessary here.
> 
> oh, indeed.
> I overlooked the difference of the requirements between RegExp's cache and
> this cache.
> https://searchfox.org/mozilla-central/rev/
> 88199de427d3c5762b7f3c2a4860c10734abd867/js/src/vm/Realm.cpp#433-437

Hmm, the code comment in |Realm::sweepRegExps()| seems to be out of date since quite some time. Filed bug 1476903.



(In reply to Paul Bone [:pbone] from comment #24)
> I don't know the rules either, but it seems like a good idea.  If you're
> still not sure then leave it ass is and we'll fix it in Bug 1476845.

Thanks for filing that bug. I think I'll leave the patch in this bug as is, pending the resolution for bug 1476845.

Comment 26

[André Bargull [:anba]]

Attachment: bug1475678-part6-unnecessary-rooting.patch

Update part 6 per review comments, carrying r+.

Comment 27

[André Bargull [:anba]]

Attachment: bug1475678-part10-combine-race-all-loop.patch

Update part 10 per review comments, carrying r+.

Comment 28

[André Bargull [:anba]]

Attachment: bug1475678-part11-property-lookup-cache.patch

Update part 11 per review comments, carrying r+.

Comment 29

[André Bargull [:anba]]

Attachment: bug1475678-part12-reaction-record-alloc-size.patch

Update part 12 per review comments, carrying r+.

Comment 30

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=638d715e1948366cfb4b8ffa2bffa271b881efaf

Comment 31

[Andreea Pavel [:apavel]]

Tried to land this an got: 


applying bug1475678-part7-unwrapping-calls.patch
patching file js/src/builtin/Promise.cpp
Hunk #3 FAILED at 1151
Hunk #4 FAILED at 2247
2 out of 6 hunks FAILED -- saving rejects to file js/src/builtin/Promise.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1475678-part7-unwrapping-calls.patch

Comment 32

[André Bargull [:anba]]

The error in comment #31 happens when part 7 is applied after part 5, but part 6 hasn't yet been applied. 

Thanks,
André

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d04aa5f853
Part 1: Use PromiseObject instead JSObject as the parameter type for async generators. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ca4196b325
Part 2: Skip RejectMaybeWrappedPromise when the promise is definitely not wrapped. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8a735fbc88
Part 3: Remove additional rooting of already rooted values. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/9999014fea3e
Part 4: Change size_t variables to uint32_t when the underlying type is int32/uint32. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f16b5db5feb
Part 5: Improve dense array initialization for Promise code. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/297e8205c591
Part 6: Remove unnecessary rooting in Promise code. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dae42009b52
Part 7: Avoid duplicate UncheckedUnwrap calls and use AutoRealm instead of JSAutoRealm. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f122f832e9
Part 8: Add a PromiseCapability struct. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7a3253ffecd
Part 9: Directly initialize dense elements in PerformPromiseAll using NewbornArrayPush. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/7404656e431f
Part 10: Create a shared helper for PerformPromiseAll and PerformPromiseRace. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5851d5a423
Part 11: Add cache for Promise property lookups. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e064d391ad24
Part 12: Reduce slot size of PromiseReactionRecord to fit into OBJECT8 alloc kind. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d65a2e8d382
Part 13: Use the Call() helper from Interpreter.h to reduce code duplication. r=arai

Comment 34

[Andreea Pavel [:apavel]]

i just used the default order. pushed now.

Thanks

Comment 35

[André Bargull [:anba]]

(In reply to Andreea Pavel [:apavel] from comment #34)
> i just used the default order. pushed now.
> 
> Thanks

Cool, thanks! Part 14 seems to be missing, though. :-)

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1af3f6ba686
Part 14: Clear the incumbent global slot when it's no longer used. r=arai

Comment 37

[Andreea Pavel [:apavel]]

:anba sorry about that.

Comment 38

[André Bargull [:anba]]

No worries! And again thanks for taking care of pushing my patches! :-)

Comment 39

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f8d04aa5f853
https://hg.mozilla.org/mozilla-central/rev/e1ca4196b325
https://hg.mozilla.org/mozilla-central/rev/ef8a735fbc88
https://hg.mozilla.org/mozilla-central/rev/9999014fea3e
https://hg.mozilla.org/mozilla-central/rev/3f16b5db5feb
https://hg.mozilla.org/mozilla-central/rev/297e8205c591
https://hg.mozilla.org/mozilla-central/rev/3dae42009b52
https://hg.mozilla.org/mozilla-central/rev/04f122f832e9
https://hg.mozilla.org/mozilla-central/rev/c7a3253ffecd
https://hg.mozilla.org/mozilla-central/rev/7404656e431f
https://hg.mozilla.org/mozilla-central/rev/9d5851d5a423
https://hg.mozilla.org/mozilla-central/rev/e064d391ad24
https://hg.mozilla.org/mozilla-central/rev/1d65a2e8d382
https://hg.mozilla.org/mozilla-central/rev/c1af3f6ba686