Closed Bug 1300401 Opened 8 years ago Closed 8 years ago

2.87 - 4.15% cart (linux64) regression on push 958201c5ca69 (Fri Sep 2 2016)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: jmaher, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: perf, regression, talos-regression)

Attachments

(3 files, 1 obsolete file)

Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1300401 - Part 2. Add more comments and assertions.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1300401 - Part 3. Reftest for combining mask, filter and opacity.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Talos has detected a Firefox performance regression from push 958201c5ca69. As author of one of the patches included in that push, we need your help to address this regression. Regressions: 4% cart summary linux64 opt 36.86 -> 38.39 3% cart summary linux64 pgo 29.02 -> 29.97 3% cart summary linux64 opt e10s 39.48 -> 40.76 3% cart summary linux64 pgo e10s 30.65 -> 31.53 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=2948 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
This seems to be a lot of patches landed at once, but retriggers shows that push is the culprit, here is a look at a compare view: https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=dea9d3f8bfe0&newProject=autoland&newRevision=958201c5ca697530f9b07c18dc0c6975c1a415e9&framework=1&filter=cart%20linux&showOnlyImportant=0 and pretty much all the subtests regressed equally. :cjku, can you look at this bug and determine the root cause and how we should resolve it? If you want help, we can push to try and bisect it down to the specific patch, but we would need to know that each patch is independent of the other patches.
Flags: needinfo?(cku)
I think the reason should be here - bug 1234485 comment 5: ...If an element has both a clip-path / mask and a filter, the separation might conceivably hurt performance a bit, but I don't think that's worth worrying about much. That says if you have both clip-path and filtler css attribute specific on the same element, the painting performance of the new version will be slower a bit. These patches are the first step of performance enhancement of clip-path and mask, my next target is to make clip-path and css-mask drawn on the mask layer(in bug 1234485), there, we will take every loss back and get more, especially for css-animation. bug 1234485 should be done a 6-weeks work.
We should check whether there actually is an element in the customize toolbar animation that uses both a mask and a filter.
(In reply to Markus Stange [:mstange] from comment #3) > We should check whether there actually is an element in the customize > toolbar animation that uses both a mask and a filter. I think so, browser.css uses both clip-path and filter a lot. But I will confirm it later.
Not clip-path but opacity. We divided nsDisplaySVGEffect into nsDisplayMask & nsDisplayFilter. nsDisplayMask is in charge of opacity/ clip-path and mask, and nsDisplayFilter is for painting filter only. In this case, I can do logic change in nsFrame::BuildDisplayListForStackingContext 1. Apply opacity in nsDisplayFilter if we create nsDisplayFilter only 2. Apply opacity in nsDisplayMask if we create nsDisplayMask only. 3. Apply opacity in nsDisplayMask if we create both nsDisplayMask and nsDisplayFilter. And, pass a parameter to nsSVGIntegrationUtils::PaintFilter to tell whether apply opacity. Code change should be minor.
Assignee: nobody → cku
Flags: needinfo?(cku)
Attachment #8788054 - Flags: review?(mstange)
Attachment #8788055 - Flags: review?(mstange)
Component: Untriaged → Layout
Product: Firefox → Core
thank you for figuring this out and getting a patch uploaded so quickly!
Comment on attachment 8788055 [details] Bug 1300401 - Part 2. Add more comments and assertions. https://reviewboard.mozilla.org/r/76624/#review74840
Attachment #8788055 - Flags: review?(mstange) → review+
Comment on attachment 8788054 [details] Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter. https://reviewboard.mozilla.org/r/76622/#review74842 This is becoming quite complicated. I wonder if implementing nsDisplayFilter::ApplyOpacity and nsDisplayMask::ApplyOpacity would make things simpler. I'll continue reviewing this tomorrow unless you want to try the ApplyOpacity approach. Do we have a reftest with an element that has opacity, a mask, and a filter, and which tests that we only apply the opacity once? I think such a test would be good to have.
On the other hand, Matt Woodrow tried the ApplyOpacity approach before in bug 1263829 comment 4 but then discarded it again. I'm not really sure why.
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox51: --- → affected
Version: 49 Branch → 51 Branch
(In reply to Markus Stange [:mstange] from comment #18) > Comment on attachment 8788054 [details] > Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter. > > https://reviewboard.mozilla.org/r/76622/#review74842 > > This is becoming quite complicated. I wonder if implementing > nsDisplayFilter::ApplyOpacity and nsDisplayMask::ApplyOpacity would make > things simpler. I do not have experience on ApplyOpacity before. Read code briefly, I think it may work. I will rework this patch tomorrow, thank for this advice > I'll continue reviewing this tomorrow unless you want to try the > ApplyOpacity approach. > Do we have a reftest with an element that has opacity, a mask, and a filter, > and which tests that we only apply the opacity once? I think such a test > would be good to have. No idea. But I will make sure we have it.
Comment on attachment 8788054 [details] Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter. https://reviewboard.mozilla.org/r/76622/#review74842 nsDisplayXXX::ApplyOpacity is called from nsDisplayOpacity::ShouldFlattenAway(), than means we need to have nsDisplayOpacity first. But nsDisplayMask/Filter and nsDisplayOpacity is kind of mutual exclusive. http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2475 useOpacity is most likely false if we have any SVG effects. So, I think this path may not work
Attachment #8788446 - Flags: review?(mstange)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #21) > nsDisplayXXX::ApplyOpacity is called from > nsDisplayOpacity::ShouldFlattenAway(), than means we need to have > nsDisplayOpacity first. But nsDisplayMask/Filter and nsDisplayOpacity is > kind of mutual exclusive. > > http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2475 > useOpacity is most likely false if we have any SVG effects. Yes, changing this would be part of the ApplyOpacity approach. But we can leave that to another bug. I'll review these patches later today.
Comment on attachment 8788054 [details] Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter. https://reviewboard.mozilla.org/r/76622/#review74842 In the new version, I replace opacityItemCreated by handleOpacity, which is easier to understand what it means when reading nsDisplaySVFEffects/nsSVGIntegrationUtil::PaintFilter and PanitMaskAndClipPath. No new data member introduced in this version.
Attachment #8788446 - Attachment is obsolete: true
Attachment #8788446 - Flags: review?(mstange)
Attachment #8788454 - Flags: review?(mstange)
Comment on attachment 8788054 [details] Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter. https://reviewboard.mozilla.org/r/76622/#review75150 Thanks, this is a bit simpler. I'm not quite sure why it's faster, though.
Attachment #8788054 - Flags: review?(mstange) → review+
Comment on attachment 8788454 [details] Bug 1300401 - Part 3. Reftest for combining mask, filter and opacity. https://reviewboard.mozilla.org/r/76960/#review75154 ::: layout/reftests/w3c-css/submitted/masking/mask-opacity-1a.html:25 (Diff revision 1) > + top: 10px; > + background-color: rgb(255,255,0); > + width: 100px; > + height: 100px; > + filter: invert(100%); > + mask-image: url(support/blue-100x50-transparent-100x50.png); Was this meant to be url(#myMask)? Same for the 1b test. ::: layout/reftests/w3c-css/submitted/masking/reftest.list:81 (Diff revision 1) > +== mask-opacity-1a.html mask-opacity-1-ref.html > +== mask-opacity-1b.html mask-opacity-1-ref.html > +== mask-opacity-1c.html mask-opacity-1-ref.html Tests that use opacity:0.5 usually require a fuzz of 1 on at least one of the platforms.
Comment on attachment 8788454 [details] Bug 1300401 - Part 3. Reftest for combining mask, filter and opacity. https://reviewboard.mozilla.org/r/76960/#review75156
Attachment #8788454 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #33) > Comment on attachment 8788054 [details] > Bug 1300401 - Part 1. Handle opacity in nsDisplayFilter. > > https://reviewboard.mozilla.org/r/76622/#review75150 > > Thanks, this is a bit simpler. > I'm not quite sure why it's faster, though. div { opacity: 0.5; filter: XXX; } Before this change,nsIFrame::BuildDisplayListForStackingContext would create one nsDisplayFilter(for filter css property) and one nsDisplayMask(for css opacity). Which means two indirect painting is required. After this change, only one ndDisplayFilter will be created. Only one indirect painting needed.
Oh, it's because nsDisplayMask would create a mask surface and do a real masking operation. The PushGroup call you're adding in nsDisplayFilter rendering creates an intermediate surface behind the scenes, so you're adding one "indirect painting" to the filter path. But it only uses opacity and not a mask, so it's much cheaper.
verified fixed in perfherder! Thanks for fixing this so fast:)
(In reply to Joel Maher ( :jmaher) from comment #42) > verified fixed in perfherde
Blocks: mask-ship, mask-image
(In reply to Joel Maher ( :jmaher) from comment #42) > verified fixed in perfherder! Thanks for fixing this so fast:) no problem :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: