854765 - Get rid of the DidSetStyleContext overrides in SVG frame code to avoid some major perf hits

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Example condition where SVG reflow is unnecessary.

As discussed in bug 845205, the nsSVGPathGeometryFrame and nsSVGGlyphFrame currently perform a reflow unconditionally in DidSetStyleContext().  This can cause unnecessary repainting when no-op style events occur.

The attached file provides an example test case, although that particular no-op may not trigger the condition once bug 845205 is resolved.

Note, dbaron writes in bug 845205 comment 21:

"And to be clear:  the SVG code is wacky because nearly all users of the DidSetStyleContext hook are wacky; our primary style change handling codepath is the change hint processing loop in nsCSSFrameConstructor::ProcessRestyledFrames."

Comment 1

[Jet Villegas (inactive)]

+cc: jwatt. Should we have expected DLBI to catch that the displaylist is unchanged from the last paint event?

Comment 2

[Jonathan Watt [:jwatt]]

We invalidate the frame explicitly in ReflowSVG implementations, which tells DLBI that the frame needs to be repainted. So DLBI itself is doing the right thing.

The main issue here is indeed that DidSetStyleContext blindly reflows SVG. We've known this to be an issue for a long time (see bug 270264 comment 1, for example), but other things like the various overhauls of SVG reflow and invalidation have been higher priority, and it's not been clear how we can fit this nicely into nsCSSFrameConstructor::ProcessRestyledFrames. I'd be happy to spend some time working on this now that those other things have been done. It's likely now one of the higher perf priorities.

Comment 3

[Jonathan Watt [:jwatt]]

Removing these calls is turning out to be non-trivial.

So let's say I have:

<svg>
  <defs>
    <rect ...>
  </defs>
  <path referencing the rect/>
</svg>

If the 'fill' property of the rect changes, we need to invalidate the path. I'd expect that this would happen via a restyle event being posted, then on processing the event we would end up in nsStyleSVG::CalcDifference and return the nsChangeHint_RepaintFrame hint:

https://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?mark=917-919&rev=cc139752bed4#917

On processing that hint in DoApplyRenderingChangeToTree we'd call InvalidateFrameSubtree: 

https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?mark=7734&rev=cc139752bed4#7726

Unfortunately this does not happen.

In nsStyleContext::CalcStyleDifference we do not call nsStyleSVG::CalcDifference because |(maxDifference & aParentHintsNotHandledForDescendants)| evaluates to zero, so we don't get the nsChangeHint_RepaintFrame hint. aParentHintsNotHandledForDescendants is "wrong" because in nsFrameManager::ReResolveStyleContext |(providerFrame != aFrame->GetParent())| evaluates to true, so we do NOT set aParentFrameHintsNotHandledForDescendants to nsChangeHint_Hints_NotHandledForDescendants:

https://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.cpp?mark=1125-1130&rev=cc139752bed4#1125

The reason that providerFrame is the rect's immediate parent's frame (the nsSVGContainer frame for the <defs>) is because in nsFrame::CorrectStyleParentFrame under:

  nsFrame::CorrectStyleParentFrame
  GetCorrectedParent
  nsFrame::DoGetParentStyleContextFrame
  nsFrame::GetParentStyleContextFrame
  nsFrameManager::ReResolveStyleContext

|parent->StyleContext()->GetPseudo()| returns nullptr for the nsSVGContainerFrame frame for the <defs>:

https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?mark=7200-7208&rev=cc139752bed4#7200

so we return that nsSVGContainerFrame.

Having tracked down _what_ is going wrong, it's not clear to me how things should be different since I don't know enough about the particular details of the style system at hand. bz, can you chip in?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> In nsStyleContext::CalcStyleDifference we do not call nsStyleSVG::CalcDifference because
> |(maxDifference & aParentHintsNotHandledForDescendants)| evaluates to zero

If the style of the rect itself changed, shouldn't "compare" be true in nsStyleContext::CalcStyleDifference?  I mean, we have a changed specified style, which means a changed set of nsIStyleRules, which means a changed nsRuleNode...  Which part of that is not happening?

> |(providerFrame != aFrame->GetParent())| evaluates to true

You mean false, right, if the if body is not entered?

> The reason that providerFrame is the rect's immediate parent's frame 

Is because that's the normal setup that usually happens.  The case when it's _not_ is the weird case: only happens when anonymous boxes or tables are involved.

