Closed
Bug 1314354
Opened 8 years ago
Closed 8 years ago
2.02 - 11.09% tabpaint / tart (linux64, windows7-32, windows8-64, windowsxp) regression on push f4459d4ba9ba (Fri Oct 28 2016)
Categories
(Core :: JavaScript: Internationalization API, defect, P1)
Core
JavaScript: Internationalization API
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | + | fixed |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
People
(Reporter: jmaher, Unassigned)
References
Details
(Keywords: perf, regression, talos-regression)
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•8 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)
Comment 2•8 years ago
|
||
bug 1303091 should help to fix the performance regression by sharing time zone data across multiple compartments.
Comment 3•8 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
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
What's the status of this now?
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Component: Untriaged → JavaScript: Internationalization API
Flags: needinfo?(jmaher)
Product: Firefox → Core
Version: unspecified → Trunk
Reporter | ||
Comment 6•8 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)
Comment 8•8 years ago
|
||
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•8 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.
Comment 10•8 years ago
|
||
(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•8 years ago
|
||
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.
Reporter | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
||
André/Waldo: is there a plan for fixing the remaining regression?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(andrebargull)
Comment 14•8 years ago
|
||
(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)
Comment 15•8 years ago
|
||
[Tracking Requested - why for this release]: Pref regression that needs to be tracked carefully.
Comment 17•8 years ago
|
||
(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)
Comment 18•8 years ago
|
||
(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)
Comment 19•8 years ago
|
||
(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)
Comment 20•8 years ago
|
||
(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)
Comment 21•8 years ago
|
||
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)
Comment 22•8 years ago
|
||
(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
Closed: 8 years ago
Flags: needinfo?(andrebargull)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•