2.02 - 11.09% tabpaint / tart (linux64, windows7-32, windows8-64, windowsxp) regression on push f4459d4ba9ba (Fri Oct 28 2016)

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jmaher, Unassigned)

Tracking

({perf, regression, talos-regression})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53 fixed, firefox54 fixed)

Details

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

Regressions:

 11%  tabpaint summary windowsxp pgo       68.94 -> 76.59
  6%  tart summary linux64 pgo             6.54 -> 6.91
  5%  tabpaint summary windowsxp opt       94.62 -> 99.33
  4%  tabpaint summary windows7-32 opt     89.42 -> 93.12
  4%  tabpaint summary windows8-64 opt     86.04 -> 89.53
  2%  tart summary windows8-64 opt         6.46 -> 6.6
  2%  tart summary windows8-64 pgo         5.12 -> 5.23


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

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

3 years ago
Andre, I see this landed, was backed out, fixed and landed again.  We have detected a performance regression with this on a few tests as noted above.  Can you take a look at this and help us determine why these performance regressions are occurring and if we can do anything to fix them or need to accept this regression?
Flags: needinfo?(andrebargull)
Depends on: 1303091
Flags: needinfo?(andrebargull)
bug 1303091 should help to fix the performance regression by sharing time zone data across multiple compartments.

Comment 3

