Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Stop calling nsSVGUtils::InvalidateBounds for SVG transform changes, and use DLBI instead

RESOLVED FIXED in mozilla23

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

We should stop calling nsSVGUtils::InvalidateBounds for SVG transform changes, since DLBI should be handling that for us.
Created attachment 712245 [details] [diff] [review]
patch
Attachment #712245 - Flags: review?(matt.woodrow)

Comment 2

5 years ago
Is this likely to fix bug 817993?
I'm not sure.
Comment on attachment 712245 [details] [diff] [review]
patch

Actually, even this small piece of my patch for bug 827915 fails on:

reftests/svg/smil/motion/animateMotion-by-1.svg
reftests/svg/smil/motion/animateMotion-from-to-1.svg
reftests/svg/smil/motion/animateMotion-mpath-pathLength-1.svg
reftests/svg/smil/motion/animateMotion-mpath-targetChange-1.svg

It looks like the DLBI initiated invalidations are very infrequent, and the moving pieces don't get an invalidation for their final position.
Attachment #712245 - Flags: review?(matt.woodrow)
Created attachment 712292 [details]
testcase

This simpler testcase shows the problem clearly when run with vs without the attached patch.
I suspect that you need to still call nsIFrame::SchedulePaint in the cases where you have removed the invalidation. I don't think anything else on the AttributeChanged codepath will be doing this for you.

Note that most of the 'x' and 'y' attribute changes shouldn't need to invalidate either, the display item will move in those cases.
Yeah, adding 'SchedulePaint()' to nsSVGPathGeometryFrame::AttributeChanged fixes this bug.

You'll need to do this in every place that you remove invalidations, so that it actually triggers a paint (and DLBI).
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> I suspect that you need to still call nsIFrame::SchedulePaint in the cases
> where you have removed the invalidation. I don't think anything else on the
> AttributeChanged codepath will be doing this for you.

Okay, thanks. I thought the layers code would detect the transform change, and then do the right thing.

> Note that most of the 'x' and 'y' attribute changes shouldn't need to
> invalidate either, the display item will move in those cases.

Yeah, I'm trying to split this work up into smaller logical parts though in order to aid regression hunting if necessary.
Created attachment 712377 [details] [diff] [review]
patch

