1.79 - 9.04% tabpaint (linux64) regression on push a80fdfc128b0c29dc34dd3fc28868252111a5d52 (Sat Jul 16 2016)

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ashiue, Assigned: till)

Tracking

({perf, regression, talos-regression})

50 Branch
mozilla51
perf, regression, talos-regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 wontfix, firefox51 fixed)

Details

Attachments

(9 attachments, 8 obsolete attachments)

58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
58 bytes, text/x-review-board-request
efaust
: review+
Details
(Reporter)

Description

2 years ago
Talos has detected a Firefox performance regression from push a80fdfc128b0c29dc34dd3fc28868252111a5d52. As author of one of the patches included in that push, we need your help to address this regression.

Summary of tests that regressed:

  tabpaint summary linux64 opt - 1.79% worse
  tabpaint summary linux64 pgo - 9.04% worse


You can find links to graphs and comparsion views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=1942

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
(Reporter)

Comment 1

2 years ago
Here is the zooming better view:
https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,7f20e2504c722e15e70fde0c06c926f946d6022c,1,1%5D&series=%5Bmozilla-inbound,f91449d883e157f8cb3ce5d6e6ac3144d7b30689,1,1%5D&zoom=1468621207973.146,1468727078782.256,60,90

This issue might be caused by a80fdfc128b0c29dc34dd3fc28868252111a5d52.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=469d01eebea4e2055553289ce6542fc093460bbd&tochange=a80fdfc128b0c29dc34dd3fc28868252111a5d52

Hi Till, as you are the patch author, can you take a look at this and determine what is the root cause? Thanks!
Blocks: 1280481, 911216
Flags: needinfo?(till)
Keywords: perf, regression, talos-regression
(Assignee)

Comment 2

2 years ago
I'll take a look at this as soon as I can. As I'm on PTO right now, I don't have a good way to investigate. 

If at all possible, please don't back out bug 911216 over this: it tends to bitrot quickly and landing it was pretty hard. I'll definitely investigate this regression and fix it in-place (with an uplift to aurora).
Flags: needinfo?(till)
thanks Till, we will follow up when you are back next week.  As a note, this will be on Aurora next week, so lets remember to uplift any fixes to Aurora :)
status-firefox47: --- → unaffected
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
this also shows up on osx:
Summary of tests that regressed:

  tabpaint summary osx-10-10 opt - 6.85% worse
(Reporter)

Comment 5

2 years ago
This regression patch a80fdfc128b0 has been merged into Aurora, so please make sure to uplift any fixes to Aurora, thank you.

https://treeherder.mozilla.org/perf.html#/alerts?id=2185

  tabpaint summary osx-10-10 opt - 5.5% worse
it is interesting that the linux64 regression didn't show up initially, looking at the graph, I also see a 8.24% regression on linux64 when we merged to Aurora.

:till, as you are back from vacation, can you please look at this bug and give a timeline of when you can look into this?
Flags: needinfo?(till)
(Assignee)

Comment 7

