Closed Bug 1159053 Opened 9 years ago Closed 9 years ago

Cache svg bbox calculations for better performance

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch bbox (obsolete) — Splinter Review
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.)
Attachment #8598321 - Flags: review?(cam)
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?
Have we tests for this i.e get a bbox change an attribute getbbox again?
(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.
Attached patch patch (obsolete) — Splinter Review
Do you want to review this Robert?
Attachment #8598321 - Attachment is obsolete: true
Attachment #8598321 - Flags: review?(cam)
Attachment #8598632 - Flags: review?(longsonr)
Keywords: perf
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?
Attachment #8598632 - Flags: review?(longsonr) → review-
(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.
Attached patch patchSplinter Review
Attachment #8598632 - Attachment is obsolete: true
Attachment #8599001 - Flags: review?(cam)
Attachment #8599001 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/605d1ea1ff14
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1179608
You need to log in before you can comment on or make changes to this bug.