561516 - [FIX]Switch pseudo-class matching from an array of function pointers to a switch

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It benchmarked a tiny little bit slower on Mac back when, but we now have more arguments to the functions and maintaining this code is a pain if we change the arguments at all.  With the new arguments, I wouldn't be surprised if a switch is faster, since most cases don't actually need all the extra variables, and since on x86 we're now having to pass args to the callback functions on the stack.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

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

Comment on attachment 441238 [details] [diff] [review]
Like so

>-PR_STATIC_ASSERT(NS_ARRAY_LENGTH(sPseudoClassInfo) >
>+PR_STATIC_ASSERT(NS_ARRAY_LENGTH(sPseudoClassBits) >
>                    nsCSSPseudoClasses::ePseudoClass_NotPseudoClass);

Can't this be "== NotPseudoClass + 1" instead of "> NotPseudoClass"?


Don't you want to check sPseudoClassBits *outside* the switch and then 
make the default case of the switch be an NS_ABORT_IF_FALSE(PR_FALSE,
"...")?


I hope you were careful when you figured out what should be a return
PR_FALSE and what should be a break.  I just checked that there weren't
any |return PR_TRUE| left. :-)


r=dbaron with that

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Can't this be "== NotPseudoClass + 1" instead of "> NotPseudoClass"?

Yes.

> Don't you want to check sPseudoClassBits *outside* the switch

I could, I suppose.  I really wonder whether it would compile to the same thing...  Probably better to have the bits check outside the switch to be safe.

> I hope you were careful when you figured out what should be a return
> PR_FALSE and what should be a break.

I tried, yes.  I'll double-check them all again before pushing, and run tryserver of course.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well, I definitely had one bug at least: for :root I had an == where I should have had a !=.  Fixed locally, and review comments addressed.  Pushed to try again, and still need to do the double-checking.

Comment 5

[Boris Zbarsky [:bzbarsky]]

And I did verify that at least Linux gcc creates a jump table here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/330848610d95