83836 - pull out initialization of SelectorMatchesData another level

Status: RESOLVED FIXED

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

Description

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

Right now we initialize the SelectorMatchesData within
nsCSSStyleSheetImpl::RulesMatching.  We could pull it outside of another loop
(into StyleSetImpl::WalkRuleProcessors) at the cost of some modularity.

Probably the easiest and the fastest way to do this is just to make the
SelectorMatchesData something that's usable by all the stylesheet classes, and
not just the CSSStyleSheetImpl.  This solution bugs me a bit but it's the only
one I can think of.

Any other thoughts on how me might do this?

Comment 1

[Pierre Saslawsky]

How about attaching part of the information to the RuleWalker?

Comment 2

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

Attachment: patch that doesn't really work yet

Well, I tried to do this, and this patch seems to slow things (i.e., jrgm's
tests) down by 10% and also break some cases of view positioning.  I guess I
have to figure out what I did wrong...

Comment 3

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

Attachment: working patch

This patch seems to work fine, and gives a 2.3% performance impromevent on
jrgm's tests.  (The key error in the previous patch was a "!" I didn't want
at the beginning of HTMLCSSStyleSheetImpl::RulesMatching(Pseudo...).)

In addition to pulling out the initialization of SelectorMatchesData, it:
 * simplifies the matching of :-moz-bound-element, since the scoped stylesheet
   handling code now has access to the enumeration data
 * fixes StyleSetImpl::WalkRuleProcessors to respect the enumerator's request
to
   halt enumeration in most cases, and clarified the difference between
   WalkRuleProcessors and FileRules.  (Perhaps we should use WalkRuleProcessors

   for AttributeAffectsStyle as well...?)

Comment 4

[Pierre Saslawsky]

2.3 percent? you rule! (pun intended)

Problem: I get the following assertion quite frequently...
    null pointer: 'aRuleWalker', file nsCSSStyleSheet.cpp, line 3225

Comment 5

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

oops.  I think I need to move the rule walker assertion into the
ContentRuleProcessorData and PseudoRuleProcessorData constructors and not the
StateRuleProcessorData constructor.  I hadn't done a debug build with these
changes yet...

Comment 6

[Chris Waterson]

r=waterson, although getting attinasi, fantasai, or pierre to take a look might
be a good idea.

Comment 7

[Chris Waterson]

Comment on attachment 57114 [details] [diff] [review]
working patch

r=waterson

Comment 8

[Marc Attinasi]

Comment on attachment 57114 [details] [diff] [review]
working patch

This looks fine to me. Nice job David!

Comment 9

[Pierre Saslawsky]

r=pierre with a fix for the assertion.

Note: the number of memory allocations at startup did not change (approx 284000) 
but it is not surprising because the SelectorMatchesData is allocated from an 
arena, but it means that the 2.3% percent performance improvement comes just from 
initializating the structure: wow!

Any chance we can have it approved 0.9.6?

Comment 10

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

Fix checked in 2001-11-10 15:51 PDT.

I don't think it's worth it for 0.9.6 - 2% isn't noticeable by users -- it's the
accumulation of multiple fixes that is noticeable.

Comment 11

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

It's interesting to note that the tinderbox stats showed the number of
RuleProcessorData constructed only fell by a factor of 2.9.  I'm guessing this
is because there are many arena-allocated ones at one level of the cascade for
some elements (probably for the descendant combinators in html.css / quirk.css).

Comment 12

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

Hmmm.  HP is giving a dynamic linker error at runtime:

 /usr/lib/dld.sl: Unresolved symbol: typeid__XT17RuleProcessorData_ (data)  from
/builds/tinderbox/SeaMonkey/HP-UX_B.11.00_Clobber/mozilla/dist/bin/components/libgklayout.sl
  /usr/lib/dld.sl: Unresolved symbol: __dt__17RuleProcessorDataFv (code)  from
/builds/tinderbox/SeaMonkey/HP-UX_B.11.00_Clobber/mozilla/dist/bin/components/libgklayout.sl

Comment 13

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

Well, I fixed the HP linker error by making the destructor for RuleProcessorData
non-virtual, which it should have been anyway.