In any case, the question is why "compare" is not true in nsStyleContext::CalcStyleDifference.  What does the testcase you're using look like?

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky (:bz) from comment #4)
> If the style of the rect itself changed, shouldn't "compare" be true in
> nsStyleContext::CalcStyleDifference?

It is.

> You mean false, right, if the if body is not entered?

Right, yes.

> In any case, the question is why "compare" is not true in
> nsStyleContext::CalcStyleDifference.

So actually it is - lldb wasn't doing a great job of letting me step through the macro code so I was having to extrapolate on the code paths that we were taking from the values of variables. I saw aParentHintsNotHandledForDescendants being set in my "working" case and not in my "broken" case and then misread the |if (compare...)| block.

So the real issue is that PeakStyleSVG() is returning nullptr in nsStyleContext::CalcStyleDifference for my broken case. Which brings me to this:

> What does the testcase you're using look like?

The broken testcase consists of:

<svg xmlns="http://www.w3.org/2000/svg"
     xmlns:xlink="http://www.w3.org/1999/xlink"
     onclick="document.getElementById('e').setAttribute('fill','lime')">
  <defs>
    <rect id="e" fill="red" width="100%" height="100%"/>
    <pattern id="pat" width="20" height="20">
      <use xlink:href="#e"/>
    </pattern>
  </defs>
  <rect fill="url(#pat)" width="100%" height="100%"/>
</svg>

Note that this bug is specific to <use> being used.

So I believe what is happening here is that "e" is cloned into the <use>, and when we paint it is the style data on that clone that is looked at. We never actually look at the style data on "e", ever, which is why GetStyle*() on it will always return nullptr. As a result CalcStyleDifference() will never call nsStyleSVG::CalcDifference(), and we'll never return the change hints we need.

Fun, fun.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: <use> testcase

Comment 7

[Jonathan Watt [:jwatt]]

s/which is why GetStyle*()/which is why PeekStyle*()/, obviously.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  So in today's world, how does a style change on "e" get propagated to the clone inside the <use>?

Comment 9

[Jonathan Watt [:jwatt]]

SVGUseElement is a mutation observer, SVGUseElement::AttributeChanged being called when the "fill" attribute is set on "e". SVGUseElement::AttributeChanged calls SVGUseElement::TriggerReclone, which calls presShell->PostRecreateFramesFor(this), which posts a nsChangeHint_ReconstructFrame restyle event. Under nsCSSFrameConstructor::ProcessRestyledFrames we then end up in nsSVGUseFrame::CreateAnonymousContent and recreate the cloned content for the <use>.

If the <use> was not inside a pattern then DLBI would pick up on the change to the frames for the anonymous content and invalidate as appropriate, but since the <use> is in a <pattern> it doesn't get any display lists.

