Cache svg bbox calculations for better performance

RESOLVED FIXED in Firefox 40

Status

()

Core
SVG
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Depends on: 1 bug, {perf})

Trunk
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8598321 [details] [diff] [review]
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.)
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?

Comment 3

2 years ago
Have we tests for this i.e get a bbox change an attribute getbbox again?
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
Created attachment 8598632 [details] [diff] [review]
patch

Do you want to review this Robert?
Attachment #8598321 - Attachment is obsolete: true
Attachment #8598321 - Flags: review?(cam)
Attachment #8598632 - Flags: review?(longsonr)
(Assignee)

Updated

2 years ago
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-
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
Created attachment 8599001 [details] [diff] [review]
patch
Attachment #8598632 - Attachment is obsolete: true
Attachment #8599001 - Flags: review?(cam)
Attachment #8599001 - Flags: review?(cam) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/605d1ea1ff14
https://hg.mozilla.org/mozilla-central/rev/605d1ea1ff14
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

2 years ago
Depends on: 1179608
You need to log in before you can comment on or make changes to this bug.