1358879 - Optimize handling of internally-created Promise objects more

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[Till Schneidereit [:till]]

Attachment: Optimize handling of internally-created Promise objects

JS Promise objects created via DOM Promises currently always have a dummy executor and create actual JSFunctions for resolve/reject. This is silly as internally we can perfectly well deal with not having those. The attached patch does away will all this, and additionally optimizes the case of not having resolve/reject functions when resolving/rejecting promises a bit more by making use of the fact that we know we don't have a wrapped promise in most cases.

bz, asking you for review because while all bits of this are trivial, most of the diff is in DOM-land. (It's admittedly almost entirely code removal, but still.)

Comment 1

[Till Schneidereit [:till]]

Attachment: Optimize handling of internally-created Promise objects. v2

Now with fewer invalid changes. Specifically, PromiseObject::resolve can't skip the detection of thenables and self-resolution.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8860758 [details] [diff] [review]
Optimize handling of internally-created Promise objects. v2

This seems reasonable.

One question about the context here.  In RunResolutionFunction, can we have a situation in which resolutionFun is null but PROMISE_FLAG_DEFAULT_REJECT_FUNCTION/PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION are _not_ set?  The comment seems to imply no, but if it _can_ happen, it would be good to adjust the comment accordingly.

The docs for JS::NewPromiseObject should say that "executor" is allowed to be null and what the resulting behavior is (e.g. the promise can only be resolved/rejected via JS::ResolvePromise/JS::RejectPromise, right?)

r=me.

Comment 3

[[:philipp]]

hi, would this be ready to land? it might help with a top crasher on release/beta and having it for a couple of days on nightly might already give an indication if things are improving....

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bca3f9530e
Optimize handling of internally-created Promise objects. r=bz

Comment 5

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2)

> One question about the context here.  In RunResolutionFunction, can we have
> a situation in which resolutionFun is null but
> PROMISE_FLAG_DEFAULT_REJECT_FUNCTION/PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION
> are _not_ set?  The comment seems to imply no, but if it _can_ happen, it
> would be good to adjust the comment accordingly.

This situation can occur, which is why I wrote "absence of a resolve/reject function *can* mean". That is admittedly quite subtle, so I added an explanation.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b9bca3f9530e

Comment 7

[[:philipp]]

would it help for the crash in bug 1347984 getting this onto beta?
(though there are still a handful of [@ EnqueuePromiseReactionJob] reports on nightly after the patch has landed)

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Which versions are affected here?