(In reply to Boris Zbarsky (:bz) from comment #8)
> Hmm.  So in today's world, how does a style change on "e" get propagated to
> the clone inside the <use>?

Things proceed as above, and when the frames are created for the new cloned content we end up here:

  nsSVGEffects::InvalidateRenderingObservers
  nsSVGPathGeometryFrame::DidSetStyleContext
  nsFrame::Init
  nsSVGGeometryFrame::Init
  nsCSSFrameConstructor::InitAndRestoreFrame

So it's the DidSetStyleContext call on the _cloned_ content rather than on the referenced rect that is triggering the InvalidateRenderingObservers that we need to invalidate the <use>'s observer, the <pattern> (which will then in turn invalidate its observer, the displayed <rect>).

Without the InvalidateRenderingObservers call in nsSVGPathGeometryFrame::DidSetStyleContext, we really need the <use> to notify rendering observers when its anonymous content is recreated. Probably nsSVGUseFrame::CreateAnonymousContent should make an InvalidateRenderingObservers() call (or perhaps SVGUseElement::TriggerReclone).

Comment 10

[Jonathan Watt [:jwatt]]

I span the <use> issue (comment 3 - comment 9) out into bug 874487.

Comment 11

[Jonathan Watt [:jwatt]]

First, note that the change hint nsChangeHint_RepaintFrame causes DoApplyRenderingChangeToTree to call InvalidateFrameSubtree which calls InvalidateFrameSubtree which calls InvalidateFrame which calls InvalidateFrameInternal which calls nsSVGEffects::InvalidateDirectRenderingObservers up the parent chain. So when a style change results in DoApplyRenderingChangeToTree processing nsChangeHint_RepaintFrame for the frame, then that style change will result in SVG rendering observers being invalidated.

Now on to specific DidSetStyleContext calls...

filter primitive frames:

The only CSS properties to affect filter primitives are color and color-interpolation-filters (all), flood-color and flood-opacity (feFlood only), and lighting-color (feDiffuseLighting and feSpecularLighting only). Changes to these properties all make the relevant CalcDifference implementation (for nsStyleColor, nsStyleSVG and nsStyleSVGReset) return nsChangeHint_RepaintFrame. Since the only thing that the DidSetStyleContext overrides for SVGFEContainerFrame, SVGFEImageFrame and SVGFELeafFrame do is call nsSVGEffects::InvalidateRenderingObservers, these overrides are redundant and can be removed.

gradients and stops:

The only CSS properties to affect gradient stops are color, stop-color and stop-opacity. These are handled in nsStyleColor/nsStyleSVGReset::CalcDifference which return nsChangeHint_RepaintFrame, making the gradient and stop frames' DidSetStyleContext overrides redundant since they only invalidate rendering observers.

pattern:

The only CSS properties to affect patterns are clip and overflow, although we don't support either of those on pattern right now. That said, even if we did, nsStyleDisplay::CalcDifference will return nsChangeHint_ReconstructFrame for overflow changes and nsChangeHint_RepaintFrame for clip changes. Since nsSVGPatternFrame::DidSetStyleContext only invalidates rendering observers, it is redundant.

mask:

The only CSS properties to affect mask are clip, overflow, mask-type, color-interpolation and color-rendering. The same logic for pattern also applies to clip and overflow for mask. color-rendering is not implemented, but if it was then we would handle it in nsStyleSVG::CalcDifference. mask-type and color-interpolation both result in nsChangeHint_RepaintFrame being returned from nsStyleSVGReset::CalcDifference and nsStyleSVG::CalcDifference, respectively. Since nsSVGMaskFrame::DidSetStyleContext only invalidates direct rendering observes, it is therefore redundant.

foreignObject:

As far as I can tell the only properties to affect foreignObject are clip, overflow, opacity, visibility and some font properties such as font-size (by changing what em units resolve to in "x", etc.). Again I've checked that these result in nsChangeHint_RepaintFrame being returned from the relevant CalcDifference method. nsSVGForeignObjectFrame::DidSetStyleContext invalidates rendering observers, which is redundant, but it also schedules a reflow. I believe this is only necessary when font properties that can change what em resolves to change, and in those cases nsStyleFort::CalcDifference should return the required reflow hint. It may be that SVG 2 will add stroking and markers to foreignObject, but even if it does nsDisplaySVG::CalcDifference will make us do the right thing.

shapes and image (nsSVGPathGeometryFrame):

There are a large number of properties that can affect these elements, but this is exactly what nsStyleSVG/nsStyleSVGReset/etc.::CalcDifference should be handling. These are actually quite broken, so I've spun off bug 875037 to fix them. That done, nsSVGPathGeometryFrame::DidSetStyleContext is redundant.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 13

[Jonathan Watt [:jwatt]]

roc: you only need comment 11 for the commentary on this patch.

Comment 14

[Boris Zbarsky [:bzbarsky]]

What about nsSVGGlyphFrame?

Comment 15

[Boris Zbarsky [:bzbarsky]]

And by the way, thank you for sorting through all that!

Comment 16

[Jonathan Watt [:jwatt]]

Oh, right. I meant to say that I didn't bother spending time on nsSVGGlyphFrame since we're killing that code once svg.text.css-frames.enabled is flipped (bug 839955), which is very close.

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> And by the way, thank you for sorting through all that!

Sure thing. We'll all be glad to get rid of these crazy DidSetStyleContext overrides. :)

Comment 18

[Jonathan Watt [:jwatt]]

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

Comment 19

[(no longer active)]

Backed out because of mochitest-1 assertions: https://hg.mozilla.org/integration/mozilla-inbound/rev/c04054ed1bd9

Comment 20

[Jonathan Watt [:jwatt]]

The failure was caused by bug 875037.

Comment 21

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/297eccee61b5

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/297eccee61b5

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Fx24 in bug 907503.