Bug 61183 - SVG svgAttributeChanged/synchronizeProperty/parseMappedAttribute should be optimized
Summary: SVG svgAttributeChanged/synchronizeProperty/parseMappedAttribute should be op...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 68679
  Show dependency treegraph
 
Reported: 2011-05-20 04:55 PDT by Nikolas Zimmermann
Modified: 2014-05-14 23:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (309.92 KB, patch)
2011-05-20 06:00 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (310.39 KB, patch)
2011-05-20 07:24 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (310.69 KB, patch)
2011-05-20 08:11 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v4 (311.10 KB, patch)
2011-05-20 08:35 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v5 (311.34 KB, patch)
2011-05-20 08:46 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-05-20 04:55:52 PDT
SVG svgAttributeChanged/synchronizeProperty/parseMappedAttribute should be optimized.

Example: rect.x.baseVal.value = 100;
What happens: SVGRectElement::svgAttributeChanged(const QualifiedName& attrName) is invoked with "SVGNames::rectAttr" as parameter.

void SVGRectElement::svgAttributeChanged(const QualifiedName& attrName)
{
    SVGStyledTransformableElement::svgAttributeChanged(attrName);

    // Handle my own attribute changes...
}

We always traverse the base class hiearchy, when invoking svgAttributeChanged (this has a reason, explained below (*) ).
An efficient pattern would look like:

HashSet<QualifiedName> attributesSupportedByRect; (eg, containing SVGNames::xAttr/yAttr/widthAttr/.. and the SVGTests/SVGLangSpace/... properties)
void SVGRectElement::svgAttributeChanged(const QualifiedName& attrName)
{
    if (!attributesSupportedByRect.contains(attrName)) {
        SVGStyledTransformableElement::svgAttributeChanged(attrName);
        return;
    }

    // When we get here, we know for sure it's one of our attributes that has changed.
    // Note for eg. SVGNames::transformAttr, the call from SVGRectElement::svgAttributeChanged, would be immediately forwarded to sVGStyledTransformableElement which handles transformAttr changes)

    if (attrName == SVGNames::xAttr) { do_work(); return; }
    if (attrName == SVGNames::yAttr) { do_work(); return; }
...
    ASSERT_NOT_REACHED();
}

Exactly the same pattern can be applied to synchronizeProperty and parseMappedAttribute. I have not yet measured the performance benefits, but just imagine, how much fewer string comparisions are now done when parsing attributes / synchronizing svg<->xml dom and when changing attributes via scripts either through DOM or SVG DOM.

Note (*):
Every svgAttributeChanged call from a class like SVGRectElement has to reach the base class SVGStyledElement::svgAttributeChanged, as it handles invalidation of the instances of an element.
Say that a <rect> is referenced by a <use> and we change the 'x' attribute of the <rect>, then SVGStyledElement::svgAttributeChanged, calls SVGElementInstance::invalidateAllInstancesOfElement(this), so that the <use> can rebuild its shadow tree... That's the only reason we do the base-class dance, and can easily be avoided.
Comment 1 Nikolas Zimmermann 2011-05-20 06:00:53 PDT
Created attachment 94203 [details]
Patch 

A first EWS run, still have to measure PLT perf diffs.
Comment 2 Early Warning System Bot 2011-05-20 06:12:19 PDT
Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8722149
Comment 3 WebKit Review Bot 2011-05-20 06:48:45 PDT
Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8722151
Comment 4 Gyuyoung Kim 2011-05-20 07:03:15 PDT
Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8719364
Comment 5 WebKit Review Bot 2011-05-20 07:22:26 PDT
Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8719371
Comment 6 Nikolas Zimmermann 2011-05-20 07:24:59 PDT
Created attachment 94217 [details]
Patch v2

Fix non-allinone-builds.
Comment 7 WebKit Review Bot 2011-05-20 07:36:26 PDT
Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8716621
Comment 8 Early Warning System Bot 2011-05-20 07:39:59 PDT
Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8716627
Comment 9 WebKit Review Bot 2011-05-20 07:54:01 PDT
Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8721318
Comment 10 Nikolas Zimmermann 2011-05-20 08:11:37 PDT
Created attachment 94220 [details]
Patch v3
Comment 11 Early Warning System Bot 2011-05-20 08:22:01 PDT
Comment on attachment 94220 [details]
Patch v3

Attachment 94220 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8723168
Comment 12 WebKit Review Bot 2011-05-20 08:31:37 PDT
Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8718487
Comment 13 Nikolas Zimmermann 2011-05-20 08:35:17 PDT
Created attachment 94225 [details]
Patch v4

More build fixes..
Comment 14 WebKit Review Bot 2011-05-20 08:39:20 PDT
Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8721334
Comment 15 Nikolas Zimmermann 2011-05-20 08:46:15 PDT
Created attachment 94227 [details]
Patch v5

Fix merge problem.
Comment 16 Early Warning System Bot 2011-05-20 08:53:59 PDT
Comment on attachment 94225 [details]
Patch v4

Attachment 94225 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8722192
Comment 17 WebKit Review Bot 2011-05-20 09:09:18 PDT
Comment on attachment 94220 [details]
Patch v3

