598832 - Consider eliminating RuleProcessorData

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Except for the nth-* stuff, everything else can be gotten off the node quickly enough nowadays to not regress matching on "real" pages, and some things speed up due to not having to create and destroy these structs.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I have patches to do this, but they've likely bitrotted.  I'll try to dust them off once the fx4 dust settles.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm going to post my wip queue here, merged to tip.  Not asking for review yet, because there are unresolved issues with the nth-child cache stuff and intrinsic state.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: imported patch stop-contentenumfunc-callback

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Add RuleMatchingContext

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Move mScopedRoot to RuleMatchingContext.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop using mIsHTML and mIsHTMLContent

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Move mCompatMode

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove mNthIndices

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Kill mLanguage

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: kill mContentState

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: hoist NodeMatchContext higher up the callstack

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Kill mContentTag

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove mContentID, mHasAttributes, mNameSpaceID, mClasses

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove mParentContent and mElement

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Change around SelectorMatchesTree some

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Make querySelector faster

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the various rule processor data structs stack-only

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 497475 [details] [diff] [review]
Remove mNthIndices

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

MoFo

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yes.  And that patch is getting rewritten anyway to deal with bug 631529.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move TreeMatchContext out to where things other than the rule processor can get at it.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Move tracking of the scope root to the TreeMatchContext.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop keeping track of whether the node or document is HTML in the RuleProcessorData.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Move tracking of whether we're in quirks mode to the TreeMatchContext.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Move caching of nth-index stuff to the TreeMatchContext.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Stop caching the language of elements; just reget it from the DOM as needed.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Stop caching the content state of elements; just reget it from the DOM as needed.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Move NodeMatchContext higher up the callstack, so that we don't have to call IsLink as much.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Stop caching the element's local name; just get it from the DOM as needed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Stop caching information about the element's attributes; just get it from the DOM as needed.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Stop caching the parent content pointer and the element pointer; just use the passed-in aElement.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Stop using RuleProcessorData in SelectorMatchesTree.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Stop using RuleProcessorData in the DOM-exposed selector-matching methods.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Make RuleProcessorData a stack-only class.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Make RuleProcessorData not inherit from TreeMatchContext anymore, so we can decouple the lifetimes.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Use a single TreeMatchContext for all the style resolution that the frame constructor does as part of a single frame construction batch.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Make RuleProcessorData not inherit from TreeMatchContext anymore, so we can decouple the lifetimes.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Use a single TreeMatchContext for all the style resolution that the frame constructor does as part of a single frame construction batch.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Use a single TreeMatchContext for all the style resolution that the frame constructor does as part of a single frame construction batch.

Comment 39

[David Baron :dbaron:]

Comment on attachment 510864 [details] [diff] [review]
part 1.  Move TreeMatchContext out to where things other than the rule processor can get at it.

>+ * A |TreeMatchContext| has data about a matching operation.  This
>+ * data is not node-specific but is an invariant of the DOM tree the

s/This data is/The data are/

>+  // Reset this data for :visited matching.  This should only be

s/for :visited matching/for matching for the style-if-:visited/, perhaps?

Also, I don't see why ResetForVisitedMatching sets mHaveRelevantLink
to false.  It's only called *after* mHaveRelevantLink is tested, as far
as I can tell.


I'm a little uncomfortable with the way the RuleProcessorData constructor
initializes mVisitedHandling in the case where there isn't a rule walker --
it's assuming a default -- but I'm hoping that goes away in a later patch.

r=dbaron with that

Comment 40

[David Baron :dbaron:]

Comment on attachment 510866 [details] [diff] [review]
part 2.  Move tracking of the scope root to the TreeMatchContext.

r=dbaron

Comment 41

[David Baron :dbaron:]

Comment on attachment 510867 [details] [diff] [review]
part 3.  Stop keeping track of whether the node or document is HTML in the RuleProcessorData.

r=dbaron

Comment 42

[David Baron :dbaron:]

Comment on attachment 510868 [details] [diff] [review]
part 4.  Move tracking of whether we're in quirks mode to the TreeMatchContext.

r=dbaron

Comment 43

[Boris Zbarsky [:bzbarsky]]

> s/This data is/The data are/
> s/for :visited matching/for matching for the style-if-:visited/, perhaps?

Done.

> Also, I don't see why ResetForVisitedMatching sets mHaveRelevantLink
> to false.  It's only called *after* mHaveRelevantLink is tested.

Not quite: we test mHaveRelevantLink in selector matching too.  But more importantly, once all is said and done we'll be reusing these objects for multiple matching operations, so I wanted to reset them "fully".

> but I'm hoping that goes away in a later patch.

Yeah, it basically does.

Comment 44

[David Baron :dbaron:]

Comment on attachment 510869 [details] [diff] [review]
part 5.  Move caching of nth-index stuff to the TreeMatchContext.

I think I should probably be on the Contributors line in the new files
too, since it is based on the existing code from bug 75375.

We do have a whole bunch of tests for this code, so I didn't feel like I needed to be *really* careful reviewing it.

r=dbaron

Comment 45

[David Baron :dbaron:]

Comment on attachment 510870 [details] [diff] [review]
part 6.  Stop caching the language of elements; just reget it from the DOM as needed.

r=dbaron

(I hope you're indentation's right.  I turned this into a diff -w.)

Comment 46

[David Baron :dbaron:]

Comment on attachment 510874 [details] [diff] [review]
part 7.  Stop caching the content state of elements; just reget it from the DOM as needed.

r=dbaron

Comment 47

[David Baron :dbaron:]

Comment on attachment 510890 [details] [diff] [review]
part 8.  Move NodeMatchContext higher up the callstack, so that we don't have to call IsLink as much.

r=dbaron

