Bug 61183 – SVG svgAttributeChanged/synchronizeProperty/parseMappedAttribute should be optimized

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 94203 [details]
Patch 

A first EWS run, still have to measure PLT perf diffs.

[Early Warning System Bot]

Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8722149

[WebKit Review Bot]

Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8722151

[Gyuyoung Kim]

Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8719364

[WebKit Review Bot]

Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8719371

[Nikolas Zimmermann]

Created attachment 94217 [details]
Patch v2

Fix non-allinone-builds.

[WebKit Review Bot]

Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8716621

[Early Warning System Bot]

Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8716627

[WebKit Review Bot]

Comment on attachment 94203 [details]
Patch 

Attachment 94203 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8721318

[Nikolas Zimmermann]

Created attachment 94220 [details]
Patch v3

[Early Warning System Bot]

Comment on attachment 94220 [details]
Patch v3

Attachment 94220 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8723168

[WebKit Review Bot]

Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8718487

[Nikolas Zimmermann]

Created attachment 94225 [details]
Patch v4

More build fixes..

[WebKit Review Bot]

Comment on attachment 94217 [details]
Patch v2

Attachment 94217 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8721334

[Nikolas Zimmermann]

Created attachment 94227 [details]
Patch v5

Fix merge problem.

[Early Warning System Bot]

Comment on attachment 94225 [details]
Patch v4

Attachment 94225 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8722192

[WebKit Review Bot]

Comment on attachment 94220 [details]
Patch v3

Attachment 94220 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8719398

[Rob Buis]

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?

[Nikolas Zimmermann]

(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.

[Rob Buis]

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.

[Rob Buis]

Comment on attachment 94227 [details]
Patch v5

r=me

[Nikolas Zimmermann]

Landed in r87010.

[Dirk Schulze]

This patch caused a regression if the web author uses another prefix for Xlink namespace than 'xlink'. See bug 68679.

[Dirk Schulze]

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?

[Nikolas Zimmermann]

Comment on attachment 94227 [details]
Patch v5

Dirk, can we close this bug now?

[Eric Seidel (no email)]

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.

[Dirk Schulze]

(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.