[webkit-reviews] review denied: [Bug 87942] [Performance] Optimize querySelector() by caching SelectorQuery objects : [Attachment 145204] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 87942: [Performance] Optimize querySelector() by caching SelectorQuery
objects
https://bugs.webkit.org/show_bug.cgi?id=87942

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=145204&action=review


Looks better!

> Source/WebCore/dom/SelectorQuery.cpp:183
> +SelectorQueryCache::Entry::Entry(const AtomicString& selectors, Document*
document, ExceptionCode& ec)
> +{
> +    CSSParser parser(document);
> +    parser.parseSelector(selectors, m_querySelectorList);
> +
> +    if (!m_querySelectorList.first() ||
m_querySelectorList.hasUnknownPseudoElements()) {
> +	   ec = SYNTAX_ERR;
> +	   return;
> +    }
> +
> +    // throw a NAMESPACE_ERR if the selector includes any namespace
prefixes.
> +    if (m_querySelectorList.selectorsNeedNamespaceResolution()) {
> +	   ec = NAMESPACE_ERR;
> +	   return;
> +    }
> +    
> +    m_selectorQuery.initialize(m_querySelectorList);
> +}

The parsing should be done in the calling function not the Entry constructor
(as parsing can fail and we just constructed something we don't need). The
constructor can take CSSSelectorList as argument.

> Source/WebCore/dom/SelectorQuery.h:94
> +class Entry : public RefCounted<SelectorQueryCache::Entry> {
> +public:
> +    Entry(const AtomicString&, Document*, ExceptionCode&);
> +    SelectorQuery& selectorQuery() { return m_selectorQuery; };
> +
> +private:
> +    // m_querySelectorList keeps the lifetime of CSSSelectors in
m_selectorQuery.
> +    // Since m_selectorQuery just holds pointers to CSSSelector objects,
> +    // m_querySelectorList must not be destructed before m_selectorQuery is
destructed.
> +    CSSSelectorList m_querySelectorList;
> +    SelectorQuery m_selectorQuery;
> +};

Indentation.

> Source/WebCore/dom/SelectorQuery.h:99
> +    PassRefPtr<SelectorQueryCache::Entry> createNewEntry(const
AtomicString&, Document*, ExceptionCode&);
> +    PassRefPtr<SelectorQueryCache::Entry> findEntry(const AtomicString&,
Settings*);

A single function (maybe just add() since HashMap::add() has semantics like
that) should be sufficient. If the entry does not exists it creates it. This is
more efficient since we will only need one hash lookup when adding.

SelectorQueryCache::Entry could be a private class, the function could just
return SelectorQuery*.

> Source/WebCore/dom/SelectorQuery.h:100
> +    void invalidateEntries();

Just call it invalidate()

> Source/WebCore/page/Settings.h:330
> -	   void setCSSCustomFilterEnabled(bool enabled) {
m_isCSSCustomFilterEnabled = enabled; }
> +	   void setCSSCustomFilterEnabled(bool enabled) {
m_cssSelectorCacheInvalidated = true; m_isCSSCustomFilterEnabled = enabled; }
>	   bool isCSSCustomFilterEnabled() const { return
m_isCSSCustomFilterEnabled; }
>  
>  #if ENABLE(CSS_REGIONS)
> -	   void setCSSRegionsEnabled(bool enabled) { m_cssRegionsEnabled =
enabled; }
> +	   void setCSSRegionsEnabled(bool enabled) {
m_cssSelectorCacheInvalidated = true; m_cssRegionsEnabled = enabled; }

I don't think this invalidation code is needed at all. Flipping these settings
is not a user scenario. If someone changes them it is reasonable to expect them
to take full effect only on subsequent document loads. The existing code works
like this too (we don't reparse stylesheets for example).