We can save one CPU word per instance (wohoo!) by moving HTMLImageElement::ismap to the end.
Created attachment 104864 [details] Proposed patch
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.
Rescoping since Darin has a pretty good point.
Created attachment 105360 [details] Proposed patch v2 Remove the class members altogether and look them up with fast{Has,Get}Attribute() when needed.
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 = ..."
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.
Created attachment 105617 [details] Patch for landing
Comment on attachment 105617 [details] Patch for landing Clearing flags on attachment: 105617 Committed r94074: <http://trac.webkit.org/changeset/94074>
All reviewed patches have been landed. Closing bug.