Bug 99694 - Refactor SVGAnimationElement's calcMode() and animationMode() for performance
Summary: Refactor SVGAnimationElement's calcMode() and animationMode() for performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-18 00:45 PDT by Philip Rogers
Modified: 2012-10-31 13:54 PDT (History)
11 users (show)

See Also:


Attachments
First pass (44.43 KB, patch)
2012-10-20 15:06 PDT, Philip Rogers
eric: review-
Details | Formatted Diff | Diff
Patch 1 - Cache calcMode() (6.18 KB, patch)
2012-10-23 21:08 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff
Patch 2 - Let SVGElements have pending resources. (14.44 KB, patch)
2012-10-23 23:39 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff
Patch 3 - Cache animationMode() (26.31 KB, patch)
2012-10-29 21:56 PDT, Philip Rogers
eric: review+
Details | Formatted Diff | Diff
Patch 3 - Cache animationMode() (27.63 KB, patch)
2012-10-31 10:16 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-10-18 00:45:59 PDT
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.
Comment 1 Philip Rogers 2012-10-20 15:06:37 PDT
Created attachment 169775 [details]
First pass
Comment 2 Philip Rogers 2012-10-20 15:09:04 PDT
It may be best to split this patch into two but I wanted to let reviewers see the final goal before splitting it up.
Comment 3 Eric Seidel (no email) 2012-10-20 15:48:25 PDT
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.
Comment 4 Dirk Schulze 2012-10-22 16:02:38 PDT
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/
Comment 5 Eric Seidel (no email) 2012-10-23 16:36:57 PDT
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?
Comment 6 Philip Rogers 2012-10-23 21:08:01 PDT
Created attachment 170307 [details]
Patch 1 - Cache calcMode()

The original patch was way too big. This is the first self-contained piece: caching calcMode().
Comment 7 Philip Rogers 2012-10-23 23:39:19 PDT
Created attachment 170327 [details]
Patch 2 - Let SVGElements have pending resources.

This is the second self-contained piece: let SVGElements have pending resources.
Comment 8 Eric Seidel (no email) 2012-10-28 10:40:29 PDT
Comment on attachment 170307 [details]
Patch 1 - Cache calcMode()

OK.
Comment 9 Philip Rogers 2012-10-28 16:05:44 PDT
Comment on attachment 170307 [details]
Patch 1 - Cache calcMode()

Patch #1: off to the queue!
Comment 10 WebKit Review Bot 2012-10-28 16:32:42 PDT
Comment on attachment 170307 [details]
Patch 1 - Cache calcMode()

Clearing flags on attachment: 170307

Committed r132755: <http://trac.webkit.org/changeset/132755>
Comment 11 Eric Seidel (no email) 2012-10-29 13:53:27 PDT
Comment on attachment 170327 [details]
Patch 2 - Let SVGElements have pending resources.

OK.
Comment 12 WebKit Review Bot 2012-10-29 14:08:04 PDT
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>
Comment 13 WebKit Review Bot 2012-10-29 14:08:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Philip Rogers 2012-10-29 14:23:09 PDT
Reopening. I saved the best patch for last! Patch forthcoming
Comment 15 Philip Rogers 2012-10-29 21:56:49 PDT
Created attachment 171370 [details]
Patch 3 - Cache animationMode()

This is the final and best patch.
Comment 16 Philip Rogers 2012-10-29 21:57:26 PDT
Re-reopening for the final patch.
Comment 17 Eric Seidel (no email) 2012-10-30 11:33:59 PDT
Comment on attachment 171370 [details]
Patch 3 - Cache animationMode()

LGTM.
Comment 18 Philip Rogers 2012-10-31 10:16:11 PDT
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.
Comment 19 WebKit Review Bot 2012-10-31 13:54:48 PDT
Comment on attachment 171681 [details]
Patch 3 - Cache animationMode()

Clearing flags on attachment: 171681

Committed r133074: <http://trac.webkit.org/changeset/133074>
Comment 20 WebKit Review Bot 2012-10-31 13:54:54 PDT
All reviewed patches have been landed.  Closing bug.