Bug 60708 – make :any work with fastRejectSelector

• Status: NEW
• Component: New Bugs

[Ojan Vafai]

make :any work with fastRejectSelector

[Ojan Vafai]

Created attachment 93354 [details]
Patch

[Ojan Vafai]

Sorry about the huge patch. The code changes themselves are reasonably sized, but the two manual test cases are each 16k lines. I'm happy to move those into a separate patch if you'd like.

I'm a bit unhappy with the memory management in this patch. See the section in WebCore ChangeLog under deleteRareHashes. I tried working around it by doing something with VectorTraits to use memcpy, but that didn't address the case where a RuleData was created on the stack. I would love ideas on how to better handle this.

[Antti Koivisto]

I don't like this. The ancestor identifier filter specifically aims to speed up the common cases. Adding special handling for a rare case sort of misses the point.

This adds a branchiness to fastRejectSelector which is almost certainly enough to regress it (it currently inlines into a very tight loop).

One approach for optimizing :any might be to split them up into multiple RuleData entries, each with unique descendantSelectorIdentifierHashes but referencing the same RuleSet. This wouldn't work for multilevel :any (due to exponential growth) but simply optimizing one level would cover most realistic cases.

The performance test files are huge. Wouldn't it be possible to generate the test stylesheet programmatically?

[Antti Koivisto]

Comment on attachment 93354 [details]
Patch

r- based on comments above. At minimum there would need to be some convincing profiles showing that this does not regress the common case.

[Ojan Vafai]

(In reply to comment #3)
> One approach for optimizing :any might be to split them up into multiple RuleData entries, each with unique descendantSelectorIdentifierHashes but referencing the same RuleSet. This wouldn't work for multilevel :any (due to exponential growth) but simply optimizing one level would cover most realistic cases.

I like this much better. It's slightly less memory friendly, but I think it likely will simplify code and be more performant. This also should naturally fix bug 56991. For now, the spec (and our code) disallows nested :any, so we don't need to handle that case.

It does take away some of the potential performance benefits of using :any though, in that we we now need to selector match each RuleData instead of one. I think I'm OK with that given the code simplicity of this approach.

> The performance test files are huge. Wouldn't it be possible to generate the test stylesheet programmatically?

Yeah, sorry. Will fix.

[Ojan Vafai]

Just want to make sure I understand what you're proposing. For each RuleData that we create for a :any case, we'd need to create a new CSSSelector to stick in it, right? For example, for "div :any(h1, h2)" we'd create two RuleData objects and stick a new CSSSelector in each on ("div h1" and "div h2"). 

This would mean that each time we go to add a RuleData to a RuleSet, we would need to iterate over the CSSSelectors in the selector to make sure none of them are :any. If one of them is :any, then we need to iterate over can construct new CSSSelectors as we go.

This all seems fine to me, just wanted to run it by you.

[Antti Koivisto]

Basically you would have multiple RuleData objects that reference the same CSSStyleRule and CSSSelector and differ by m_descendantSelectorIdentifierHashes only. 

The only complication I see is that you don't want the same rule be applied multiple times (because css counters and similar might get confused). That should be easy and fast to filter at the end. The copied rules will have identical specificity/position and sortMatchedRules() will sort them next to each other.

[Antti Koivisto]

If :any is the topmost selector, their RuleData entries may actually be identical (but live as separate entires in RuleSet hashes that classify based on the topmost selector).

[Ojan Vafai]

Created attachment 98250 [details]
Patch

[WebKit Review Bot]

Attachment 98250 [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

Source/WebCore/css/CSSStyleSelector.cpp:3184:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

Created attachment 98253 [details]
Patch

[WebKit Review Bot]

Attachment 98253 [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

Source/WebCore/css/CSSStyleSelector.cpp:3186:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

Comment on attachment 98253 [details]
Patch

This patch almost certainly doesn't apply cleanly to trunk anymore. Removing the r? for now.