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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({perf})

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

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.
Created attachment 409744 [details] [diff] [review]
Part 1.  Make sure pseudo-element selectors don't land in SelectorMatches
Attachment #409744 - Flags: review?(dbaron)
Created attachment 409746 [details] [diff] [review]
Part 2.  Macros to differentiate state-related pseudoclasses from others
Attachment #409746 - Flags: review?(dbaron)
Created attachment 409748 [details] [diff] [review]
Part 3.  Introduce an enum for pseudo-classes
Attachment #409748 - Flags: review?(dbaron)
Created attachment 409749 [details] [diff] [review]
Part 4.  Use the new enum in IsStateSelector
Attachment #409749 - Flags: review?(dbaron)
To be clear, I'm not entirely happy with the naming of the enumerated values; if people have better ideas I'm all ears.
Created attachment 409753 [details] [diff] [review]
Part 5.  The changes to SelectorMatches
Attachment #409753 - Flags: review?(dbaron)
Created attachment 409754 [details] [diff] [review]
For the record, alternate part 5 using switch
Created attachment 409756 [details] [diff] [review]
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.
Summary: Get rid of the pseudoclass if cascade in SelectorMatches → [FIX]Get rid of the pseudoclass if cascade in SelectorMatches
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
Attachment #409744 - Flags: review?(dbaron) → review+
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
Attachment #409748 - Flags: review?(dbaron) → review+
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
Attachment #409749 - Flags: review?(dbaron) → review+
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
Attachment #409753 - Flags: review?(dbaron) → review+
> 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.
You need to log in before you can comment on or make changes to this bug.