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().
Created attachment 144288 [details] Performance tests
Created attachment 144289 [details] Patch
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.
Created attachment 144324 [details] Patch
(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.
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.
Created attachment 144437 [details] Patch
(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.
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/
Comment on attachment 144437 [details] Patch Clearing flags on attachment: 144437 Committed r118736: <http://trac.webkit.org/changeset/118736>
All reviewed patches have been landed. Closing bug.
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.