2 years ago
(In reply to Joel Maher ( :jmaher ) from comment #6)
> it is interesting that the linux64 regression didn't show up initially,
> looking at the graph, I also see a 8.24% regression on linux64 when we
> merged to Aurora.
> 
> :till, as you are back from vacation, can you please look at this bug and
> give a timeline of when you can look into this?

I did start looking into it, but it's not easy to figure out what's going on here.

While some tabpaint tests have regressed others, such as the win32 ones, are entirely unchanged; osx-10-10 opt e10s has even improved substantially.

Win64 pgo is puzzling: the non-e10s version might've become a bit more noisy, but is largely unchanged. The e10s version, OTOH, improved drastically. The weird thing is that it improved with the first landing of bug 911216 part 30 on the 12th and didn't regress when that was backed out.


I have three different hypotheses on what's going on here, but at least the two most actionable of them have serious problems:

1. The new Promise implementation allocates more memory, causing more memory pressure, more GCs, decreased performance.

This would only seem plausible if there were a lot of promises created during the tabpaint test. I checked, each test run creates about 100 promises, which IMO pretty much rules out this explanation.

2. The new Promise implementation causes Promise reactions (i.e., callbacks supplied to a Promise#then() call) to be invoked later than with the old implementation in some circumstances.

This should result in a slowdown across all platforms and configurations, as it'd be a change in high-level logic, which compilers shouldn't have any influence on. At the very least, it's not compatible with some configurations actually seeing a speedup.

3. The substantially different code paths cause completely different results in the compilers' inlining heuristics, resulting in different performance for some code.

The other two explanations ruled out, this seems to be remaining.

I'm working on some memory usage optimizations which we should land, and probably uplift to Aurora, regardless. They might or might not help, but certainly won't hurt. If they don't help, I'm at a bit of a loss here, I'm afraid.
Flags: needinfo?(till)
(Assignee)

Comment 8

2 years ago
Boris, maybe you have any other ideas on what's happening here?
Flags: needinfo?(bzbarsky)
Not offhand.  Of your list of three things, the second one seems most plausible to me.  If there are tests that regress reliably and the regression can be reproduced on try, it might be worth adding some logging to see when promises invoke their callbacks in both the spidermonkey promise setup and the DOM promise setup to see whether something jumps out at us...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 10

2 years ago
I have an update to this, though no solution yet. While the data is quite noisy, instrumented builds do show that with the new Promise implementation some promises created during tabpaint are resolved later. Both in terms of Milliseconds and, more importantly, in terms of micro-task checkpoints between Promise creation and resolution. Now to find out *why* that happens ...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8781112 - Attachment is obsolete: true
Attachment #8781112 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781113 - Attachment is obsolete: true
Attachment #8781113 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781114 - Attachment is obsolete: true
Attachment #8781114 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781115 - Attachment is obsolete: true
Attachment #8781115 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781116 - Attachment is obsolete: true
Attachment #8781116 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781117 - Attachment is obsolete: true
Attachment #8781117 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781118 - Attachment is obsolete: true
Attachment #8781118 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
Attachment #8781119 - Attachment is obsolete: true
Attachment #8781119 - Flags: review?(efaustbmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

2 years ago
(Sorry for the churn, I hadn't realized that I need to re-push all mozreview requests if I want to update just one of them.)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8781525 [details]
Bug 1289318 - Part 1: Store contents of spec-defined `capabilities` struct in Promise reaction jobs directly.

https://reviewboard.mozilla.org/r/71932/#review71060

::: js/src/builtin/Promise.js:890
(Diff revision 1)
>  
>      let incumbentGlobal = _GetObjectFromIncumbentGlobal();
>      // Step 5.
>      let fulfillReaction = {
>          __proto__: PromiseReactionRecordProto,
> -        capabilities: resultCapability,
> +        resolve: resultCapability.resolve,

This means that we have to do some plumbing here when we add new things to reactions objects, instead of just passing their capabilities through. Is it worth documenting these places in a "// NB:" style comment?
Attachment #8781525 - Flags: review?(efaustbmo) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8781526 [details]
Bug 1289318 - Part 2: Make Promise reaction records their own object type with a constructor and all.

https://reviewboard.mozilla.org/r/71934/#review71066

Much cleaner.
Attachment #8781526 - Flags: review?(efaustbmo) → review+

Comment 33

2 years ago
mozreview-review
Comment on attachment 8781527 [details]
Bug 1289318 - Part 3: Merge Promise fulfillment and rejection reaction lists into a single list.

https://reviewboard.mozilla.org/r/71936/#review71070

A nice complexity reduction.
Attachment #8781527 - Flags: review?(efaustbmo) → review+

Comment 34

2 years ago
mozreview-review
Comment on attachment 8781528 [details]
Bug 1289318 - Part 4: Only allocate the Promise reactions array once the first reaction record is added.

https://reviewboard.mozilla.org/r/71938/#review71088

It would be interesting to consider having a helper to do the set and assert that the state is always pending, but I think it's vacuously true for this.
Attachment #8781528 - Flags: review?(efaustbmo) → review+

Comment 35

2 years ago
mozreview-review
Comment on attachment 8781529 [details]
Bug 1289318 - Part 5: Port most Promise functions directly involved in Promise resolution from JS to C++.

https://reviewboard.mozilla.org/r/71940/#review71096

We talked about other possible performance investigations here, with moving some of these things from properties to reserved slots, and the addition of a new JSClass. I wonder if it's faster. Do you think it's worth looking into in the future? If so, please file a followup.

::: js/src/builtin/Promise.cpp:110
(Diff revision 1)
> +        if (!GetProperty(cx, reaction, reaction, handlerName, &handler))
> +            return false;
> +        MOZ_ASSERT((handler.isNumber() &&
> +                    (handler.toNumber() == PROMISE_HANDLER_IDENTITY ||
> +                     handler.toNumber() == PROMISE_HANDLER_THROWER)) ||
> +                   handler.toObject().isCallable());

For readability, I would *vastly* prefer to expand these asserts.

MOZ_ASSERT_IF(!handler.isObject(), handler.isNumber());
MOZ_ASSERT_IF(handler.isNumber(), handler.toNumber() == PROMISE_HANDLER_IDENTITY ||
                                  handler.toNumber() == PROMISE_HANDLER_THROWER);
MOZ_ASSERT_IF(handler.isObject(), handler.toObject().isCallable());

or even

#ifdef DEBUG
if (handler.isNumber()) {
    MOZ_ASSERT(handler.toNumber() == PROMISE_HANDLER_IDENTITY || handler.toNumber() == PROMISE_HANDLER_THROWER);
} else {
    MOZ_ASSERT(handler.isObject());
    MOZ_ASSERT(handler.toObject().isCallable());
}
#endif

or something. Merging them, even with the parens and indentation, took me a moment too long to parse.

::: js/src/builtin/Promise.cpp:581
(Diff revision 1)
>      //   promise: [the promise this reaction resolves],
>      //   resolve: [the `resolve` callback content code provided],
>      //   reject:  [the `reject` callback content code provided],
> -    //   handler: [the internal handler that fulfills/rejects the promise]
> +    //   fulfillHandler: [the internal handler that fulfills the promise]
> +    //   rejectHandler: [the internal handler that rejects the promise]
> +    //   incumbentGlobal: [an object from the global that was incumbent when

This maybe belong elsewhere in the stack? looks like an easrlier part.

::: js/src/tests/ecma_6/Promise/promise-basics.js:23
(Diff revision 1)
>  
> +new Promise((res, rej)=> {res('result');  rej('rejection'); })
> +  .catch(val=>{throw new Error("mustn't be called");})
> +  .then(val=>results.push('then after resolve+reject with val: ' + val),
> +        val=>{throw new Error("mustn't be called")});
> +  

nit: whitespace on blank line.
Attachment #8781529 - Flags: review?(efaustbmo) → review+

Comment 36

2 years ago
mozreview-review
Comment on attachment 8781530 [details]
Bug 1289318 - Part 6: Don't store a reference to the reject function on Promise instances themselves.

https://reviewboard.mozilla.org/r/71942/#review71128

Nice reduction.

::: js/src/builtin/Promise.cpp:685
(Diff revision 1)
>  {
>      if (this->getFixedSlot(PROMISE_STATE_SLOT).toInt32() != unsigned(JS::PromiseState::Pending))
>          return true;
>  
>      RootedValue funVal(cx, this->getReservedSlot(PROMISE_RESOLVE_FUNCTION_SLOT));
> +    // TODO: ensure that this holds for xray'd promises. (It probably doesn't)

Wow. That's a scary comment. Did anything ever happen with this? Does it need to be addressed?
Attachment #8781530 - Flags: review?(efaustbmo) → review+

Comment 37

2 years ago
mozreview-review
Comment on attachment 8781531 [details]
Bug 1289318 - Part 7: Store the Promise reactions list in the same slot as the result.

https://reviewboard.mozilla.org/r/71944/#review71132

I guess there are already asserts in the rest of the places that expect results that state != pending. If not, please add.
Attachment #8781531 - Flags: review?(efaustbmo) → review+

Comment 38

2 years ago
mozreview-review
Comment on attachment 8781532 [details]
Bug 1289318 - Part 8: Combine Promise state and rejection handling info into a single flags field.

https://reviewboard.mozilla.org/r/71946/#review71136

Works for me.

::: js/src/builtin/Promise.cpp:734
(Diff revision 1)
>          }
>      }
>      promise->setFixedSlot(PROMISE_RESOLUTION_SITE_SLOT, ObjectOrNullValue(stack));
>      promise->setFixedSlot(PROMISE_RESOLUTION_TIME_SLOT, DoubleValue(MillisecondsSinceStartup()));
>  
> -    if (promise->state() == JS::PromiseState::Rejected &&
> +    if (promise->state() == JS::PromiseState::Rejected && promise->markedAsUncaught())

nit: mild preference for some word other than "marked". I visually grepped this as "markAsUncaught" first.
Attachment #8781532 - Flags: review?(efaustbmo) → review+

Comment 39

2 years ago
mozreview-review
Comment on attachment 8781120 [details]
Bug 1289318 - Part 9: Port Promise.resolve and Promise.reject to C++ and optimize various common cases.

https://reviewboard.mozilla.org/r/71622/#review71176

::: js/src/tests/ecma_6/Promise/promise-basics.js:40
(Diff revision 2)
>  assertEq(results[4], 'then after catch with val: 2');
>  assertEq(results[5], 'then after resolve+reject with val: result');
>  
> +results = [];
> +
> +Promise.resolve('resolution').then(res=>results.push(res), 

nit: whitespace at end of line, and on line 23
Attachment #8781120 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 40

2 years ago
mozreview-review-reply
Comment on attachment 8781529 [details]
Bug 1289318 - Part 5: Port most Promise functions directly involved in Promise resolution from JS to C++.

https://reviewboard.mozilla.org/r/71940/#review71096

> This maybe belong elsewhere in the stack? looks like an easrlier part.

Not really: I forgot to amend the comment when adding the `incumbentGlobal` field in a patch that has already landed.

Comment 41

2 years ago
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ceb37552a97
Part 1: Store contents of spec-defined `capabilities` struct in Promise reaction jobs directly. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58a19199304
Part 2: Make Promise reaction records their own object type with a constructor and all. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/436d0a7bcb43
Part 3: Merge Promise fulfillment and rejection reaction lists into a single list. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b97a1aa491a
Part 4: Only allocate the Promise reactions array once the first reaction record is added. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab4d5b055e4
Part 5: Port most Promise functions directly involved in Promise resolution from JS to C++. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46d3d4e8c84
Part 6: Don't store a reference to the reject function on Promise instances themselves. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3c34e6074a
Part 7: Store the Promise reactions list in the same slot as the result. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/e819902b13a9
Part 8: Combine Promise state and rejection handling info into a single flags field. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd28f500db07
Part 9: Port Promise.resolve and Promise.reject to C++ and optimize various common cases. r=efaust
Assignee: nobody → till
I have verified this on the graphs, are we interested in uplifting this to Aurora where the regression was introduced?
Depends on: 1298776
(In reply to Joel Maher ( :jmaher) from comment #43)
> I have verified this on the graphs, are we interested in uplifting this to
> Aurora where the regression was introduced?
Flags: needinfo?(till)
Naveed, what do you think? Thanks
Flags: needinfo?(nihsanullah)
These patches are pretty big to uplift to beta IMO.
Yes Till and I agree this is too big too uplift.
Flags: needinfo?(nihsanullah)
status-firefox50: affected → wontfix
Flags: needinfo?(till)
Depends on: 1339999
You need to log in before you can comment on or make changes to this bug.