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

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect, P1)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 2

[André Bargull [:anba]]

bug 1303091 should help to fix the performance regression by sharing time zone data across multiple compartments.

Comment 3

[Alison Shiue]

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

Comment 4

[André Bargull [:anba]]

(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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

What's the status of this now?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

On trunk, I see what appears to be the tabpaint regressions fixed, but the tart/ts_paint regressions still are there.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Any thoughts on next steps here, André?

Comment 8

[André Bargull [:anba]]

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.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 10

[André Bargull [:anba]]

(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.

Comment 11

[Justin Dolske [:Dolske]]

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.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

tabpaint win7 - 1/2 fixed:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,0429a33ec5c1d5fc657faf0f1880ba5d6574b48d,1,1%5D&series=%5Bautoland,0429a33ec5c1d5fc657faf0f1880ba5d6574b48d,1,1%5D&zoom=1476616157946.7212,1484142565932.377,83.8252780162712,100.47955310920801

tabpaint win8 - 1/2 fixed:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,34db89ecacd36642ee313b1611357b8315ca163d,1,1%5D&series=%5Bautoland,34db89ecacd36642ee313b1611357b8315ca163d,1,1%5D&zoom=1476441125202.869,1484158478000,82.82341967728058,94.92379142449248

tart win8 - no fix, more regressions:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,5d06c7780d3bdfc9f446cc1b6f9894d8d06081e4,1,1%5D&series=%5Bautoland,5d06c7780d3bdfc9f446cc1b6f9894d8d06081e4,1,1%5D&zoom=1476468394536.232,1484056454618.3574,6.29851299413518,7.397026005287596

ts_paint win7 - was found after reporting, no fix and slow creep:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=%5Bmozilla-inbound,2b1d3ea8736e7fadcd5c3aec5749366bc1770257,1,1%5D&series=%5Bautoland,2b1d3ea8736e7fadcd5c3aec5749366bc1770257,1,1%5D&zoom=1476496639686.0842,1484156462000,933.7546309573943,1209.5910621841601

Comment 13

[Justin Dolske [:Dolske]]

André/Waldo: is there a plan for fixing the remaining regression?

Comment 14

[André Bargull [:anba]]

(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

Comment 15

[Astley Chen (inactive)]

[Tracking Requested - why for this release]: Pref regression that needs to be tracked carefully.

Comment 16

[Julien Cristau [:jcristau]]

Tracking as new perf regression in 52.

Comment 17

[Astley Chen (inactive)]

(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?

Comment 18

[André Bargull [:anba]]

(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.

Comment 19

[Astley Chen (inactive)]

(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?

Comment 20

[André Bargull [:anba]]

(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.

Comment 21

[Astley Chen (inactive)]

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.

Comment 22

[André Bargull [:anba]]

(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.