3 years ago
As a note, this regression patch has been merged into Aurora, so please make sure to uplift any fixes to Aurora, thank you.

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4129
(In reply to Alison Shiue from comment #3)
> As a note, this regression patch has been merged into Aurora, so please make
> sure to uplift any fixes to Aurora, thank you.

Aurora approval request: https://bugzilla.mozilla.org/show_bug.cgi?id=1303091#c17
What's the status of this now?
Component: Untriaged → JavaScript: Internationalization API
Flags: needinfo?(jmaher)
Product: Firefox → Core
Version: unspecified → Trunk
Reporter

Comment 6

2 years ago
On trunk, I see what appears to be the tabpaint regressions fixed, but the tart/ts_paint regressions still are there.
Flags: needinfo?(jmaher)
Any thoughts on next steps here, André?
Flags: needinfo?(andrebargull)
Does each tart/ts_paint test restart the complete browser, or are the tests repeated within an already running browser instance? If the browser is restarted every time, the patch in bug 1303091 won't help here. (bug 1303091 stores the calculated time zone data within the JSRuntime class so we can share it across compartments.)

Another option to further reduce the time needed to initialize the Intl object, is to delay initializing the internal properties of the Intl prototypes (Intl.Collator.prototype, Intl.NumberFormat.prototype etc.). I can explore this idea more easily after bug 1328386, but I still need to investigate if this could lead to possible spec violations.
Flags: needinfo?(andrebargull)
Reporter

Comment 9

2 years ago
between each reported test, we kill the browser and create a fresh profile.  Within a test like tart, we collect many data points from all the subtests, in that case we run all the tart tests in the same session.

For tests like ts_paint and sessionrestore*, we restart the browser frequently, but those tests are specifically measuring the time to start the browser.
(In reply to André Bargull from comment #8)
> Another option to further reduce the time needed to initialize the Intl
> object, is to delay initializing the internal properties of the Intl
> prototypes (Intl.Collator.prototype, Intl.NumberFormat.prototype etc.). I
> can explore this idea more easily after bug 1328386, but I still need to
> investigate if this could lead to possible spec violations.

This test case:
---
// Measure the time needed to initialize the Intl object.
var intl = 0;
var t = Date.now();
for (var i = 0; i < 1000; ++i) {
    intl += newGlobal().eval("{ let t = Date.now(); this.Intl; Date.now() - t; }");
}
var total = Date.now() - t;
print(`total=${total}, intl=${intl}`);
---

currently reports "total=2600, intl=1240" for me. When I disable the calls to IntlInitialize in Create{Collator, DateTimeFormat, NumberFormat}Prototype, this improves to "total=1450, intl=50". So this idea seems to be worth exploring.


(In reply to Joel Maher ( :jmaher) from comment #9)
> between each reported test, we kill the browser and create a fresh profile. 
> Within a test like tart, we collect many data points from all the subtests,
> in that case we run all the tart tests in the same session.

Hmm, as long as the newly created compartments belong to the same JSRuntime, they should benefit from the changes in bug 1303091. For example when I replaced this line [1] with `tz = "UTC"` in order to remove the calls to the costly ICU time zone functions and then ran this test case:
---
var t = Date.now();
for (var i = 0; i < 1000; ++i) {
  newGlobal().eval("Intl.DateTimeFormat");
}
print(Date.now() - t);
---

I saw no performance changes, which means the cache from bug 1303091 works correctly and thus should have fixed the regressions from bug 837961.

[1] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/js/src/builtin/Intl.js#2373

> For tests like ts_paint and sessionrestore*, we restart the browser
> frequently, but those tests are specifically measuring the time to start the
> browser.

Okay, in that case we probably need to test the idea from comment #8.
What the current state of tabpaint/tart with the partial(?) fixes that have landed?

It's... not great that we've had this regression in the tree for more than two months at this point.
Depends on: 1331473
André/Waldo: is there a plan for fixing the remaining regression?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(andrebargull)
(In reply to Justin Dolske [:Dolske] from comment #13)
> André/Waldo: is there a plan for fixing the remaining regression?

I hope we can regain some performance with the changes in bug 1328386, bug 1331474, and bug 1331473. Results on try with patches for these bugs applied:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c2758bfe89c720b9ac0c34a5c56a4a1997abade0&newProject=try&newRevision=869f5cb6fb014ab16f8bc7104500e3028d40537d
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(andrebargull)
Depends on: 1332610
[Tracking Requested - why for this release]: Pref regression that needs to be tracked carefully.
Priority: -- → P1
Tracking as new perf regression in 52.
(In reply to André Bargull from comment #14)
> (In reply to Justin Dolske [:Dolske] from comment #13)
> > André/Waldo: is there a plan for fixing the remaining regression?
> 
> I hope we can regain some performance with the changes in bug 1328386, bug
> 1331474, and bug 1331473. Results on try with patches for these bugs applied:

André, from the try result, a 7.78% pref regression was still observed in 'tabpaint opt e10s' test.
Does it mean there is still something to be improved?
Flags: needinfo?(andrebargull)
(In reply to Astley Chen [:astley] (UTC+8) from comment #17)
> André, from the try result, a 7.78% pref regression was still observed in
> 'tabpaint opt e10s' test.
> Does it mean there is still something to be improved?

The results for linux64 in the try run from comment #14 are probably incorrect due to missing reruns (only a single run compared to six runs for osx-10-10/windows7-32). As soon as bug 1332610 has landed we should recheck the current performance numbers for the Talos tests.
Flags: needinfo?(andrebargull)
Depends on: 1332604
Depends on: 1328386
(In reply to André Bargull from comment #18)
> osx-10-10/windows7-32). As soon as bug 1332610 has landed we should recheck
> the current performance numbers for the Talos tests.

Per comment 14, looks like we still have bug 1331473 to go before completely fixing this regression?
Flags: needinfo?(andrebargull)
(In reply to Astley Chen [:astley] (UTC+8) from comment #19)
> (In reply to André Bargull from comment #18)
> > osx-10-10/windows7-32). As soon as bug 1332610 has landed we should recheck
> > the current performance numbers for the Talos tests.
> 
> Per comment 14, looks like we still have bug 1331473 to go before completely
> fixing this regression?

Yes, thanks for the remainder. I still need to prepare backports for Aurora/Beta with the changes from that bug. (Actually I think I'm now preferring a more minimal change and therefore will propose the change from bug 1333197 for backporting.) It's probably worth noting that the changes from 1332610/1333197 will only be applied for Aurora/Beta, because other infrastructural (but non-backportable) updates to the Intl classes make these changes unnecessary in Central.
Flags: needinfo?(andrebargull)
Depends on: 1333197
Hooray!
Looks like all changes in bug 1333197, bug 1332610, and bug 1333197 were all uplifted to aurora and beta, could you confirm that and update the status flags accordingly ?
Thanks.
Flags: needinfo?(andrebargull)
(In reply to Astley Chen [:astley] (UTC+8) from comment #21)
> Looks like all changes in bug 1333197, bug 1332610, and bug 1333197 were all
> uplifted to aurora and beta, could you confirm that and update the status
> flags accordingly ?

Confirmed, all depending bugs are fixed, therefore marking this bug as fixed, too.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(andrebargull)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.