Bug 87627 – [Performance] Optimize querySelector() and querySelectorAll() by removing redundant dummy style sheet creations

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kentaro Hara]

Currently Node::querySelector() and Node::querySelectorAll() have the following call path:

    PassRefPtr<Element> Node::querySelector(const String& selectors, ExceptionCode& ec)
    {
        CSSParser parser(document()); /* bottleneck (1) */
        CSSSelectorList querySelectorList;
        parser.parseSelector(selectors, querySelectorList);
        ...;
    }

    void CSSParser::parseSelector(const String& string, CSSSelectorList& selectorList)
    {
        RefPtr<StyleSheetInternal> dummyStyleSheet = StyleSheetInternal::create(); /* bottleneck (2) */
        setStyleSheet(dummyStyleSheet.get());
        ...;
    }

I've found that 'CSSParser parser(document())' and 'StyleSheetInternal::create()' are bottlenecks of querySelector() and querySelectorAll().

We do not need to construct a CSSParser and a dummyStyleSheet every time. By caching the CSSParser with a dummyStyleSheet on Document, we can remove the CSSParser constructor and the dummyStyleSheet creation from the call path of querySelector() and querySelectorAll().

[Kentaro Hara]

Created attachment 144288 [details]
Performance tests

[Kentaro Hara]

Created attachment 144289 [details]
Patch

[Antti Koivisto]

Comment on attachment 144289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144289&action=review

> Source/WebCore/css/CSSParser.cpp:1127
> +void CSSParser::parseSelectorWithDummyStyleSheet(const String& string, CSSSelectorList& selectorList)
> +{
> +    setDummyStyleSheet();
> +    parseSelector(string, selectorList);
>  }

Why does this case still need dummy stylesheet? Could you figure out how to get rid of it completely at least for selector parsing? I think it might be just matter of adding some null checks to code that deals with the stylesheet during the parsing.

[Kentaro Hara]

Created attachment 144324 [details]
Patch

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 144289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144289&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1127
> > +void CSSParser::parseSelectorWithDummyStyleSheet(const String& string, CSSSelectorList& selectorList)
> > +{
> > +    setDummyStyleSheet();
> > +    parseSelector(string, selectorList);
> >  }
> 
> Why does this case still need dummy stylesheet? Could you figure out how to get rid of it completely at least for selector parsing? I think it might be just matter of adding some null checks to code that deals with the stylesheet during the parsing.

Makes sense. Removed a dummy style sheet.

[Antti Koivisto]

Comment on attachment 144324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144324&action=review

> Source/WebCore/ChangeLog:83
> +        - We do not need to construct a CSSParser every time. This patch caches
> +        the CSSParser on Document, by which we can remove the CSSParser constructor
> +        from querySelector() and querySelectorAll().

This may be problematic as we grab bunch of settings and state from the document on CSSParser construction. If those change the cached parser instance will need to be invalidated. The cached instance could also be used more widely than just querySelector.

I think this part should be done in a separate patch. Lets focus on avoiding dummy stylesheet construction here.

[Kentaro Hara]

Created attachment 144437 [details]
Patch

[Kentaro Hara]

(In reply to comment #6)
> This may be problematic as we grab bunch of settings and state from the document on CSSParser construction. If those change the cached parser instance will need to be invalidated. The cached instance could also be used more widely than just querySelector.
> 
> I think this part should be done in a separate patch. Lets focus on avoiding dummy stylesheet construction here.

Makes sense. I'll do it in a separate patch with more investigation (Caching a CSSParser on Document is important since it improves performance by 30%~).

I uploaded a patch that just removes a dummy style sheet.

[Antti Koivisto]

Comment on attachment 144437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144437&action=review

r=me

> Source/WebCore/ChangeLog:12
> +        Performance tests: https://bugs.webkit.org/attachment.cgi?id=144288

You should add a version of the performance test to PerformanceTests/

[WebKit Review Bot]

Comment on attachment 144437 [details]
Patch

Clearing flags on attachment: 144437

Committed r118736: <http://trac.webkit.org/changeset/118736>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 144437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144437&action=review

> Source/WebCore/css/CSSGrammar.y:963
>          if (p->m_styleSheet)
> -            $$->setTag(QualifiedName(namespacePrefix, $2,
> -                                      p->m_styleSheet->determineNamespace(namespacePrefix)));
> -        else // FIXME: Shouldn't this case be an error?
> -            $$->setTag(QualifiedName(nullAtom, $2, p->m_defaultNamespace));
> +            $$->setTag(QualifiedName(namespacePrefix, $2, p->m_styleSheet->determineNamespace(namespacePrefix)));
> +        else
> +            $$->setTag(QualifiedName(namespacePrefix, $2, p->m_defaultNamespace));

A better way to factor this to avoid repeating an if statement three times is to make a CSSParser member function that does this same job. Something like this:

    $$->setTag(p->namespacedName(namespacePrefix, $4));

Not sure about the function name namespacedName, but with the right function name this would be a better factoring between CSSGrammar.y and CSSParser.h/cpp, which exists to provide helper functions like that.