Created attachment 145805 [details] Patch RuleSet::addRule always uses the first simple selector component as the key for its rule set hashmap. This causes "#foo.bar" and ".bar#foo" to be optimized differently. Same for worse off cases like "input[type=text].foo". In RuleSet::addRule, the first simple selector would be an attribute selector rather than the class selector. So its inserted into the m_tagRules hash rather than m_classRules. This change promotes higher specificify selectors like IDs or classes to the front of the tag history chain. This ensures they'll be used as the most efficent index.
Comment on attachment 145805 [details] Patch Thanks for taking the time to do this. The code change looks like it’s a good one. Changes to WebKit need to include change log entries. Changes that say they speed things up need to include a test case demonstrating the speedup, or at least some test data showing what the effect of the change was when measuring performance.
Created attachment 146040 [details] CSS Selector Match performance test Run the CSS Profiler on the sample page. Without the patch, all the selectors will so up since they aren't being indexed correctly. With the patch, only the single relevant selector will.
Created attachment 146042 [details] Patch
Seems sensible. Could you add a performance test (under PerformanceTests/CSS) that shows improvement with this change?
Created attachment 146081 [details] Patch Performance Test results Before avg 980.6982122127804 runs/s median 981.5950920245399 runs/s stdev 5.581228266040944 runs/s min 958.7217043941412 runs/s max 985.2216748768473 runs/s After avg 1433.9319662152998 runs/s median 1435.8974358974358 runs/s stdev 8.980484754085072 runs/s min 1408.8050314465409 runs/s max 1447.0284237726098 runs/s
Comment on attachment 146081 [details] Patch r=me
Comment on attachment 146081 [details] Patch Rejecting attachment 146081 [details] from commit-queue. New failing tests: css1/box_properties/border_top_width.html css1/box_properties/border_style.html Full output: http://queues.webkit.org/results/12911023
Created attachment 146122 [details] Archive of layout-test-results from ec2-cq-01 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 146081 [details] Patch Attachment 146081 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12913114 New failing tests: fast/css/css-selector-text.html fast/css/css-set-selector-text.html css2.1/t0510-c25-pseudo-elmnt-00-c.html fast/dom/css-selectorText.html css1/pseudo/pseudo_elements_in_selectors.html
Created attachment 146206 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 146602 [details] Patch Adds an additional guard to prevent reordering selector if there is a pseudo element. Otherwise we might "fix" invalid selectors that have pseudo element selectors not at the end.
Attachment 146602 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 PerformanceTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebCore/css/CSSParserValues.cpp:111: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/css/CSSParserValues.cpp:112: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/css/CSSParserValues.cpp:116: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/css/CSSParserValues.cpp:117: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 6 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 146602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146602&action=review > Source/WebCore/css/CSSParserValues.cpp:118 > +bool CSSParserSelector::hasPseudoElement() > +{ > + CSSParserSelector* selector = this; > + while (selector) { > + if (selector->m_selector->matchesPseudoElement()) > + return true; > + selector = selector->tagHistory(); > + } > + return false; > +} WebKit coding style is to use 4 spaces for indentation, not 2. I think this could be written a little more clearly as a for loop. Do we need to worry about the performance implications of this extra loop?
(In reply to comment #13) > Do we need to worry about the performance implications of this extra loop? Antti, do you know?
Comment on attachment 146081 [details] Patch Cleared Antti Koivisto's review+ from obsolete attachment 146081 [details] so that this bug does not appear in http://webkit.org/pending-commit.
We made significant changes to the CSS parser (including merging Blink's improvements), so it doesn't seem like there is more to do here.