[webkit-reviews] review denied: [Bug 87942] [Performance] Optimize querySelector() by caching SelectorQuery objects : [Attachment 145027] 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 145027: Patch
https://bugs.webkit.org/attachment.cgi?id=145027&action=review

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


> Source/WebCore/css/CSSParser.cpp:9631
> +    if (!m_querySelectorList.first() ||
m_querySelectorList.hasUnknownPseudoElements()) {
> +	   ec = SYNTAX_ERR;
> +	   return;
> +    }

Exceptions should usually be generated on DOM interface classes, not on
internal types.

> Source/WebCore/css/CSSParser.h:497
> +class CSSSelectorCache : public RefCounted<CSSSelectorCache> {

This is more like SelectorQueryCacheEntry.

> Source/WebCore/css/CSSParser.h:501
> +    CSSSelectorCache() { };
> +    SelectorQuery& selectorQuery() { return m_selectorQuery; };
> +    void parseSelector(const String&, Document*, ExceptionCode&);

I think it would be better to just pass in the CSSSelectorList as a constructor
parameter instead of having parseSelector() here.

> Source/WebCore/css/CSSParser.h:505
> +    CSSSelectorList m_querySelectorList;
> +    SelectorQuery m_selectorQuery;

What do you need the m_querySelectorList as a member?

> Source/WebCore/dom/Document.cpp:748
> +PassRefPtr<CSSSelectorCache> Document::findCSSSelectorCache(const
AtomicString& selectors)
> +{
> +    Settings* currentSettings = settings();
> +    if (currentSettings && currentSettings->cssSelectorCacheInvalidated()) {

> +	   currentSettings->clearCSSSelectorCacheInvalidated();
> +	   invalidateCSSSelectorCache();
> +	   return 0;
> +    }
> +
> +    HashMap<AtomicString, RefPtr<CSSSelectorCache> >::iterator it =
m_cssSelectorCacheMap.find(selectors);
> +    if (it == m_cssSelectorCacheMap.end())
> +	   return 0;
> +    return it->second;
> +}

Bit sad to see all this stuff added to the already-bloated Document.

I think you should go with a different factoring. You would have a
SelectorQueryCache class that (either per-document or a singleton but better
start with per-document) that represents the cache. Most of the code and fields
you currently have on Document move this this class. What you now call
CSSSelectorCache becomes SelectorQueryCacheEntry (or
SelectorQueryCache::Entry).

> Source/WebCore/dom/Document.cpp:774
> +    invalidateCSSSelectorCache();

Instead of having the fragile invalidation scheme, I think you could just
include a copy of CSSParserContext used for parsing the selector along with the
cache (or cache entry). When restoring an entry you would check the context is
still equal. If not you throw it out.

What do you think?