[webkit-reviews] review granted: [Bug 73853] Inserting nodes is slow due to Node::notifyNodeListsAttributeChanged (20%+) : [Attachment 120057] 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 73853: Inserting nodes is slow due to Node::notifyNodeListsAttributeChanged
(20%+)
https://bugs.webkit.org/show_bug.cgi?id=73853

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

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


Looks OK. I wasn’t able to think through everything about the data structure in
detail. I would have liked to see a more thorough test, though. Given how much
different code there is in here I’d like to see something that covers more of
the code for different types, for example.

> Source/WebCore/dom/DynamicNodeList.cpp:37
> +    if (type != LabelNodeListType)
> +	   rootNode()->registerDynamicSubtreeNodeList(type, this);

This mysterious if condition needs a comment.

> Source/WebCore/dom/DynamicNodeList.cpp:49
> +    if (!hasValidCache())
> +	   node()->treeScope()->nodeListCachedValue(m_type);

It’s a little strange to call a function saying “I cached a value” before
caching the value.

> Source/WebCore/dom/Node.cpp:487
> +    for (NodeListsNodeData::ClassNodeListCache::const_iterator it =
nodeLists->m_classNodeListCache.begin();
> +	    it != nodeLists->m_classNodeListCache.end(); ++it) {
> +	   if (currentTreeScope)
> +	       currentTreeScope->removeNodeListCache(ClassNodeListType,
it->second);
> +	   newTreeScope->addNodeListCache(ClassNodeListType, it->second);
> +    }

So ugly. I can’t wait until we support only compilers with good C++11 support.
If we had one of those then we could write:

    for (const auto& value: nodeLists->m_classNodeListCache.values()) {
	if (currentTreeScope)
	    currentTreeScope->removeNodeListCache(ClassNodeListType, value);
	newTreeScope->addNodeListCache(ClassNodeListType, value);
    }

I wonder if we can achieve something similar with a macro.

> Source/WebCore/dom/Node.cpp:543
> +	   // FIXME: assert that nodes don't have node lists

Sentence style please.

Not sure that an idea for an assertion really qualifies as FIXME. I might just
make a comment about the possible improvement without using FIXME. Recently I
have been thinking we should strive to consistently use FIXME only when we
think something is wrong, not when there is simply room for improvement.

> Source/WebCore/dom/Node.cpp:1064
> +static DynamicSubtreeNodeListType attrToNodeListType(const QualifiedName&
attrName)

I think a better name is:

    nodeListTypeForAttribute

These noun-type names work better if the beginning of the name is the noun.

> Source/WebCore/dom/Node.cpp:1080
> +    return InvalidNodeListType;
> +    // LabelsNodeList is invalidated via notifyLocalNodeListsLabelChanged in
parseMappedAttribute

Missing period in this sentence-style comment. Comment would be better before
the return statement.

> Source/WebCore/dom/Node.cpp:1102
> +    // FIXME: We don't have to invalidate caches if the inserted or removed
node
> +    // didn't have an attribute we care about.

I think that for clarity we should say “any of the attributes” rather than “an
attribute”. Also would be clearer if this stated that this is a performance
idea.

> Source/WebCore/dom/Node.cpp:3025
> -void NodeRareData::createNodeLists(Node* node)
> +void NodeRareData::createNodeLists(Node*)

Perhaps we should remove the unused argument entirely.

> Source/WebCore/dom/Node.h:114
> +enum DynamicSubtreeNodeListType {
> +    ClassNodeListType,
> +    NameNodeListType,
> +    TagNodeListType,
> +    TagNodeListNSType,
> +#if ENABLE(MICRODATA)
> +    MicroDataItemListType,
> +#endif
> +    LabelNodeListType,
> +    InvalidNodeListType
> +};

I’d suggest:

1) Adding a brief comment about why Invalid must be the last value.
2) Sorting the other values alphabetically as we do headers, in a separate
paragraph.
3) Putting the #if value in its own separate paragraph instead of mixing it in
with the unconditional ones.

> Source/WebCore/dom/Node.h:115
> +const unsigned numDynamicSubtreeNodeListType = InvalidNodeListType;

Why make the constant unsigned instead of of the enum type? I believe you could
use the enum type consistently, even for the loop index, and avoid all the
static_cast calls in the code below.

The name should be plural with a trailing “s”: “number of types”, not “number
of type”.

> Source/WebCore/dom/TreeScope.h:63
> +    void nodeListCachedValue(DynamicSubtreeNodeListType type)

This sounds like a getter that returns a “cached value” for a “node list”.

This kind of situation is what leads to names like nodeListDidCacheValue or
nodeListWillCacheValue, either of which will be clearer than this.

> Source/WebCore/dom/TreeScope.h:69
> +    void nodeListInvalidatedCache(DynamicSubtreeNodeListType type)

I think that nodeListDidInvalidateCache is a slightly better name.

> Source/WebCore/html/LabelsNodeList.cpp:35
> +LabelsNodeList::LabelsNodeList(Node* forNode)

The old name here, “for node”, is not good naming. We want to use a noun
phrase. There might even be a good idea for the name in the HTML specification
itself.

But you need not change that name in this patch.