525952 - [FIX]Get rid of the pseudoclass if cascade in SelectorMatches

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In some cases (e.g. the HTML5 spec) this if/else cascade uses a nontrivial number of cycles.  About 25% of the total SelectorMatches time on that page, to be precise.

Obvious solution is to replace the cascade with a table lookup.  I tried two different approaches: using an explicit array of function-pointer-or-state-bit and using a C++ switch() and letting the compiler optimize.  The former actually benchmarks a little bit faster on mac (about 1-2% of the total frame construction time faster); I did make sure the compiler generated a computed jump for the switch().  The generate assembly for the NS_FASTCALL call is actually pretty minimal: two reads from the stack to registers and then a call.  At least with gcc.  On Windows there might be a stack push also, since __fastcall only uses two registers, not the three we use for gcc with NS_FASTCALL...  So on Windows, this might benchmark differently.  I find the array code more readable, though.  I'll attach both, just in case.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Make sure pseudo-element selectors don't land in SelectorMatches

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Macros to differentiate state-related pseudoclasses from others

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Introduce an enum for pseudo-classes

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4. Use the new enum in IsStateSelector

Comment 5

[Boris Zbarsky [:bzbarsky]]

To be clear, I'm not entirely happy with the naming of the enumerated values; if people have better ideas I'm all ears.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5. The changes to SelectorMatches

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: For the record, alternate part 5 using switch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Also for the record, a part 6 using switch and reordering the cases to match the enum

I saw no performance benefit from this reordering; presumably gcc handles that itself.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 409744 [details] [diff] [review]
Part 1.  Make sure pseudo-element selectors don't land in SelectorMatches

It doesn't seem like all the null-checks in nsGenericElement.cpp are worth it; TryMatchingElementsInSubtree and nsCSSRuleProcessor::SelectorListMatches are perfectly null-safe, and it seems like this would never actually happen for real-world callers.

But maybe add a mochitest that this doesn't crash?

r=dbaron

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 409746 [details] [diff] [review]
Part 2.  Macros to differentiate state-related pseudoclasses from others

r=dbaron

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 409748 [details] [diff] [review]
Part 3.  Introduce an enum for pseudo-classes

r=dbaron, but it would be good to file a followup on changing the storage of the tree stuff and then getting rid of the atom in nsPseudoClassList

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 409749 [details] [diff] [review]
Part 4.  Use the new enum in IsStateSelector

This is ok, but really this should share the same array as the next patch (which should be file-scope).  (And it's fine if you merge the patches in the process of doing that.)

r=dbaron with that

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 409753 [details] [diff] [review]
Part 5.  The changes to SelectorMatches

>+          if (0 == (data.ContentState() & stateToCheck)) {

Use "!" rather than "0 == ".

r=dbaron with that, plus the comment on the previous patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

> But maybe add a mochitest that this doesn't crash?

Done, and removed the null-checks.

> file a followup on changing the storage of the tree stuff and then getting rid
> of the atom in nsPseudoClassList

Bug 531957 filed.

> should share the same array as the next patch

Done.

> Use "!" rather than "0 == ".

Done.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/626e14ae4183
http://hg.mozilla.org/mozilla-central/rev/5bcdfa447ae5
http://hg.mozilla.org/mozilla-central/rev/40b14411b05a
http://hg.mozilla.org/mozilla-central/rev/9af2a428dcb1