1159053 - Cache svg bbox calculations for better performance

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should cache svg bbox calculations for better performance. Not only with that unblock bug 923193, but it should help us with real world SVGs where we've seen bbox calculation hurting us.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: bbox

I've not pushed this through Try just yet, but any r+ can be conditional on that working out. (I'll push it shortly with patches for bug 923193.)

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8598321 [details] [diff] [review]
bbox

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

We are unconditionally caching the bbox we calculate, but nsSVGUtils::GetBBox could be called with different aFlags values.  Shouldn't we account for that?

Comment 3

[Robert Longson [:longsonr]]

Have we tests for this i.e get a bbox change an attribute getbbox again?

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #3)
> Have we tests for this i.e get a bbox change an attribute getbbox again?

Some of the tests in dom/svg/test/test_switch.xhtml fail, but I'll add some more explicit ones.

(In reply to Cameron McCormack (:heycam) from comment #2)
> We are unconditionally caching the bbox we calculate, but
> nsSVGUtils::GetBBox could be called with different aFlags values.  Shouldn't
> we account for that?

Good point - I've added a test for that too.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Do you want to review this Robert?

Comment 6

[Cameron McCormack (:heycam)]

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

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

One small problem with the caching.

::: layout/svg/nsSVGUtils.cpp
@@ +907,5 @@
> +    FrameProperties props = aFrame->Properties();
> +    gfxRect* prop =
> +      static_cast<gfxRect*>(props.Get(ObjectBoundingBoxProperty()));
> +    if (prop) {
> +      return *prop;

I think you need to check for aFlags == eBBoxIncludeFillGeometry, since otherwise we could initially cache the fill-only bbox and then call getBBox({ fill: true, stroke: true }) and get the cached fill-only result back.  Can you add a sub-test to test_bbox-changes.xhtml that would catch that?  (Maybe a display:none element followed by explicit getBBox() calls will do it.)

::: layout/svg/nsSVGUtils.h
@@ +403,5 @@
>     * aFrame's userspace.
>     */
>    static gfxRect GetBBox(nsIFrame *aFrame,
> +                         // If the default arg changes, update the handling for
> +                         // ObjectBoundingBoxProperty() in the implementation.

I guess it's not what the default is that matters here, but what is the most common bbox requested?

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #6)
> One small problem with the caching.

Probably a bit bigger than small!

> I guess it's not what the default is that matters here, but what is the most
> common bbox requested?

Strictly speaking, yes. I'm kinda deliberately conflating the two in the comment to not make it too unwieldy.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/605d1ea1ff14

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/605d1ea1ff14