Bug 99694 – Refactor SVGAnimationElement's calcMode() and animationMode() for performance

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

SVGAnimationElement::calcMode() and SVGAnimationElement::animationMode() are a large component in SVG animation profiles. These two functions are incredibly cacheable as they only return data based off of attributes (the only exception being AnimateMotion which depends on a path).

Patch forthcoming.

[Philip Rogers]

Created attachment 169775 [details]
First pass

[Philip Rogers]

It may be best to split this patch into two but I wanted to let reviewers see the final goal before splitting it up.

[Eric Seidel (no email)]

Comment on attachment 169775 [details]
First pass

There is one step further, which is to carry a dirty-bit and only recalc on demand.  Is it possible to do these two separate?  Not required.  Just curious how hard it would have been.

[Dirk Schulze]

Comment on attachment 169775 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=169775&action=review

It looks good for me. I can remember tat we had a reason not to put it on SVGElement but don't know why right now.

> Source/WebCore/ChangeLog:147
> +            SVGPathElemetn has been changed so that it notifies any <mpath>

s/SVGPathElemetn/SVGPathElement/

[Eric Seidel (no email)]

Comment on attachment 169775 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=169775&action=review

pdr says he'll split this up. :)

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:49
> +    m_calcMode = CalcModePaced;

setCalcMode()?

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:115
>          m_path = Path();
>          buildPathFromString(attribute.value(), m_path);
> +        updateAnimationPath();

What about calling this from a setPath() (or setPathFromString?) method?

[Philip Rogers]

Created attachment 170307 [details]
Patch 1 - Cache calcMode()

The original patch was way too big. This is the first self-contained piece: caching calcMode().

[Philip Rogers]

Created attachment 170327 [details]
Patch 2 - Let SVGElements have pending resources.

This is the second self-contained piece: let SVGElements have pending resources.

[Eric Seidel (no email)]

Comment on attachment 170307 [details]
Patch 1 - Cache calcMode()

OK.

[Philip Rogers]

Comment on attachment 170307 [details]
Patch 1 - Cache calcMode()

Patch #1: off to the queue!

[WebKit Review Bot]

Comment on attachment 170307 [details]
Patch 1 - Cache calcMode()

Clearing flags on attachment: 170307

Committed r132755: <http://trac.webkit.org/changeset/132755>

[Eric Seidel (no email)]

Comment on attachment 170327 [details]
Patch 2 - Let SVGElements have pending resources.

OK.

[WebKit Review Bot]

Comment on attachment 170327 [details]
Patch 2 - Let SVGElements have pending resources.

Clearing flags on attachment: 170327

Committed r132847: <http://trac.webkit.org/changeset/132847>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

Reopening. I saved the best patch for last! Patch forthcoming

[Philip Rogers]

Created attachment 171370 [details]
Patch 3 - Cache animationMode()

This is the final and best patch.

[Philip Rogers]

Re-reopening for the final patch.

[Eric Seidel (no email)]

Comment on attachment 171370 [details]
Patch 3 - Cache animationMode()

LGTM.

[Philip Rogers]

Created attachment 171681 [details]
Patch 3 - Cache animationMode()

Updating the 3rd patch with expectations for EFL and QT so we don't break their tests.

[WebKit Review Bot]

Comment on attachment 171681 [details]
Patch 3 - Cache animationMode()

Clearing flags on attachment: 171681

Committed r133074: <http://trac.webkit.org/changeset/133074>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.