Attachment 94220 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8719398
Comment 18 Rob Buis 2011-05-20 10:09:28 PDT
Comment on attachment 94227 [details]
Patch v5

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

I like the code changes. Once some of my questions and ChangeLog suggestions are cleared up this can be r+ed quickly I think, until then r-.

> Source/WebCore/ChangeLog:18
> +        Switch to a more efficient pattern, by providing a "bool isSupportedAttribute(const QualifiedName&);" function for all SVG*Elements.

Typo: hierarchy

> Source/WebCore/ChangeLog:31
> +            // Note for eg. SVGNames::transformAttr, the call from SVGRectElement::svgAttributeChanged, would be immediately forwarded to the base class, which handles transformAttr changes)

You mean by the if section above? If so the Note can be moved above it to indicate what the if section does.

> Source/WebCore/ChangeLog:47
> +

dance sounds a bit frivolous :) Better reword

> Source/WebCore/ChangeLog:48
> +        Add "SVGElementInstance::InvalidationGuard guard(this)" statments in all svgAttributeChanged implementations, that calls invalidateAllInstancesOfElement(this)

Typo: statements

> Source/WebCore/ChangeLog:53
> +

how? Better, worse, the same?
Comment 19 Nikolas Zimmermann 2011-05-20 11:14:24 PDT
(In reply to comment #18)
> (From update of attachment 94227 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94227&action=review
> 
> I like the code changes. Once some of my questions and ChangeLog suggestions are cleared up this can be r+ed quickly I think, until then r-.
> 
> > Source/WebCore/ChangeLog:18
> > +        Switch to a more efficient pattern, by providing a "bool isSupportedAttribute(const QualifiedName&);" function for all SVG*Elements.
> 
> Typo: hierarchy
Fixed.

> 
> > Source/WebCore/ChangeLog:31
> > +            // Note for eg. SVGNames::transformAttr, the call from SVGRectElement::svgAttributeChanged, would be immediately forwarded to the base class, which handles transformAttr changes)
> 
> You mean by the if section above? If so the Note can be moved above it to indicate what the if section does.
Fixed.

> 
> > Source/WebCore/ChangeLog:47
> > +
> 
> dance sounds a bit frivolous :) Better reword
Fixed.

> 
> > Source/WebCore/ChangeLog:48
> > +        Add "SVGElementInstance::InvalidationGuard guard(this)" statments in all svgAttributeChanged implementations, that calls invalidateAllInstancesOfElement(this)
> 
> Typo: statements
Fixed.

> 
> > Source/WebCore/ChangeLog:53
> > +
> 
> how? Better, worse, the same?
Reworded: it's a slight overall performance progression (around 2-3% in DRT runs), hard to observe, other than using micro benchmarks  (eg. setting transformAttr ten thousand times, etc.).

Do you want me to upload a new version? We usually give r+, if only comments have to change, and ask the patch creator to fix before landing.
Comment 20 Rob Buis 2011-05-20 12:10:16 PDT
Hi Niko,

(In reply to comment #19)

[...]

> Do you want me to upload a new version? We usually give r+, if only comments have to change, and ask the patch creator to fix before landing.

No, that is fine, I'll change the state, so r=me.
Cheers,

Rob.
Comment 21 Rob Buis 2011-05-20 12:12:22 PDT
Comment on attachment 94227 [details]
Patch v5

r=me
Comment 22 Nikolas Zimmermann 2011-05-20 23:18:23 PDT
Landed in r87010.
Comment 23 Dirk Schulze 2011-09-23 10:00:56 PDT
This patch caused a regression if the web author uses another prefix for Xlink namespace than 'xlink'. See bug 68679.
Comment 24 Dirk Schulze 2011-09-24 01:11:57 PDT
Comment on attachment 94227 [details]
Patch v5

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

> Source/WebCore/svg/SVGURIReference.cpp:69
> +void SVGURIReference::addSupportedAttributes(HashSet<QualifiedName>& supportedAttributes)
> +{
> +    DEFINE_STATIC_LOCAL(AtomicString, xlinkPrefix, ("xlink"));
> +    QualifiedName hrefWithPrefix = XLinkNames::hrefAttr;
> +    hrefWithPrefix.setPrefix(xlinkPrefix);
> +    supportedAttributes.add(hrefWithPrefix);
> +    supportedAttributes.add(XLinkNames::hrefAttr);
> +}
> +

thorten already noticed it yesterday. This seems to be the problem: The assumption that the prefix of Xlink is always 'xlink'.

Just a general question. Can the prefix get changed dynamically? Or is it possible to assume that the prefix won't change once it is defined in the document?
Comment 25 Nikolas Zimmermann 2012-02-08 01:52:33 PST
Comment on attachment 94227 [details]
Patch v5

Dirk, can we close this bug now?
Comment 26 Eric Seidel (no email) 2012-02-10 10:20:35 PST
Comment on attachment 94227 [details]
Patch v5

Cleared Rob Buis's review+ from obsolete attachment 94227 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 27 Dirk Schulze 2014-05-14 23:13:25 PDT
(In reply to comment #25)
> (From update of attachment 94227 [details])
> Dirk, can we close this bug now?

I hope so. There are certain other optimizations that we can do. HTML did some of them recently. This should be done in more detailed bug reports though.