We should use fastGetAttribute() and fastHasAttribute() wherever the queried attribute is neither "style" or an SVG animatable attribute.
Created attachment 105951 [details] Proposed patch
Comment on attachment 105951 [details] Proposed patch Attachment 105951 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9579333 New failing tests: svg/dom/SVGScriptElement/script-set-href.svg svg/custom/js-update-image-and-display3.svg svg/dom/SVGScriptElement/script-reexecution.svg svg/dom/SVGScriptElement/script-change-externalResourcesRequired-while-loading.svg svg/dom/SVGScriptElement/script-onerror-bubbling.svg svg/dom/SVGScriptElement/script-load-and-error-events.svg svg/custom/createImageElement.svg
Comment on attachment 105951 [details] Proposed patch (In reply to comment #2) > (From update of attachment 105951 [details]) > Attachment 105951 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/9579333 > > New failing tests: > svg/dom/SVGScriptElement/script-set-href.svg > svg/custom/js-update-image-and-display3.svg > svg/dom/SVGScriptElement/script-reexecution.svg > svg/dom/SVGScriptElement/script-change-externalResourcesRequired-while-loading.svg > svg/dom/SVGScriptElement/script-onerror-bubbling.svg > svg/dom/SVGScriptElement/script-load-and-error-events.svg > svg/custom/createImageElement.svg Whoops! Investigating.
Today I learned that xlink:href is an SVG animatable attribute. :)
Created attachment 105974 [details] Proposed patch Let's try that again without messing with xlink:href.
Comment on attachment 105974 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=105974&action=review > Source/WebCore/dom/Document.cpp:632 > - const AtomicString& accessKey = element->getAttribute(accesskeyAttr); > + const AtomicString& accessKey = element->fastGetAttribute(accesskeyAttr); would not this be simpler if the check was done internally to ::getAttribute() instead of changing all call sites?
Comment on attachment 105974 [details] Proposed patch Attachment 105974 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9583242 New failing tests: svg/custom/createImageElement.svg svg/custom/js-update-image-and-display3.svg
Comment on attachment 105974 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=105974&action=review > Source/WebCore/ChangeLog:10 > + Change call sites that don't check the "style" or SVG animatable > + attributes to use fastGetAttribute()/fastHasAttribute() instead > + of getAttribute()/hasAttribute(). How is that you know none of these are SVG animatable attributes? That’s one thing that held me back in the past. I wasn’t sure which attributes could be animated by SVG. >> Source/WebCore/dom/Document.cpp:632 >> + const AtomicString& accessKey = element->fastGetAttribute(accesskeyAttr); > > would not this be simpler if the check was done internally to ::getAttribute() instead of changing all call sites? As Andreas implied in the change log, the getAttribute function is visible to the public DOM and has to handle the style attribute properly and animated SVG attributes properly. We can use fastGetAttribute at call sites where we know the attribute is not styleAttr and where we know animated SVG attributes can’t possibly be involved. That’s why the call sites need to change. > Source/WebCore/dom/Element.cpp:2006 > + const AtomicString& value = fastGetAttribute(HTMLNames::spellcheckAttr); Not sure why this has the HTMLNames prefix; other uses in this file don’t have it.
(In reply to comment #8) > (From update of attachment 105974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105974&action=review > > > Source/WebCore/ChangeLog:10 > > + Change call sites that don't check the "style" or SVG animatable > > + attributes to use fastGetAttribute()/fastHasAttribute() instead > > + of getAttribute()/hasAttribute(). > > How is that you know none of these are SVG animatable attributes? That’s one thing that held me back in the past. I wasn’t sure which attributes could be animated by SVG. > > >> Source/WebCore/dom/Document.cpp:632 > >> + const AtomicString& accessKey = element->fastGetAttribute(accesskeyAttr); > > > > would not this be simpler if the check was done internally to ::getAttribute() instead of changing all call sites? > > As Andreas implied in the change log, the getAttribute function is visible to the public DOM and has to handle the style attribute properly and animated SVG attributes properly. That I understood. I meant Element::getAttritibut(Attr attr) { if (attr != svn_bleg && attr != style) return fastGetAttribute(attr); return slowGetAttribute(attr); }
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 105974 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=105974&action=review > > > > > Source/WebCore/ChangeLog:10 > > > + Change call sites that don't check the "style" or SVG animatable > > > + attributes to use fastGetAttribute()/fastHasAttribute() instead > > > + of getAttribute()/hasAttribute(). > > > > How is that you know none of these are SVG animatable attributes? That’s one thing that held me back in the past. I wasn’t sure which attributes could be animated by SVG. > > > > >> Source/WebCore/dom/Document.cpp:632 > > >> + const AtomicString& accessKey = element->fastGetAttribute(accesskeyAttr); > > > > > > would not this be simpler if the check was done internally to ::getAttribute() instead of changing all call sites? > > > > As Andreas implied in the change log, the getAttribute function is visible to the public DOM and has to handle the style attribute properly and animated SVG attributes properly. > > That I understood. I meant > > Element::getAttritibut(Attr attr) > { > if (attr != svn_bleg && attr != style) > return fastGetAttribute(attr); > > return slowGetAttribute(attr); > } If you look at Element::getAttribute(), it already does something similar to what you're suggesting, as well as calling fastGetAttribute() in the end. It should also be noted that Element::fast*Attribute() are inline functions. Given how much they're used, we don't want to add unnecessary bloat, and changing call sites to out-of-line function calls will likely affect PLT performance.
stood. I meant > > > > Element::getAttritibut(Attr attr) > > { > > if (attr != svn_bleg && attr != style) > > return fastGetAttribute(attr); > > > > return slowGetAttribute(attr); > > } > > If you look at Element::getAttribute(), it already does something similar to what you're suggesting, as well as calling fastGetAttribute() in the end. > > It should also be noted that Element::fast*Attribute() are inline functions. Given how much they're used, we don't want to add unnecessary bloat, and changing call sites to out-of-line function calls will likely affect PLT performance. Fair enough :)
(In reply to comment #8) > (From update of attachment 105974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105974&action=review > > > Source/WebCore/ChangeLog:10 > > + Change call sites that don't check the "style" or SVG animatable > > + attributes to use fastGetAttribute()/fastHasAttribute() instead > > + of getAttribute()/hasAttribute(). > > How is that you know none of these are SVG animatable attributes? That’s one thing that held me back in the past. I wasn’t sure which attributes could be animated by SVG. Attributes animatable by SVG are documented as such in the specification. (Each property has a line saying "Animatable: yes/no.") In WebKit they can be found with "grep -r DEFINE_ANIMATED Source/WebCore/svg". I did just find that HTMLNames::classAttr is animatable on SVGStyledElement though, so this patch is unfortunately flawed. Adding Dirk and Nikolas in hopes that they may have some input on this.
(In reply to comment #12) > In WebKit they can be found with "grep -r DEFINE_ANIMATED Source/WebCore/svg". > > I did just find that HTMLNames::classAttr is animatable on SVGStyledElement though, so this patch is unfortunately flawed. Adding Dirk and Nikolas in hopes that they may have some input on this. If classAttr is animatable, then lets just be sure not to use the fast functions on it. Maybe we can make fastGetAttribute and fastHasAttribute check that the passed-in attribute is not one of the animatable ones when used in debug builds.
Committed r94421: <http://trac.webkit.org/changeset/94421>
I know this was rolled out, but I don’t know how to find out why. Would love to hear what kinds of failures this caused.
(In reply to comment #15) > I know this was rolled out, but I don’t know how to find out why. Would love to hear what kinds of failures this caused. It caused failures on Chromium. I believe it's due to their usage of Element::imageSourceAttributeName() which may return XLinkNames::hrefAttr. I will make a new patch tomorrow that doesn't do fastFooAttribute(element->imageSourceAttributeName()) anywhere and double-check with EWS.
(In reply to comment #16) > (In reply to comment #15) > > I know this was rolled out, but I don’t know how to find out why. Would love to hear what kinds of failures this caused. > > It caused failures on Chromium. I believe it's due to their usage of Element::imageSourceAttributeName() which may return XLinkNames::hrefAttr. I will make a new patch tomorrow that doesn't do fastFooAttribute(element->imageSourceAttributeName()) anywhere and double-check with EWS. To clarify, SVGImageElement returns XLinkNames::hrefAttr for the imageSourceAttributeName, which is indeed an SVG animatable attribute :/
Quick update: After closer inspection, this is a can of worms. I'm working on a proper solution (and blog post..)
Oh wow, is this patch 2 years old already? I was gonna do it "tomorrow" I don't agree with my past self that this is a good idea. All that inlining for unimportant attribute access has wonked space/time tradeoff.
The point of the fast versions is to skip unnecessary work; those extra two branches to implement the style attribute and animated SVG attributes are almost never needed when we are getting a specific attribute by name. The inlining issue is a separate one. There could be yet-another different version that is inlined.