1392088 - Shrink PromiseReactionRecord from 12 to 8 slots

Status: RESOLVED DUPLICATE of bug 1475678

(Core :: JavaScript: Standard Library, enhancement, P3)

Description

[Till Schneidereit [:till]]

PromiseReactionRecord has 9 slots but really doesn't need more than 8, so let's change that.

We can go even further than this and shrink it to 4 slots, but that requires fairly drastic changes to how reaction jobs are structured, such as introducing subclasses for the different kinds of jobs, and a dedicated subclass for jobs that have default values for resolve/reject functions, and an incumbentGlobal that's identical to the Promise's global. Combined with using the resultOrReason from the Promise instead of storing handlerArg on the reaction, that gets us to 4 slots.

I don't want to do that work until bug 1342044 is done, because that'll make it somewhat easier to do this.

Comment 1

[Till Schneidereit [:till]]

Attachment: Shrink PromiseReactionRecord from 12 to 8 slots

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8899258 [details] [diff] [review]
Shrink PromiseReactionRecord from 12 to 8 slots

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

Looks good, but I'm not sure about the latter part.

::: js/src/builtin/Promise.cpp
@@ +649,5 @@
>      // This is relevant for some html APIs like fetch that derive information
>      // from said global.
>      mozilla::Maybe<AutoCompartment> ac2;
> +    bool promiseNeedsWrapping = false;
> +    if (handler.isObject() && handler.toObject().compartment() != cx->compartment()) {

Can you explain how this change is related to this bug?

Comment 3

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Can you explain how this change is related to this bug?

Sorry, it's not related - just a tiny optimization that I threw in because I didn't think it's worth filing a bug over. I meant to mention that, but forgot. If you think it shouldn't be bundled in here, I'll file a new bug.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8899258 [details] [diff] [review]
Shrink PromiseReactionRecord from 12 to 8 slots

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

(In reply to Till Schneidereit [:till] from comment #3)
> (In reply to Tooru Fujisawa [:arai] from comment #2)
> > Can you explain how this change is related to this bug?
> 
> Sorry, it's not related - just a tiny optimization that I threw in because I
> didn't think it's worth filing a bug over. I meant to mention that, but
> forgot. If you think it shouldn't be bundled in here, I'll file a new bug.

Okay, thanks :)
just wondered if it's somehow related and I'm missing the relation.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d080b90698bc
Shrink PromiseReactionRecord from 12 to 8 slots. r=arai

Comment 6

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

Backed this out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=125342059&repo=mozilla-inbound

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc711acee91
Backed out changeset d080b90698bc for leaks a=backout CLOSED TREE

Comment 8

[Till Schneidereit [:till]]

Astoundingly, this is the patch that caused the window leaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e709bc4acca8a70e413f4292499656a93b4d8603

It's very unclear to me how this is happening. I don't have time to do it right now, but will investigate in a while.

Comment 9

[André Bargull [:anba]]

Fixed as part of bug 1475678 https://hg.mozilla.org/mozilla-central/rev/e064d391ad24.