| Summary: | SVG svgAttributeChanged/synchronizeProperty/parseMappedAttribute should be optimized | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Enhancement | CC: | dglazkov, krit, rwlbuis, thorton, webkit.review.bot, zimmermann | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 68679 | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 94203 [details]
Patch
A first EWS run, still have to measure PLT perf diffs.
Comment on attachment 94203 [details] Patch Attachment 94203 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8722149 Comment on attachment 94203 [details] Patch Attachment 94203 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8722151 Comment on attachment 94203 [details] Patch Attachment 94203 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8719364 Comment on attachment 94203 [details] Patch Attachment 94203 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8719371 Created attachment 94217 [details]
Patch v2
Fix non-allinone-builds.
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 on attachment 94217 [details] Patch v2 Attachment 94217 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8716627 Comment on attachment 94203 [details] Patch Attachment 94203 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8721318 Created attachment 94220 [details]
Patch v3
Comment on attachment 94220 [details] Patch v3 Attachment 94220 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8723168 Comment on attachment 94217 [details] Patch v2 Attachment 94217 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8718487 Created attachment 94225 [details]
Patch v4
More build fixes..
Comment on attachment 94217 [details] Patch v2 Attachment 94217 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8721334 Created attachment 94227 [details]
Patch v5
Fix merge problem.
Comment on attachment 94225 [details] Patch v4 Attachment 94225 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8722192 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 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? (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. 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 on attachment 94227 [details]
Patch v5
r=me
Landed in r87010. This patch caused a regression if the web author uses another prefix for Xlink namespace than 'xlink'. See bug 68679. 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 on attachment 94227 [details]
Patch v5
Dirk, can we close this bug now?
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. (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. |
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.