963201 - Performance implications of DOM Promise special case for Promise resolution

Status: RESOLVED INCOMPLETE

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Bug 879245 comment 40 and 41
This placeholder is to run perf tests to check whether there are wins to re-adding the special case while ensuring we respect overridden `then()` on canonical DOM Promises.

Comment 1

[Till Schneidereit [:till]]

I'm not 100% sure I even understand what the special case was. I'm pretty sure that this bug isn't useful anymore, given that the Promise implementation moved to SpiderMonkey and has a somewhat different set of optimizations.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The special case was that if a promise was resolved with another Promise we would do some sanity checks and then directly add our resolve/reject functions to its reactions list instead of doing .then() on it.  We removed it when adding thenable support in bug 879245.

For what it's worth, we _did_ readd the fast path in bug 1152902 (sorry, I had forgotten about this bug).  Then we removed it _again_ when we implemented promise subclassing in bug 1170760, because it required checking a bunch more stuff (like Symbol.species, etc) to be able to optimize out construction of the "no one cares about it" promise that then() returns.  :(

But yes, I agree this bug is no longer useful as filed.  Whatever optimizations along these lines we do should be done in the JS engine.

Comment 3

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> The special case was that if a promise was resolved with another Promise we
> would do some sanity checks and then directly add our resolve/reject
> functions to its reactions list instead of doing .then() on it.  We removed
> it when adding thenable support in bug 879245.

Ah, thanks for the explanation.

> But yes, I agree this bug is no longer useful as filed.  Whatever
> optimizations along these lines we do should be done in the JS engine.

I actually had code for this at one point, but it's not trivial to get it correct. Even if calling .then() behaves as expected and all that, not enqueuing a thenable job means we'll (at least potentially) trigger any reactions from the microtask we're about to enqueue. If in the current microtask we enqueue any other microtasks, then those should run before the reactions for the thenable are triggered.

What we'd need to make this work is a promise reaction job that has a flag saying "don't actually run me but reset this flag and just re-enqueue me.