[webkit-reviews] review denied: [Bug 97298] [Refactoring] Introduce a traversal strategy in SelectorChecker : [Attachment 165065] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 97298: [Refactoring] Introduce a traversal strategy in SelectorChecker
https://bugs.webkit.org/show_bug.cgi?id=97298

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165065&action=review


I am still happy with the general approach, just need to keep improving.

> Source/WebCore/ChangeLog:14
> +	   Since this code path is very hot, we were very careful not to
regress performance.
> +	   We used template and inline method, also we did not add a new
argument to SelectorChecker::checkOneSelector,
> +	   but replaced the SelectorCheckingContext to Strategy. When we added
a new argument to checkOneSelector,
> +	   we detected 3% ~ 5% performance regression.

This seems like black magic. Why would the performance regress like this?

Also, I am fine with Context containing Strategy, but not the other way around.
Strategy is an abstract notion and shouldn't really have state. Context
contains specifics of selector-checking. Strategy taking Context as argument
also seems fine.

> Source/WebCore/ChangeLog:42
> +	   (WebCore::SelectorChecker::DOMTraversalStrategy::isFirstChild): This
kind of methods takes arguments
> +	   instead of having a member. When we use member, we have detected 10%
performance regression.

I don't understand this comment. Maybe remove it? :)

> Source/WebCore/css/SelectorChecker.h:268
> +inline bool SelectorChecker::DOMTraversalStrategy::isFirstOfType(Element*
element, const QualifiedName& type) const

Do all these methods have to live in the header? That seems sad.

> PerformanceTests/CSS/pseudoclass-selectors.html:6
> +    <span></span>

I wonder if you would make the tree a bit more complex, you'd see more
interesting data. I worry this is too small to give you any meaningful results.