Bug 67394 – Use fastGetAttribute() and fastHasAttribute() where appropriate.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: DOM

[Andreas Kling]

We should use fastGetAttribute() and fastHasAttribute() wherever the queried attribute is neither "style" or an SVG animatable attribute.

[Andreas Kling]

Created attachment 105951 [details]
Proposed patch

[WebKit Review Bot]

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

[Andreas Kling]

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.

[Andreas Kling]

Today I learned that xlink:href is an SVG animatable attribute. :)

[Andreas Kling]

Created attachment 105974 [details]
Proposed patch

Let's try that again without messing with xlink:href.

[Antonio Gomes]

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?

[WebKit Review Bot]

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

[Darin Adler]

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.

[Antonio Gomes]

(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);
}

[Andreas Kling]

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

[Antonio Gomes]

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 :)

[Andreas Kling]

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

[Darin Adler]

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

[Andreas Kling]

Committed r94421: <http://trac.webkit.org/changeset/94421>

[Darin Adler]

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.

[Andreas Kling]

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

[Andreas Kling]

(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 :/

[Andreas Kling]

Quick update: After closer inspection, this is a can of worms. I'm working on a proper solution (and blog post..)

[Andreas Kling]

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.

[Darin Adler]

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.