[webkit-reviews] review denied: [Bug 22699] Enable NodeList caching for getElementsByTagName : [Attachment 26239] Initial patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Smith <catfish.man at gmail.com>'s
request for review:
Bug 22699: Enable NodeList caching for getElementsByTagName
https://bugs.webkit.org/show_bug.cgi?id=22699

Attachment 26239: Initial patch
https://bugs.webkit.org/attachment.cgi?id=26239&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   data->setNodeLists(std::auto_ptr<NodeListsNodeData>(new
NodeListsNodeData));

In .cpp files we normally don't use the "std::" prefix. Instead we do "using
namespace std" at the top of the file. In this file I think that's already done
so you can just omit the "std::" here.

> +    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom :
AtomicString(namespaceURI), name, result.first->second);

It would be good to change the namespaceURI argument to an AtomicString rather
than converting it here. Unrelated to your patch though.

> -struct QNameHash {
> +struct QNameImplHash {

I don't think this name change is a good one. A single hash struct/class can
support hashing multiple different types as long as there's no ambiguity, so
this could hash both Impl and non-Impl if we later implemented hashing for
non-Impl. I think adding the Impl to the name is not an improvement. I think
that instead we should combine both the new QNameHash and the old QNameImplHash
and put both in the header.

> +    // Golden ratio - arbitrary start value to avoid mapping all 0's to all
0's
> +    static const unsigned PHI = 0x9e3779b9U;

This should be a single constant shared by the various hash functions. I don't
think it's good to have it defined inside the QualifiedName class. A step in
the wrong direction. Lets find a place to put it so we can share it.

> +static inline unsigned hashComponents(const QualifiedNameComponents& buf)

This is now in a header, so it should not be marked "static", since that gives
it internal linkage. Things in headers should almost never have internal
linkage.

> +    typedef HashMap<QualifiedName::QualifiedName, DynamicNodeList::Caches*,
QNameHash, HashTraits<WebCore::QualifiedName> > TagCacheMap;

How does this compile? Does QualifiedName::QualifiedName actually work?

No need for the WebCore:: in WebCore::QualifiedName.

Instead of explicitly specifying HashTraits<QualifiedName>, you should make
that be the default. The way to do that is to specialize DefaultHash for the
QualifiedName type. For examples, look at KURL.h, AtomicString.h,
PlatformString.h, and StringImpl.h. Don't emulate IntSizeHash.h which puts the
default in a separate header. It's important that the default be specialized in
the same header that defines the type, even if the actual hash structure is
defined in another header, because otherwise you could end up getting the wrong
default depending on what headers you include.

There's enough that could be improved that I'm going to say review-, but this
looks like great work that's on the right track.

I worry a little that this uses more memory to trade off better speed. Are
there some speed tests that demonstrate the importance of this optimization on
the real web?