Consider eliminating RuleProcessorData

RESOLVED FIXED in mozilla5

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 18 obsolete attachments)

29.08 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.15 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
12.33 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.99 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
21.62 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
17.50 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.20 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.79 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.22 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
29.02 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.71 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.07 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.84 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
32.60 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
19.40 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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.
I have patches to do this, but they've likely bitrotted.  I'll try to dust them off once the fx4 dust settles.
Depends on: 598833
Blocks: 608648
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.
Created attachment 497470 [details] [diff] [review]
imported patch stop-contentenumfunc-callback
Created attachment 497471 [details] [diff] [review]
Add RuleMatchingContext
Created attachment 497472 [details] [diff] [review]
Move mScopedRoot to RuleMatchingContext.
Created attachment 497473 [details] [diff] [review]
Stop using mIsHTML and mIsHTMLContent
Created attachment 497474 [details] [diff] [review]
Move mCompatMode
Created attachment 497475 [details] [diff] [review]
Remove mNthIndices
Created attachment 497476 [details] [diff] [review]
Kill mLanguage
Created attachment 497477 [details] [diff] [review]
kill mContentState
Created attachment 497478 [details] [diff] [review]
hoist NodeMatchContext higher up the callstack
Created attachment 497479 [details] [diff] [review]
Kill mContentTag
Created attachment 497480 [details] [diff] [review]
Remove mContentID, mHasAttributes, mNameSpaceID, mClasses
Created attachment 497481 [details] [diff] [review]
Remove mParentContent and mElement
Created attachment 497482 [details] [diff] [review]
Change around SelectorMatchesTree some
Created attachment 497483 [details] [diff] [review]
Make querySelector faster
Created attachment 497484 [details] [diff] [review]
Make the various rule processor data structs stack-only
Depends on: 631529
Blocks: 631837
Comment on attachment 497475 [details] [diff] [review]
Remove mNthIndices

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

