Closed
Bug 1453944
Opened 7 years ago
Closed 6 years ago
3.57 - 4.17% displaylist_mutate (windows10-64, windows7-32) regression on push 665843c6cfd1a3049128597afbef8a68bcc1e086 (Thu Apr 12 2018)
Categories
(Core :: Web Painting, defect, P2)
Core
Web Painting
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | wontfix |
firefox62 | + | wontfix |
People
(Reporter: igoldan, Assigned: mikokm)
References
Details
(Keywords: perf, regression, talos-regression)
Talos has detected a Firefox performance regression from push: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=665843c6cfd1a3049128597afbef8a68bcc1e086 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 4% displaylist_mutate windows7-32 pgo e10s stylo 4,594.14 -> 4,785.61 4% displaylist_mutate windows10-64 pgo e10s stylo 4,032.12 -> 4,175.96 Improvements: 10% tsvgr_opacity windows7-32 pgo e10s stylo 111.50 -> 100.61 8% tsvgr_opacity windows10-64 pgo e10s stylo 107.71 -> 99.00 2% rasterflood_gradient windows7-32 pgo e10s stylo1,334.75 -> 1,361.58 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=12680 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 | ||
Updated•7 years ago
|
Component: Untriaged → Layout: Web Painting
Product: Firefox → Core
Reporter | ||
Comment 1•7 years ago
|
||
:miko Can we do something to fix these perf regressions? Or should we consider accepting them?
Flags: needinfo?(mikokm)
Assignee | ||
Comment 2•7 years ago
|
||
Changes that landed with bug 1442190 specifically targeted opacity display item painting performance, as shown on by Talos tests. It seems that these changes also added some overhead, most likely to display list iteration, where we now allocate and return a pair (display item pointer and start/end/item marker) instead of just a pointer. There is still plenty of room to improve this code, so I would prefer to accept this performance regression for now, and improve things in a follow-up bug.
Flags: needinfo?(mikokm)
Comment 3•7 years ago
|
||
What kind of timeframe do you expect that follow-up work to take place in? Can we use this bug to track improving that performance? :)
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox61:
--- → +
Flags: needinfo?(mikokm)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3) > What kind of timeframe do you expect that follow-up work to take place in? > Can we use this bug to track improving that performance? :) Bug 1439292 tracks the work we are doing to reduce (and hopefully later eliminate) the use of inactive layers, which have a huge performance overhead. Bug 1442190 covered the opacity item flattening, and next up are the transform items. This will probably cause a lot of refactoring around this same code, which is why I think these optimizations should be postponed for a while. Timeframe is probably >1 month due to my upcoming PTO. I will do some profiling later this week to see if there are some immediate easy wins here, such as, reducing the allocations for temporary items, or having a separate code path for display lists without opacity items.
Flags: needinfo?(mikokm)
Comment 5•7 years ago
|
||
That sounds good, thanks for the info. Let's leave the bug open for now pending that profiling work to see if there's any low-risk low-hanging fruit we can land to help mitigate the regression for now.
Updated•7 years ago
|
Priority: -- → P2
Not sure we need to track this.
Updated•7 years ago
|
Assignee: nobody → mikokm
Assignee | ||
Comment 8•7 years ago
|
||
Sadly, I haven't been able to look into this yet. I have been busy fixing the other issues from this same change.
Updated•6 years ago
|
status-firefox62:
--- → affected
tracking-firefox62:
--- → +
Updated•6 years ago
|
Assignee | ||
Comment 10•6 years ago
|
||
Sorry for keeping this open for so long, I just got back from PTO. The changes in bug 1460624 give me around 2% improvement in displaylist_mutate test (locally on MBP). I tried testing a couple of other micro-optimizations, but they yielded results that were within the benchmark error margin (<0.5-1%). As mentioned in comment 4, I'm going to start working on transform flattening next, which involves touching this code. As such, I'm closing this as WONTFIX.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•