Bug 67394 - Use fastGetAttribute() and fastHasAttribute() where appropriate.
Summary: Use fastGetAttribute() and fastHasAttribute() where appropriate.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 67496
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-01 06:21 PDT by Andreas Kling
Modified: 2013-09-08 19:01 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (102.05 KB, patch)
2011-09-01 06:32 PDT, Andreas Kling
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (101.18 KB, patch)
2011-09-01 08:56 PDT, Andreas Kling
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-09-01 06:21:24 PDT
We should use fastGetAttribute() and fastHasAttribute() wherever the queried attribute is neither "style" or an SVG animatable attribute.
Comment 1 Andreas Kling 2011-09-01 06:32:40 PDT
Created attachment 105951 [details]
Proposed patch
Comment 2 WebKit Review Bot 2011-09-01 07:33:43 PDT
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 3 Andreas Kling 2011-09-01 07:34:39 PDT
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.
Comment 4 Andreas Kling 2011-09-01 08:23:29 PDT
Today I learned that xlink:href is an SVG animatable attribute. :)
Comment 5 Andreas Kling 2011-09-01 08:56:04 PDT
Created attachment 105974 [details]
Proposed patch

Let's try that again without messing with xlink:href.
Comment 6 Antonio Gomes 2011-09-01 09:01:27 PDT
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 7 WebKit Review Bot 2011-09-01 09:20:50 PDT
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 8 Darin Adler 2011-09-01 10:08:55 PDT
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.
Comment 9 Antonio Gomes 2011-09-01 10:12:18 PDT
(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);
}
Comment 10 Andreas Kling 2011-09-01 10:18:13 PDT
(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.
Comment 11 Antonio Gomes 2011-09-01 10:29:20 PDT
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 :)
Comment 12 Andreas Kling 2011-09-01 10:51:45 PDT
(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.
Comment 13 Darin Adler 2011-09-01 15:07:54 PDT
(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.
Comment 14 Andreas Kling 2011-09-02 09:12:19 PDT
Committed r94421: <http://trac.webkit.org/changeset/94421>
Comment 15 Darin Adler 2011-09-02 12:29:24 PDT
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.
Comment 16 Andreas Kling 2011-09-02 19:53:39 PDT
(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.
Comment 17 Andreas Kling 2011-09-02 19:54:26 PDT
(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 :/
Comment 18 Andreas Kling 2011-09-05 08:22:04 PDT
Quick update: After closer inspection, this is a can of worms. I'm working on a proper solution (and blog post..)
Comment 19 Andreas Kling 2013-09-06 18:43:16 PDT
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.
Comment 20 Darin Adler 2013-09-08 19:01:49 PDT
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.