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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should stop calling nsSVGUtils::InvalidateBounds for SVG transform changes, since DLBI should be handling that for us.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Is this likely to fix bug 817993?

Comment 3

[Jonathan Watt [:jwatt]]

I'm not sure.

Comment 4

[Jonathan Watt [:jwatt]]

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.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: testcase

This simpler testcase shows the problem clearly when run with vs without the attached patch.

Comment 6

[Matt Woodrow (:mattwoodrow)]

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.

Comment 7

[Matt Woodrow (:mattwoodrow)]

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

Comment 8

[Jonathan Watt [:jwatt]]

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

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

Adding SchedulePaint calls did the trick, thanks Matt.

Comment 10

[Matt Woodrow (:mattwoodrow)]

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

[Matt Woodrow (:mattwoodrow)]

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.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

Ah, okay. That's a bit different to how I understood things work, but it makes sense. Thanks.

Comment 13

[Jonathan Watt [:jwatt]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/69277e48526c

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Jonathan Watt [:jwatt]]

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.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: 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.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: patch with fix for failing test

Daniel, can you review the nsSVGElement.cpp change?

Comment 18

[Daniel Holbert [: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)

Comment 19

[Jonathan Watt [:jwatt]]

(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

[Robert Longson [:longsonr]]

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.

Comment 21

[Jonathan Watt [:jwatt]]

Well spotted. Updated.

Comment 22

[Jonathan Watt [:jwatt]]

(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

[Robert Longson [:longsonr]]

It looks like antialiasing differences so try adding shape-rendering="crispEdges" to the ref and the test.

Comment 24

[Jonathan Watt [:jwatt]]

I'd rather avoid that if we can.

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: 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")?

Comment 26

[Matt Woodrow (:mattwoodrow)]

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.

Comment 27

[Jonathan Watt [:jwatt]]

Hardcoding things so that Layer::SnapTransformTranslation and Layer::SnapTransform cannot take their |if (mManager->IsSnappingEffectiveTransforms())| paths doesn't help. :(

Comment 28

[Jonathan Watt [:jwatt]]

Although it does make the rect appear to snap to the left, rather than right.

Comment 29

[Stephen Bannasch]

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

Comment 30

[Jonathan Watt [:jwatt]]

Stephen, I'll create some builds for you to try. Which OS are you using?

Comment 31

[Stephen Bannasch]

Thanks, main development systems Mac OS X 10.6.8 and 10.8.2.

Comment 32

[Jonathan Watt [:jwatt]]

Try builds for Mac, Windows and Linux can be found here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1ac14cb1c118/

Comment 33

[Stephen Bannasch]

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.

Comment 34

[Jonathan Watt [:jwatt]]

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

[Stephen Bannasch]

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

[Stephen Bannasch]

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

[Stephen Bannasch]

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

Comment 38

[Stephen Bannasch]

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/

Comment 39

[Jonathan Watt [:jwatt]]

I hope to get some time on this later this week or early next.

Comment 40

[Jonathan Watt [:jwatt]]

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

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc6bd7ea66f6

Comment 42

[Stephen Bannasch]

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

Comment 43

[Cameron McCormack (:heycam)]

It should be in the next nightly.

Comment 44

[Stephen Bannasch]

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

Comment 45

[Daniel Holbert [:dholbert]]

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

Comment 46

[Jonathan Watt [:jwatt]]

Yes, bug 867599. I'll fix the reftest.list bug number comments in a bit.

Comment 47

[Jonathan Watt [:jwatt]]

This made a huge difference to bug 884787.