(I found the Pseudo and XULTree cases a little confusing, but I think that's largely due to my memory of how the code used to work.  I guess it's relatively straightforward that we'd be constructing a node match context for the actual node; I'm just not used to it.)

Comment 48

[David Baron :dbaron:]

Comment on attachment 510891 [details] [diff] [review]
part 9.  Stop caching the element's local name; just get it from the DOM as needed.

>+  if (mTagTable.entryCount) {
>     RuleHashTableEntry *entry = static_cast<RuleHashTableEntry*>
>-                                           (PL_DHashTableOperate(&mTagTable, aTag, PL_DHASH_LOOKUP));
>+                                           (PL_DHashTableOperate(&mTagTable, tag, PL_DHASH_LOOKUP));

Assert that |tag| is non-null?

> /* virtual */ nsRestyleHint
> nsHTMLStyleSheet::HasStateDependentStyle(StateRuleProcessorData* aData)
> {
>   if (aData->mElement->IsHTML() &&
>-      aData->mContentTag == nsGkAtoms::a &&
>+      aData->mElement->Tag() == nsGkAtoms::a &&

>@@ nsHTMLStyleSheet::HasAttributeDependentS
>       element->IsHTML() &&
>-      aData->mContentTag == nsGkAtoms::a) {
>+      aData->mElement->Tag() == nsGkAtoms::a) {
...
>         element->IsHTML() &&
>-        aData->mContentTag == nsGkAtoms::table) {
>+        element->Tag() == nsGkAtoms::table) {

In all three of these, use nsIContent::IsHTML(nsIAtom*) to combine the IsHTML and Tag() == checks.

r=dbaron with that

Comment 49

[David Baron :dbaron:]

Comment on attachment 510892 [details] [diff] [review]
part 10.  Stop caching information about the element's attributes; just get it from the DOM as needed.

r=dbaron

Comment 50

[David Baron :dbaron:]

Comment on attachment 510893 [details] [diff] [review]
part 11.  Stop caching the parent content pointer and the element pointer; just use the passed-in aElement.

r=dbaron

Comment 51

[David Baron :dbaron:]

Comment on attachment 510894 [details] [diff] [review]
part 12.  Stop using RuleProcessorData in SelectorMatchesTree.

>-    if (! data) {
>+    if (! element) {

Fix the out-of-style space while you're here.

r=dbaron

Comment 52

[David Baron :dbaron:]

Comment on attachment 510895 [details] [diff] [review]
part 13.  Stop using RuleProcessorData in the DOM-exposed selector-matching methods.

Can and should we assert that the document parameter to TreeMatchContext's constructor is non-null?

r=dbaron

Comment 53

[David Baron :dbaron:]

Comment on attachment 510896 [details] [diff] [review]
part 14.  Make RuleProcessorData a stack-only class.

r=dbaron

Comment 54

[David Baron :dbaron:]

Comment on attachment 511645 [details] [diff] [review]
part 15.  Make RuleProcessorData not inherit from TreeMatchContext anymore, so we can decouple the lifetimes.

Avoid the extra blank line at the start of nsStyleSet::HasAttributeDependentStyle

Also, I tend to wrap comments at tw=72... which explains some of the extra blame you're taking for parts of comments you're not modifying.  Rewrapping back to 72 might be nice.

Comment 55

[David Baron :dbaron:]

Comment on attachment 511754 [details] [diff] [review]
part 16.  Use a single TreeMatchContext for all the style resolution that the frame constructor does as part of a single frame construction batch.

r=dbaron

Comment 56

[Boris Zbarsky [:bzbarsky]]

> I think I should probably be on the Contributors line in the new files

Good point.  Done.

> Assert that |tag| is non-null?

Added an NS_ABORT_IF_FALSE to that effect after getting all the stuff up front in ContentEnumFunc.

> In all three of these, use nsIContent::IsHTML(nsIAtom*)

Done.

> Fix the out-of-style space while you're here.

Done.

> Can and should we assert that the document parameter to TreeMatchContext's
> constructor is non-null?

The constructor will crash if it's not, since it calls aDocument->IsHTML().  Not much point asserting it, I think, given that.

> Avoid the extra blank line at the start of
> nsStyleSet::HasAttributeDependentStyle

Done.

> Rewrapping back to 72 might be nice.

Done.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Pushed:

  http://hg.mozilla.org/mozilla-central/rev/111f5d2ac1d4
  http://hg.mozilla.org/mozilla-central/rev/062e9c1f0f12
  http://hg.mozilla.org/mozilla-central/rev/9e8a8d33d832
  http://hg.mozilla.org/mozilla-central/rev/cc247d27fd26
  http://hg.mozilla.org/mozilla-central/rev/578244f76827
  http://hg.mozilla.org/mozilla-central/rev/b97449d87066
  http://hg.mozilla.org/mozilla-central/rev/9620fda1e190
  http://hg.mozilla.org/mozilla-central/rev/d5e60cc85ae6
  http://hg.mozilla.org/mozilla-central/rev/507584f316f5
  http://hg.mozilla.org/mozilla-central/rev/0ac105c41106
  http://hg.mozilla.org/mozilla-central/rev/b6e6b9207590
  http://hg.mozilla.org/mozilla-central/rev/d0198ec8be5c
  http://hg.mozilla.org/mozilla-central/rev/3efc95473d4b
  http://hg.mozilla.org/mozilla-central/rev/49fa60035338
  http://hg.mozilla.org/mozilla-central/rev/5a3e00beb461
  http://hg.mozilla.org/mozilla-central/rev/c52e41b73b97

dromaeo_css, which uses querySelectorAll, apparently, got 20-25% faster.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Though that might have been from bug 631837, which had this bug as a prerequisite.