Adding SchedulePaint calls did the trick, thanks Matt.
Attachment #712245 - Attachment is obsolete: true
Attachment #712377 - Flags: review?(matt.woodrow)
(In reply to Jonathan Watt [:jwatt] from comment #8)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> > I suspect that you need to still call nsIFrame::SchedulePaint in the cases
> > where you have removed the invalidation. I don't think anything else on the
> > AttributeChanged codepath will be doing this for you.
> 
> Okay, thanks. I thought the layers code would detect the transform change,
> and then do the right thing.

It does, but layer building doesn't happen until the next paint :)
Comment on attachment 712377 [details] [diff] [review]
patch

Review of attachment 712377 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGInnerSVGFrame.cpp
@@ +205,5 @@
> +        nsSVGUtils::InvalidateBounds(this, false);
> +        nsSVGUtils::ScheduleReflowSVG(this);
> +      } else if (aAttribute == nsGkAtoms::transform) {
> +        // Don't invalidate or update bounds (the layers code does that).
> +        SchedulePaint(PAINT_COMPOSITE_ONLY);

This shouldn't be passing PAINT_COMPOSITE_ONLY (none of them should). That flag is only for when you've updated the layer tree manually (for example, you've modified the contents of a CanvasLayer by drawing into it), and a layer tree composition is all that's required to show the updates on screen.

In this case the transform has only changed on the frame, but the layer tree is unmodified. We need to run a full paint cycle to build new nsDisplayTransform items (with the updated transform set), DLBI to detect that these are different to the previous items, and layer building to set the new transform on the existing layer.

@@ +216,5 @@
> +        while (kid) {
> +          if (kid->GetStateBits() & NS_FRAME_SVG_LAYOUT &&
> +              !kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD) {
> +            // Don't invalidate or update bounds (the layers code does that).
> +            kid->SchedulePaint(PAINT_COMPOSITE_ONLY);

We only need to call SchedulePaint once for the window, it's a global state flag.
Created attachment 712645 [details] [diff] [review]
patch

Ah, okay. That's a bit different to how I understood things work, but it makes sense. Thanks.
Attachment #712377 - Attachment is obsolete: true
Attachment #712377 - Flags: review?(matt.woodrow)
Attachment #712645 - Flags: review?(matt.woodrow)
Attachment #712645 - Flags: review?(matt.woodrow) → review+
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/69277e48526c
Backed out due to reftest orange:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd883f16f17f

This appears to have broken animateMotion-mpath-pathLength-1.svg on most (all?) platforms. (w/ 20000 differing pixels, so it's not a fuzziness issue)

Sample failure logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19648585&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19649181&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19649501&tree=Mozilla-Inbound
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 840469
I couldn't reproduce locally, so I triggered some retests on TBPL:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=69277e48526c

Seems like the orange is intermittent. On digging into that I found that the initial state of animateMotion-mpath-pathLength-1.svg is also its pass state. Changing that to not be the case (bug 840469) allows me to reproduce locally (and I'd guess turns the test perma-orange on TBPL).

I'll look into this.
Created attachment 712867 [details]
testcase showing that elements that start offscreen never animate

It seems that with this patch, elements that start offscreen (elements that won't initially create a display list item) never animate onto the screen.
Created attachment 715582 [details] [diff] [review]
patch with fix for failing test

Daniel, can you review the nsSVGElement.cpp change?
Attachment #715582 - Flags: review?(dholbert)
Comment on attachment 715582 [details] [diff] [review]
patch with fix for failing test

as discussed in IRC, I think the comment was wrong (it means to say "animateTransform"), and the hint might not be quite right.

Can we just call GetAttributeChangeHint() to figure out the hint, instead of hardcoding the hint?

r=me on the nsSVGElement bits with that (assuming it works & makes sense to you too)
Attachment #715582 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #18)
> the hint might not be quite right.

Yes, it's not right. Specifically it should include nsChangeHint_UpdateTransformLayer too.

> Can we just call GetAttributeChangeHint() to figure out the hint, instead of
> hardcoding the hint?

Sure.

I've pushed to Try, where you can see the updated comment:

https://tbpl.mozilla.org/?tree=Try&rev=9f357ee68a45

Comment 20

5 years ago
You're passing transform into the GetAttributeChange hint rather than the actual transfrom attribute which would be the result of GetTransformListAttrName() 

Since GetTransformListAttrName() is virtual you should just call it the once in this function.
Well spotted. Updated.
(In reply to Jonathan Watt [:jwatt] from comment #19)
> Yes, it's not right. Specifically it should include
> nsChangeHint_UpdateTransformLayer too.

*sigh* Except that adding nsChangeHint_UpdateTransformLayer causes the rendering to end up being a little fuzzy in some reftests. The Try push shows those failures which can be viewed using the "open reftest analyzer" link. In the case of skew-1.svg the number of differing pixels is 13239 with a max difference of 124.

I'd be a bit sad to mark tests as fuzzy with such extreme fuzziness, but I'm not sure if there's anything that can be done from the layers side to avoid that.

Comment 23

5 years ago
It looks like antialiasing differences so try adding shape-rendering="crispEdges" to the ref and the test.
I'd rather avoid that if we can.
Created attachment 716117 [details]
reduced testcase demonstrating subpixel issue

Matt, roc, can you shed any light on what's going on here, and if there's something we can do?

In this testcase both rects should be blurry on their left and right edges due to the half pixel offset. However, with the patch applied, when you load the testcase you'll see that for about 0.5 seconds the top rect (the one having its transform animated) has sharp edges. It's only after the 0.5s expires (and the layer becomes non-active, presumably) that it renders correctly.

Is the compositing of layers being snapped or something? If so, could we change this to get the compositing to happen at fractional pixel offsets (at least for SVG)? Or is the best course of action to remove sub-pixel positioning from animated reftests (or add shape-rendering="crispEdges")?
Yep, we attempt to snap layer transforms to whole pixel offsets via Layer::SnapTransform.

I guess you could add a flag to the layer saying that we should skip that, and set it for the SVG created nsDisplayTransform layers.
Hardcoding things so that Layer::SnapTransformTranslation and Layer::SnapTransform cannot take their |if (mManager->IsSnappingEffectiveTransforms())| paths doesn't help. :(
Although it does make the rect appear to snap to the left, rather than right.

Comment 29

5 years ago
Jonathan,

Is your work on this bug likely to speed up 791699? If so does your patch apply cleanly to the tip of the master branch? Is there a build with the patch I could test to see if the performance issues is getting better?

If that's hard could you try running this simulation on your patched version to see if it continually slows down?

http://lab.concord.org/examples/interactives/embeddable.html#interactives/benchmarks/2-oil-and-water-shake.json

Since FFv18 our molecular dynamics modeling simulation is unusable in FF, however it works fine in FF v16.

A bit more precisely:

  Last good nightly: 2012-09-28
  First bad nightly: 2012-09-29

For more info see Boris Zbarsky's comment: https://bugzilla.mozilla.org/show_bug.cgi?id=791699#c28
Stephen, I'll create some builds for you to try. Which OS are you using?

Comment 31

5 years ago
Thanks, main development systems Mac OS X 10.6.8 and 10.8.2.
Try builds for Mac, Windows and Linux can be found here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwatt@jwatt.org-1ac14cb1c118/

Comment 33

5 years ago
Hi Jonathan,

I just tried the try-build on Mac OS X 10.6.8 and the performance problem described here is gone:

My specific test is to open the Oil and Water benchmark Interactive:

http://lab.dev.concord.org/examples/interactives/interactives.html#interactives/benchmarks/2-oil-and-water-shake.json

At the bottom of the window open the "Benchmarks" section and click "Run Benchmarks" six times.

The specific data I am looking at is in the fps column on the right.

Using FirefoxNightly-22.0a1-1ac14cb1c118 (the try build) I get fps values of: 35.5, 41.5, 40.0, 43.0, 41.0, 46.0

This is great!

The same test run on a regular 22.0a1  from 2013-02-21 generates these fps data: 22.5, 20.5, 20.0, 17.5, 18.0, 14.5

Note how it starts slower than your try build ... but even worse the fps rate gets slower the more times the benchmark tests is run!

I see equivalent slowdowns on FF >= 18.

For me the results from your try-build are fantastic!

Of course my next question (which may not have an easy answer) is when might this land in Nightly ... and then when might this fix land in in beta and stable releases?

I 'mmaking a change in our project today to display a non-modal warning about performance if the Interactives are being run in FF v18 or newer. The Interactives are (and will be) deployed in edX courses with large enrollments (last fall 23,000).

It would be great to be able to provide more specific details for FF users of the Molecular Dynamics Interactives that in the future they will be able to once again be able to use FF.
Hi Stephen. Great to hear. I'm somewhat surprised, since when I looked at your benchmark it seemed like most of the dynamic changes it makes are to x/y etc. coordinates rather than transforms. The only transform changes seemed to be on empty "label" elements as far as I could see. I guess we're currently doing a lot of work for transform changes on those empty elements that this patch eliminates. The good news is that when the (future) patch to kill of nsSVGUtils::InvalidateBounds calls for x/y etc. changes lands, you can expect your benchmark to speed up even more.

As to when the patch in this bug might land, I don't know. The snapping issue (see comment 22 - comment 28) needs to be debugged and fixed, and I don't anticipate having time for that in the next few weeks. If someone else takes that on then this could land soon (and early enough to put on branches), otherwise, I don't know.

Comment 35

5 years ago
More good results.

Additional tests on Mac OS X 10.8.2 and Windows 7 show your try-build also fixes the performance problems there too.

FirefoxNightly-22.0a1-1ac14cb1c118
http://lab.dev.concord.org/examples/interactives/interactives.html#interactives/benchmarks/2-oil-and-water-shake.json

FPS benchmark results (6 sequential samples):

2012 MacBook Pro (Retina) 10.8.2: 42.5, 60.0, 59.5, 59.5, 59.5, 59.0

2010 MacBook Pro running Windows 7: 44.0, 58.0, 53.0, 59.0, 59.0, 60.0

Comment 36

5 years ago
It seems like your current patch does more than just making InvalidateBounds for SVG transform changes faster ... the data I collect from an un-patched Nightly showing performance starting slower but then continually slowing seems to also indicate you're also fixing some egregious memory leak ... ???

In the Oil and Water" benchmark with just 44 atoms the slowdown over six samples in unpatched Nightly is about 35%.

However testing the "Plastic Forces" benchmark Interactive with over 250 atoms and bonds the slowdown in fps over six benchmark samples in an unpatched Nightly is over 80%:

http://lab.dev.concord.org/examples/interactives/interactives.html#interactives/benchmarks/7-plasticforces.json

FirefoxNightly-22.0a1-1ac14cb1c118: 14.0, 15.0, 15.0, 13.5, 15.0, 13.0

FirefoxNightly-22.0a1 (2013-02-21): 7.5, 4.5, 3.5, 3.0, 2.5, 2.5

Comment 37

5 years ago

Another possible clue is that the performance slowdown is almost entirely in the browser repaint and *not* in the JavaScript code.

The benchmark column "model+graphics" measures the model ticks/second for running the computational model *and* the rendering calls to manipulate the dom for 100 model steps *without* pausing to let the browser repaint.

The results from a patched and unpatched Nightly are almost identical and *no* slowdown is evident:

FirefoxNightly-22.0a1-1ac14cb1c118: 19.7, 20.7, 19.9, 20.3, 19.8, 20.0

FirefoxNightly-22.0a1 (2013-02-21): 19.6, 19.5, 19.1, 19.3, 18.6, 18.7
(Assignee)

Updated

4 years ago
Depends on: 861805

Comment 38

4 years ago
Any updates on when these patches might land?

This is a serious enough performance regression that our molecular dynamics simulations are all but unusable in Firefox: http://mw.concord.org/nextgen/interactives/
I hope to get some time on this later this week or early next.
I've landed this with the tests marked fuzzy for now. Hopefully that will make it easier to get some traction on figuring out the active layers bugs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6bd7ea66f6
https://hg.mozilla.org/mozilla-central/rev/dc6bd7ea66f6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 42

4 years ago
Is this in nightly yet ... I am not seeing any improvements yet in running the benchmarks four times for this molecular dynamics simulation:

http://lab.dev.concord.org/examples/interactives/interactives.html#interactives/benchmarks/2-oil-and-water-shake.json

Note both the slow speed in FF (much slower than v16 of FF) AND the continual slowdown in measurements of frames/s in subsequent tests:

Firefox 23.0a1 (2013-04-29)
model+graphics
steps/s           frames/s
----------------------------
48.9                24.0
48.6                20.5
45.0                18.0
46.8                16.5
45.8                15.0

Compare results to Firefox v16:

Firefox 16.0.2
model+graphics
steps/s           frames/s
----------------------------
102.1               29.5
100.0               29.0
104.5               29.5
99.0                30.5
104.8               29.0


Or Chrome v25:

Chrome	25.0.1364.99
model+graphics
steps/s           frames/s
----------------------------
223.7               60.5
209.2               60.5
221.7               60.5
222.7               61.0
217.9               61.0
It should be in the next nightly.

Comment 44

4 years ago
I no longer see the continuous performance decrease running in Nightly!

Computational performance in the model and rendering the view is still slower than FF v16.02 but the most serious performance regression appears to be fixed.


Firefox 23.0a1 (2013-04-30)
model+graphics
steps/s           frames/s
----------------------------
46.0                25.5
48.6                28.0
47.7                29.5
50.7                33.5


Firefox 16.0.2
model+graphics
steps/s           frames/s
----------------------------
104.2               32.0
110.5               34.5
98.4                33.0
100.1               34.0
(Assignee)

Updated

4 years ago
Depends on: 867599
(Assignee)

Updated

4 years ago
Blocks: 835041
(Assignee)

Updated

4 years ago
Blocks: 732108
(In reply to Jonathan Watt [:jwatt] from comment #40)
> I've landed this with the tests marked fuzzy for now. Hopefully that will
> make it easier to get some traction on figuring out the active layers bugs.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6bd7ea66f6

Was there ever a bug filed on investigating the fuzziness there?

These fuzzy tests are currently all marked like e.g.
> fuzzy(110,1056) == additive-1.svg additive-1-ref.svg     # bug 839865
right now, which implies that bug 839865 is _tracking_ the fuzziness -- but in fact bug 839865 is this bug here, the bug that _introduced_ the fuzziness.
Yes, bug 867599. I'll fix the reftest.list bug number comments in a bit.
(Assignee)

Updated

4 years ago
Blocks: 722090

Updated

4 years ago
Depends on: 882050
This made a huge difference to bug 884787.
Blocks: 884787
Depends on: 942451

Updated

4 years ago
Depends on: 975757
Depends on: 1042865
Depends on: 1067375
You need to log in before you can comment on or make changes to this bug.