1456281 - Consider not calling AllocateGeometry and ComputeInvalidationRegion for items <svg> elements

Status: NEW

(Core :: Graphics: WebRender, defect, P2)

Description

[Markus Stange [:mstange]]

SVGs commonly have a large number of display items. Any work that we do per-item has a potentially big performance impact. Here's a profile of moving my mouse on https://www.article19.org/xpa-17-metric/ , focused on mozilla::layers::DIGroup::ComputeGeometryChange: https://perfht.ml/2vEfoLF

As far as I know, SVG elements do not make use of DLBI. Anything that affects the appearance of something inside <svg> invalidates the frame. The only exceptions I can think of are:
 - async image decoding invalidation for <svg:image> (?)
 - <foreignObject>

It would be nice if we could avoid allocating geometries for DLBI for SVG items.
foreignObject should be easy to exclude from that optimization.

Matt, can you think of other reasons why we might need to call AllocateGeometry and ComputeInvalidationRegion?
Here's nsDisplaySVGGeometry::ComputeInvalidationRegion, it doesn't do anything interesting other than calling ShouldInvalidateToSyncDecodeImages:
https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/layout/svg/SVGGeometryFrame.cpp#137-152

Comment 1

[Matt Woodrow (:mattwoodrow)]

Looks like nsDisplaySVGGeometry is using nsDisplayItemGenericImageGeometry, which also checks for border rect changes.

If we're confident that SVG will invalidate the frame for border rect changes (which makes sense since we don't really reflow SVG), then we should be able to conditionally allocate the geometry if we actually have an image to paint.

Can you think of anything else we need here Jonathan?

Comment 2

[Jonathan Watt [:jwatt]]

(In reply to Markus Stange [:mstange] from comment #0)
> As far as I know, SVG elements do not make use of DLBI.

It's been five years and my memories are hazy, but unless something's changed or we have a different understanding of what precisely "DLBI" means, I thought SVG has been relying on DLBI since bug 827915.

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Looks like nsDisplaySVGGeometry is using nsDisplayItemGenericImageGeometry,
> which also checks for border rect changes.
> 
> If we're confident that SVG will invalidate the frame for border rect
> changes (which makes sense since we don't really reflow SVG),

We don't reflow in the sense of box model reflow, but we have a reflow-lite that has to set mRect (see SVGGeometryFrame::ReflowSVG) and we depend on mRect having useful values for DLBI to work.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1456281. Ensure that we don't need to call ComputeInvalidationRegion.

SVG reflow should make any changed items as invalid so this path
of the code should never be hit.

Review commit: https://reviewboard.mozilla.org/r/241042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241042/

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8972441 [details]
Bug 1456281. Ensure that we don't need to call ComputeInvalidationRegion.

https://reviewboard.mozilla.org/r/241042/#review246826

What about async image decoding?

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8972441 [details]
Bug 1456281. Ensure that we don't need to call ComputeInvalidationRegion.

https://reviewboard.mozilla.org/r/241042/#review247740

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Matt, Markus, I'm guessing we don't need to fix this for 61?

Comment 7

[Markus Stange [:mstange]]

This optimization idea is mostly targeted at WebRender, so there's no rush to fix this.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Moving to p2 because no activity for at least 24 weeks.
See How Do You Triage for more information

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:gw, since the bug has high priority, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Markus Stange [:mstange]]

This was the patch: https://hg.mozilla.org/mozreview/gecko/rev/17cb6969152eecb590d8746a0156dceed571a3a9

I had completely forgotten about this idea. It seems worthwhile.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0437f57074b07a1affd4870465beff988fe402d9

Comment 12

[Jeff Muizelaar [:jrmuizel]]

This fails layout/reftests/bugs/220165-1.svg

Comment 13

[Markus Stange [:mstange]]

Ah, that file tests foreignObject, which we would need to special case (see comment 0). Too bad the patch didn't include the special case, now we actually have to do some thinking ourselves.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

layout/reftests/svg/baseline-middle-01.svg also fills

Comment 15

[Jeff Muizelaar [:jrmuizel]]

After disabling the broken sync image decoding invalidation the assertion still fails on tests like layout/reftests/svg/text/mask-applied.svg because SVG reflow doesn't actually mark the frame as invalid because as of bug 802628 it just relies on DLBI

Comment 16

[Jeff Muizelaar [:jrmuizel]]

After making SVG reflow mark frames as invalid:

diff --git a/layout/svg/SVGContainerFrame.cpp b/layout/svg/SVGContainerFrame.cpp
index 38f4bf7a79470..ae1741d92a9ba 100644
--- a/layout/svg/SVGContainerFrame.cpp
+++ b/layout/svg/SVGContainerFrame.cpp
@@ -359,20 +359,21 @@ void SVGDisplayContainerFrame::ReflowSVG() {
     // nsChangeHint_UpdateEffects):
     SVGObserverUtils::UpdateEffects(this);
   }

   FinishAndStoreOverflow(overflowRects, mRect.Size());

   // Remove state bits after FinishAndStoreOverflow so that it doesn't
   // invalidate on first reflow:
   RemoveStateBits(NS_FRAME_FIRST_REFLOW | NS_FRAME_IS_DIRTY |
                   NS_FRAME_HAS_DIRTY_CHILDREN);
+  InvalidateFrame();
 }

 void SVGDisplayContainerFrame::NotifySVGChanged(uint32_t aFlags) {
   MOZ_ASSERT(aFlags & (TRANSFORM_CHANGED | COORD_CONTEXT_CHANGED),
              "Invalidation logic may need adjusting");

   if (aFlags & TRANSFORM_CHANGED) {
     // make sure our cached transform matrix gets (lazily) updated
     mCanvasTM = nullptr;
   }

We run into invalidations caused by transform changes with nsDisplayTransformGeometry in layout/reftests/svg/dynamic-text-06.svg. We could solve these by having a frame flag for container properties (transform, opacity, blendmode) changes.