[webkit-reviews] review denied: [Bug 73853] Inserting nodes is slow due to Node::notifyNodeListsAttributeChanged (20%+) : [Attachment 149165] Removed more code

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied 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 149165: Removed more code
https://bugs.webkit.org/attachment.cgi?id=149165&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149165&action=review


Overall, this looks like a big improvement!

This patch also uses an extra pointer per DynamicNodeList now because it adds
more of them to m_listsInvalidatedAtDocument, right? I think that's fine, but
the ChangeLog should state it. I don't have a good intuition for how many
concurrent DynamicNodeLists a give page has. Have you instrumented gmail or
facebook to get a sense of what the memory impact of this change would be?

> Source/WebCore/dom/Document.cpp:3879
> +	   if (!attrName || (*it)->shouldInvalidateOnAttributeChange())

As the code is, it's weird for this to take a QualifiedName instead of an enum
or something (e.g. ShouldClearAttributeCaches). But, I think in the future, we
probably want to be even more fine-grained and only clear the attribute caches
that are affected by this attribute name. Mind putting a FIXME to that effect?
With the FIXME there it makes more sense to pass in a QualifiedName.

> Source/WebCore/dom/Node.cpp:967
> +    document()->clearNodeListCaches(&attrName);

The comment on lines 950-955 probably needs to be updated.