Closed Bug 1080688 Opened 5 years ago Closed 5 years ago

2.4% Win8 SVG Opacity regression on inbound (v.35) Oct 3 from push 7c26c6d5b2fb

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: jmaher, Assigned: jwatt)

References

(Depends on 1 open bug)

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(1 file)

This was caused by bug 934183, part 1:

  http://hg.mozilla.org/integration/mozilla-inbound/rev/2f7b5e239d1e

The svg opacity tests are very rect heavy (in fact entirely based on rects) and gfxContext has optimizations for rect specifically. Previously we used to call gfxContext::Rectangle, which stores a Rect, and then call gfxContext::GetUserPathExtent, which just does a simple transformation of the stored Rect. Now we create a Moz2D Path object and do the calculations via that.

I just landed a fix for bug 1074161 where I introduced a mechanism to avoid creating Moz2D Path objects for painting. We should leverage this new GetSimplePath() mechanism in nsSVGPathGeometryFrame::GetBBoxContribution to fix this regression.
Assignee: nobody → jwatt
No longer blocks: 1075399
Attached patch patchSplinter Review
Actually let's do this in a way that is more bug 1034958 friendly, since I want to fix that soon too.
Attachment #8509501 - Flags: review?(longsonr)
Everything inside the |if (!gotSimpleBounds) {| block is just increasing the indentation (well, except for the use of the factored out |getFill| and |getStroke| variables.
In SVGRectElement::GetGeometryBounds the Inflate should be by aStrokeWidth/2. Fixed that locally.
Comment on attachment 8509501 [details] [diff] [review]
patch

I did catch the stroke width issue too but then so did you :-)

> 
>+inline bool HasNonScalingStroke(nsIFrame* aFrame) {
>+  return aFrame->StyleSVGReset()->mVectorEffect ==
>+    NS_STYLE_VECTOR_EFFECT_NON_SCALING_STROKE;
>+}
>+

Better in StyleSVGReset as a const member function. I guess I can live with it as is though.


nsSVGPathGeometryFrame::GetBBoxContribution could really do with being split up into smaller pieces e.g. have separate getSimpleBounds and getComplexBounds functions, call the first and if it returns false call the second. That would make the logic easier to follow. This could be a follow up though.
Attachment #8509501 - Flags: review?(longsonr) → review+
Comment on attachment 8509501 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/f51420708a03

I moved the HasNonScalingStroke() function to StyleSVGReset as you suggested.

I'm looking at splitting and cleaning up GetBBoxContribution too, although I want to move much of this to content so that we can get bounds without a frame tree.
Attachment #8509501 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/f51420708a03
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
any chance this could land on Aurora?
Flags: needinfo?(jwatt)
We got some decent wins on the 'svg opacity' tests from this:

http://graphs.mozilla.org/graph.html#tests=[[225,131,31],[225,131,25],[225,131,35],[225,131,37],[225,63,21],[225,63,24]]&sel=1414244738397.378,1414388462799.2917,60,513.3333333333334&displayrange=7&datatype=running

Roughly:

  7% on Win 8
 10% on Win 7
  4% on Win XP

Not much change on the Mac or Linux runs.
Flags: needinfo?(jwatt)
(In reply to Joel Maher (:jmaher) from comment #8)
> any chance this could land on Aurora?

It wouldn't apply cleanly, but it could probably be safely landed with some hand merging. That said, while the wins are nice, the 2.4% regression on a few synthetic tests is probably not something to worry about as long as we know a fix is coming. I don't think we need to rush this to Aurora unless there's some B2G milestone or something I don't know about that would otherwise miss getting these improvements.
agreed, lets leave this alone.e the wins are nice, the 2.4% regression on a few
synthetic tests is probably not something to worry about as long as we know a
fix is coming. I don't think we need to rush this to Aurora unless there's some
B2G milestone or something I don't know about that would otherwise miss getting
these improvements.
Depends on: 1089768
Comment on attachment 8509501 [details] [diff] [review]
patch

Requesting approval so that we can land bug 1099197 on Aurora. This should be low risk.
Attachment #8509501 - Flags: approval-mozilla-aurora?
Comment on attachment 8509501 [details] [diff] [review]
patch

approving for aurora because of the need in bug 1099197
Attachment #8509501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 1322643
Depends on: 1322643
You need to log in before you can comment on or make changes to this bug.