[webkit-reviews] review granted: [Bug 89783] REGRESSION(r120979): getElementsByTagName is 12% slower : [Attachment 149167] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 89783: REGRESSION(r120979): getElementsByTagName is 12% slower
https://bugs.webkit.org/show_bug.cgi?id=89783

Attachment 149167: Patch
https://bugs.webkit.org/attachment.cgi?id=149167&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=149167&action=review


A few things that still look like mistakes.

> Source/WebCore/dom/NodeRareData.h:62
> +    template <typename StringType>
> +    struct NodeListCacheMapEntryHash {
> +	   static unsigned hash(const std::pair<StringType, unsigned char>&
entry)
> +	   {
> +	       return WTF::DefaultHash<StringType>::Hash::hash(entry.first) +
entry.second;
> +	   }
> +
> +	   static bool equal(const std::pair<StringType, unsigned char>& a,
const std::pair<StringType, unsigned char>& b) { return a == b; }
> +
> +	   static const bool safeToCompareToEmptyOrDeleted =
WTF::DefaultHash<StringType>::Hash::safeToCompareToEmptyOrDeleted;
> +    };

We could make this smaller by deriving from WTF::PairHash and overriding only
the hash function. There is no need to write a custom equal function or to set
safeToCompareEmptyOrDeleted if we derive.

There is no need for the WTF:: prefix in WTF::DefaultHash; it can just be
DefaultHash.

> Source/WebCore/dom/NodeRareData.h:64
> +    typedef HashMap<std::pair<AtomicString, unsigned char>,
DynamicSubtreeNodeList*, NodeListCacheMapEntryHash<AtomicString> >
NodeListAtomicNameCacheMap;

Why did we reorder the pair to put the string first? Why did we change from
unsigned short to unsigned char. Maybe those are good changes, but nothing here
says they were intentional or helpful changes.

> Source/WebCore/dom/NodeRareData.h:169
> -    std::pair<unsigned short, AtomicString>
namedNodeListKey(DynamicNodeList::NodeListType listType, const AtomicString&
name)
> +    std::pair<AtomicString, unsigned short>
namedNodeListKey(DynamicNodeList::NodeListType listType, const AtomicString&
name)
>      {
> -	   return std::pair<unsigned short, AtomicString>(listType, name);
> +	   return std::pair<AtomicString, unsigned short>(name, listType);
>      }

We changed the maps to use unsigned char, but for some reason left it unsigned
short here. That will result in conversions. I suggest just leaving this all
alone, or making the types consistent if there is a good reason to change that.