Bug 66784 – HTMLImageElement: Don't cache "ismap" and "usemap" attributes.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

We can save one CPU word per instance (wohoo!) by moving HTMLImageElement::ismap to the end.

[Andreas Kling]

Created attachment 104864 [details]
Proposed patch

[Darin Adler]

Comment on attachment 104864 [details]
Proposed patch

This change is OK.

But really ismap is just the cached result of hasAttribute(ismapAttr). It would be better to get rid of the boolean entirely and just call hasAttribute(ismapAttr) instead.

We could do something similar for usemap too. Generally speaking there is little value to caching a copy of an attribute unless there’s something super-performance-critical. For m_name and m_id this may be justified since it’s critical to know what the old value of the attribute was when it changes, but even there I think we might be able to optimize the storage out.

[Andreas Kling]

Rescoping since Darin has a pretty good point.

[Andreas Kling]

Created attachment 105360 [details]
Proposed patch v2

Remove the class members altogether and look them up with fast{Has,Get}Attribute() when needed.

[Andreas Kling]

Comment on attachment 105360 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=105360&action=review

> Source/WebCore/html/HTMLImageElement.cpp:-137
> -    } else if (attrName == ismapAttr)
> -        ismap = true;

I'm unsure whether it's preferred to still catch ismapAttr here, but do nothing, rather than calling the base class.

> Source/WebCore/html/HTMLImageElement.cpp:401
> +    AtomicString usemap = fastGetAttribute(usemapAttr);

On second thought, this should probably be "const AtomicString& usemap = ..."

[Darin Adler]

Comment on attachment 105360 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=105360&action=review

>> Source/WebCore/html/HTMLImageElement.cpp:-137
>> -        ismap = true;
> 
> I'm unsure whether it's preferred to still catch ismapAttr here, but do nothing, rather than calling the base class.

I think it’s fine to leave out the code and thus call the base class. I don’t know of any concrete benefits of not calling the base class.

>> Source/WebCore/html/HTMLImageElement.cpp:401
>> +    AtomicString usemap = fastGetAttribute(usemapAttr);
> 
> On second thought, this should probably be "const AtomicString& usemap = ..."

Yes, that would be slightly better.

> Source/WebCore/html/HTMLImageElement.cpp:403
> +    if (usemap.string()[0] == '#')
> +        return false;

I think this could use a “why” comment.

> Source/WebCore/html/HTMLImageElement.cpp:405
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(usemap)).isEmpty();

I’m not sure we have to call completeURL just to check if it’s empty. I think that the completeURL implementation guarantees that passed-in empty strings result in an empty URL and passed-in non-empty strings result in a non-empty URL.

[Andreas Kling]

Created attachment 105617 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 105617 [details]
Patch for landing

Clearing flags on attachment: 105617

Committed r94074: <http://trac.webkit.org/changeset/94074>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.