523288 - [FIX]Speed up RuleProcessorData ctor and SelectorMatches a bit

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See the "RuleProcessorData and performance" thread in .layout.

I decided to go with a fairly conservative fix, in the end; just moving the obvious expensive stuff out of the constructor and reorganizing code a tad.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Move class/id matching earlier in SelectorMatches.

I also changed the class/id matching to just return PR_FALSE when not matching instead of lugging |result| around.  We might want to do a followup to get rid of |result| elsewhere too, but the attr matching was a bit daunting in that respect.  ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Main guts of the speed improvement: get mLinkState/mIsLink/mContentState lazily

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. A bit more constructor cleanup

Main changes here:

1)  Not writing most of the members twice in the ctor.  Those movl calls were
    showing up all over in the assembly.  Moving some more member init into our
    lazy getters.
2)  Not trying to get classes/id if we have no attributes; as long as we're
    getting the attr count anyway, we might as well do this.  Most nodes have no
    classes/ids.
3)  Adding mIsHTML to the RuleProcessorData, since we need this for the common
    case of tag selectors.
4)  Using mParentContent in a few places where for some reason we weren't.

Comment 4

[Boris Zbarsky [:bzbarsky]]

For reference, these give me about a 20% speedup or so on http://www2.webkit.org/perf/slickspeed/ (goes from 620ms to 500ms or so on my hardware) for the querySelector column.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 fixed to properly set mIsLink to false as needed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I just realized that part 1 means I should perhaps remove this comment in SelectorMatches:

  // The selectors for which we set node bits are, unfortunately, early
  // in this function (because they're pseudo-classes, which are
  // generally quick to test, and thus earlier).  If they were later,
  // we'd probably avoid setting those bits in more cases where setting
  // them is unnecessary.

David, thoughts?

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 407208 [details] [diff] [review]
Part 2.  Main guts of the speed improvement: get mLinkState/mIsLink/mContentState lazily

I had to make one of the assertions in SelectorMatches use LinkState() instead of mLinkState to make this compile in a DEBUG build.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, I think I might have made that change locally after posting the patches...  :(

Comment 9

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

Comment on attachment 407207 [details] [diff] [review]
Part 1.  Move class/id matching earlier in SelectorMatches.

>+        if (!elementClasses->Contains(classList->mAtom, isCaseSensitive ? eCaseMatters : eIgnoreCase)) {

Could you wrap this line at under 80 characters?

r=dbaron with that

Comment 10

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

Comment on attachment 407208 [details] [diff] [review]
Part 2.  Main guts of the speed improvement: get mLinkState/mIsLink/mContentState lazily

Maybe the |protected| section you're adding to should actually be 
|private|?  

Also, the comment for mGotIsLink should say it applies to both mIsLink 
and mLinkState.  (Maybe it should have a more neutral name?)

Maybe it's better not to initialize mIsLink and mContentState in the 
constructor?  Then valgrind would catch failure to initialize.  (Not 
sure which is faster, though; initializing big swaths could be faster.)
If you do, though, you need to change the assertion in LinkState to 
check mGotIsLink (and perhaps you should anyway).

r=dbaron

Comment 11

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

Comment on attachment 407321 [details] [diff] [review]
Part 3 fixed to properly set mIsLink to false as needed.

r=dbaron

(And it looks like this addresses the third of my comments on patch 2, except for the assertion in LinkState.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Could you wrap this line at under 80 characters?

Done.  

> Maybe the |protected| section you're adding to should actually be |private|? 

Done.  Made mLanguage and mNthIndices private too.

> (Maybe it should have a more neutral name?)

Renamed to mGotLinkInfo and commented better.

> change the assertion in LinkState to check mGotIsLink

Good catch.  Done.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 updated to tip and comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to tip and comments

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to tip and comments

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch I had used on tryserver for the measurements for this bug

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/1e9e3cd957f2
http://hg.mozilla.org/mozilla-central/rev/b3636bfdc6d9
http://hg.mozilla.org/mozilla-central/rev/a6508c37eb41