869611 - Stop reflowing in nsSVGPathGeometryFrame::NotifySVGChanged when transforms change

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Right now nsSVGPathGeometryFrame::NotifySVGChanged schedules a reflow of itself without checking aFlags. This means that when aFlags is just set to TRANSFORM_CHANGED (i.e. a transform changed on an ancestor of the frame) it is unnecessarily reflowing. This is preventing us from benefiting from layers when transforms are animating on ancestors, which is a huge waste.

The only reason for us to reflow for a change on an ancestor at this point in the code is if the element's geometry has a percentage value that is resolved against the nearest coordinate context. (Note that we don't need to update the frames overflow rects for TRANSFORM_CHANGED since its only if the frame's _own_ transform changes that we need to do that.) I'll add some logic to expose that information and knock up a patch to make use of it.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Further 5x speed-up on bug 852588 (Santa's Workshop).

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 746579 [details] [diff] [review]
patch

Generally looks good.

> bool
>+nsSVGPathGeometryElement::GeometryDependsOnCoordCtx() const
>+{
>+  // Check the nsSVGLength2 attribute

I think this wants to be "attributes" (plural)?

>+  LengthAttributesInfo info = const_cast<nsSVGPathGeometryElement*>(this)->GetLengthInfo();

The const / const_cast dance here is a bit hacky & seems unnecessary. There's only one caller, and it doesn't have a const-restricted pointer, so no need for this method to be const.

r=me with that

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> >+  LengthAttributesInfo info = const_cast<nsSVGPathGeometryElement*>(this)->GetLengthInfo();
> 
> The const / const_cast dance here is a bit hacky & seems unnecessary.
> There's only one caller, and it doesn't have a const-restricted pointer, so
> no need for this method to be const.

Alternately, I think GetLengthInfo() can probably be made to be a const method. I'll give that a shot right now over in bug 869679... If you'd like to keep GeometryDependsOnCoordCtx as const and layer this on top of that bug (without the need for the hacky const_cast anymore), that's fine too. (assuming bug 869679 works out)

Comment 5

[Daniel Holbert [:dholbert]]

(Actually, disregard comment 4 -- GetLengthInfo can't be converted to const, as I just noted in bug 869679 comment 1, though we could probably add an alternate const version if it's useful enough. I'm not going to bother with that myself, but you may want to if you'd really like GeometryDependsOnCoordCtx to be const. :))

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: breaktest

Here's a reduced testcase from the the "svg/as-image" reftests that currently fail, in builds with the patch applied.

This testcase is actually taken from the *reference* case (img-height-slice-1-ref.html) because that's what changes rendering when this patch is applied. (So that's for SVG in <embed>, not for SVG-as-an-image.)

Comment 7

[Jonathan Watt [:jwatt]]

Thanks, Daniel. This is actually fixed by my latest patch in bug 869781.

Comment 8

[Daniel Holbert [:dholbert]]

woot!

Comment 9

[Jonathan Watt [:jwatt]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Jonathan Watt [:jwatt]]

This more than doubled the performance of the Santa's Workshop demo (bug 852588) for me. Not too surprising really.

Comment 12

[Jonathan Watt [:jwatt]]

This sped up the IE Test Drive Helicopter demo quite substantially; for me it went from 15-20 "fps" to 30-35 "fps".