Optimize handling of internally-created Promise objects more

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: till, Assigned: till)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 ?, firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.)
Attachment #8860737 - Flags: review?(bzbarsky)
Now with fewer invalid changes. Specifically, PromiseObject::resolve can't skip the detection of thenables and self-resolution.
Attachment #8860737 - Attachment is obsolete: true
Attachment #8860737 - Flags: review?(bzbarsky)
Attachment #8860758 - Flags: review?(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.
Attachment #8860758 - Flags: review?(bzbarsky) → review+
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....
Flags: needinfo?(till)
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bca3f9530e
Optimize handling of internally-created Promise objects. r=bz
(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.
Flags: needinfo?(till)
https://hg.mozilla.org/mozilla-central/rev/b9bca3f9530e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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)
Which versions are affected here?
You need to log in before you can comment on or make changes to this bug.