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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jmaher, Assigned: u459114)

Tracking

(Blocks 1 bug, {perf, regression, talos-regression})

51 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50 unaffected, firefox51 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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