MoFo
Yes.  And that patch is getting rewritten anyway to deal with bug 631529.
Depends on: 632137
Blocks: 631529
No longer depends on: 631529
Created attachment 510864 [details] [diff] [review]
part 1.  Move TreeMatchContext out to where things other than the rule processor can get at it.
Attachment #510864 - Flags: review?(dbaron)
Attachment #497471 - Attachment is obsolete: true
Created attachment 510866 [details] [diff] [review]
part 2.  Move tracking of the scope root to the TreeMatchContext.
Attachment #510866 - Flags: review?(dbaron)
Attachment #497472 - Attachment is obsolete: true
Created attachment 510867 [details] [diff] [review]
part 3.  Stop keeping track of whether the node or document is HTML in the RuleProcessorData.
Attachment #510867 - Flags: review?(dbaron)
Attachment #497473 - Attachment is obsolete: true
Attachment #497470 - Attachment is obsolete: true
Created attachment 510868 [details] [diff] [review]
part 4.  Move tracking of whether we're in quirks mode to the TreeMatchContext.
Attachment #510868 - Flags: review?(dbaron)
Attachment #497474 - Attachment is obsolete: true
Created attachment 510869 [details] [diff] [review]
part 5.  Move caching of nth-index stuff to the TreeMatchContext.
Attachment #510869 - Flags: review?(dbaron)
Attachment #497475 - Attachment is obsolete: true
Created attachment 510870 [details] [diff] [review]
part 6.  Stop caching the language of elements; just reget it from the DOM as needed.
Attachment #510870 - Flags: review?(dbaron)
Attachment #497476 - Attachment is obsolete: true
Created attachment 510874 [details] [diff] [review]
part 7.  Stop caching the content state of elements; just reget it from the DOM as needed.
Attachment #510874 - Flags: review?(dbaron)
Attachment #497477 - Attachment is obsolete: true
Created attachment 510890 [details] [diff] [review]
part 8.  Move NodeMatchContext higher up the callstack, so that we don't have to call IsLink as much.
Attachment #510890 - Flags: review?(dbaron)
Attachment #497478 - Attachment is obsolete: true
Created attachment 510891 [details] [diff] [review]
part 9.  Stop caching the element's local name; just get it from the DOM as needed.
Attachment #510891 - Flags: review?(dbaron)
Attachment #497479 - Attachment is obsolete: true
Created attachment 510892 [details] [diff] [review]
part 10.  Stop caching information about the element's attributes; just get it from the DOM as needed.
Attachment #510892 - Flags: review?(dbaron)
Attachment #497480 - Attachment is obsolete: true
Created attachment 510893 [details] [diff] [review]
part 11.  Stop caching the parent content pointer and the element pointer; just use the passed-in aElement.
Attachment #510893 - Flags: review?(dbaron)
Attachment #497481 - Attachment is obsolete: true
Created attachment 510894 [details] [diff] [review]
part 12.  Stop using RuleProcessorData in SelectorMatchesTree.
Attachment #510894 - Flags: review?(dbaron)
Attachment #497482 - Attachment is obsolete: true
Created attachment 510895 [details] [diff] [review]
part 13.  Stop using RuleProcessorData in the DOM-exposed selector-matching methods.
Attachment #510895 - Flags: review?(dbaron)
Attachment #497483 - Attachment is obsolete: true
Created attachment 510896 [details] [diff] [review]
part 14.  Make RuleProcessorData a stack-only class.
Attachment #510896 - Flags: review?(dbaron)
Attachment #497484 - Attachment is obsolete: true
Created attachment 510897 [details] [diff] [review]
part 15.  Make RuleProcessorData not inherit from TreeMatchContext anymore, so we can decouple the lifetimes.
Attachment #510897 - Flags: review?(dbaron)
Created attachment 510898 [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.
Attachment #510898 - Flags: review?(dbaron)
Whiteboard: [need review]
Created attachment 511645 [details] [diff] [review]
part 15.  Make RuleProcessorData not inherit from TreeMatchContext anymore, so we can decouple the lifetimes.
Attachment #511645 - Flags: review?(dbaron)
Attachment #510897 - Attachment is obsolete: true
Attachment #510897 - Flags: review?(dbaron)
Created attachment 511647 [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.
Attachment #511647 - Flags: review?(dbaron)
Attachment #510898 - Attachment is obsolete: true
Attachment #510898 - Flags: review?(dbaron)
Created 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.
Attachment #511754 - Flags: review?(dbaron)
Attachment #511647 - Attachment is obsolete: true
Attachment #511647 - Flags: review?(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
Attachment #510864 - Flags: review?(dbaron) → review+
Comment on attachment 510866 [details] [diff] [review]
part 2.  Move tracking of the scope root to the TreeMatchContext.

r=dbaron
Attachment #510866 - Flags: review?(dbaron) → review+
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
Attachment #510867 - Flags: review?(dbaron) → review+
Comment on attachment 510868 [details] [diff] [review]
part 4.  Move tracking of whether we're in quirks mode to the TreeMatchContext.

r=dbaron
Attachment #510868 - Flags: review?(dbaron) → review+
> 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 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
Attachment #510869 - Flags: review?(dbaron) → review+
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.)
Attachment #510870 - Flags: review?(dbaron) → review+
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
Attachment #510874 - Flags: review?(dbaron) → review+
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.)
Attachment #510890 - Flags: review?(dbaron) → review+
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
Attachment #510891 - Flags: review?(dbaron) → review+
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
Attachment #510892 - Flags: review?(dbaron) → review+
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
Attachment #510893 - Flags: review?(dbaron) → review+
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
Attachment #510894 - Flags: review?(dbaron) → review+
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
Attachment #510895 - Flags: review?(dbaron) → review+
Comment on attachment 510896 [details] [diff] [review]
part 14.  Make RuleProcessorData a stack-only class.

r=dbaron
Attachment #510896 - Flags: review?(dbaron) → review+
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.
Attachment #511645 - Flags: review?(dbaron) → review+
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
Attachment #511754 - Flags: review?(dbaron) → review+
> 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.
Whiteboard: [need review] → [need bug 598833 fixed?]
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need bug 598833 fixed?]
Target Milestone: --- → mozilla2.2
Though that might have been from bug 631837, which had this bug as a prerequisite.
Blocks: 630480
Depends on: 667520
Duplicate of this bug: 83834
You need to log in before you can comment on or make changes to this bug.