Optimizing pseudo-style probing
Boris Zbarsky
figure out ways to optimize all the pseudo-style probing we do right now
(for :before/:after content, for first-line and first-letter, etc).
That stuff takes up up to 25% of frame construction time; the value is
lower if more complicated CSS is used on the page, of course.
Some thoughts that could use sanity-checking:
1) Instead of putting all the pseudo-element rules into a single linked
list per ruleprocessor, we could actually create a separate rulehash per
pseudo-element, or at least for before/after/first-line/first-letter.
Then within this rulehash we hash rules as we normally would. This
means, for example, that when probing ::before we don't have to do
SelectorMatches on all the UA ::before rules we have and can instead
just check against the ones that matter (usually none).
2) In RuleHash::EnumerateAllRules, check entry counts on the tables
before doing lookups. Lookups are not that cheap with pldhash, and
often enough the tables are completely empty (e.g. in the UA level the
id table is empty; in the user level most of the tables are empty by
default, etc).
3) Perhaps combine the rule processor data for the "main" style
resolution and the pseudo-probing, since the latter just uses the same
data with the additional pseudo-tag tossed in.
-Boris
L. David Baron
> I've been doing a bit of profiling of frame construction, and trying to
> figure out ways to optimize all the pseudo-style probing we do right now
> (for :before/:after content, for first-line and first-letter, etc). That
> stuff takes up up to 25% of frame construction time; the value is lower
> if more complicated CSS is used on the page, of course.
>
> Some thoughts that could use sanity-checking:
>
> 1) Instead of putting all the pseudo-element rules into a single linked
> list per ruleprocessor, we could actually create a separate rulehash per
> pseudo-element, or at least for before/after/first-line/first-letter.
> Then within this rulehash we hash rules as we normally would. This
> means, for example, that when probing ::before we don't have to do
> SelectorMatches on all the UA ::before rules we have and can instead
> just check against the ones that matter (usually none).
Separating pseudo-element rules would be useful for other things,
such as fixing IsPseudoElement to distinguish between :before (a
pseudo-element) and \:before (an element); see
https://bugzilla.mozilla.org/show_bug.cgi?id=475216#c1 . But that
requires distinguishing through even more of the process.
Maybe we could somehow have one rule hash for every pseudo-element
for which we actually have rules? That could avoid getting too
bloated (since in many cases there are no pseudo-element rules).
> 2) In RuleHash::EnumerateAllRules, check entry counts on the tables
> before doing lookups. Lookups are not that cheap with pldhash, and
> often enough the tables are completely empty (e.g. in the UA level the
> id table is empty; in the user level most of the tables are empty by
> default, etc).
Seems reasonable.
> 3) Perhaps combine the rule processor data for the "main" style
> resolution and the pseudo-probing, since the latter just uses the same
> data with the additional pseudo-tag tossed in.
Makes sense if it's easy; I haven't looked at how much code it would
require changing.
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Boris Zbarsky
> Maybe we could somehow have one rule hash for every pseudo-element
> for which we actually have rules? That could avoid getting too
> bloated (since in many cases there are no pseudo-element rules).
We could do that, but we'd have more rule hashes than one might think in
the UA level due to all the anon boxes that we treat as pseudo-elements
(most of which we do have rules for in ua.css). We'd also have a rule
hash per different made-up tree pseudo-element. It might be ok, though.
>> 3) Perhaps combine the rule processor data for the "main" style
>> resolution and the pseudo-probing, since the latter just uses the same
>> data with the additional pseudo-tag tossed in.
>
> Makes sense if it's easy; I haven't looked at how much code it would
> require changing.
Yeah, I haven't either. The relevant frame constructor code is in
enough flux in my tree right now that I didn't really want to experiment
against it until things stabilize.
I'll look into at least doing items 2 and 3.
-Boris
L. David Baron
> L. David Baron wrote:
>> Maybe we could somehow have one rule hash for every pseudo-element
>> for which we actually have rules? That could avoid getting too
>> bloated (since in many cases there are no pseudo-element rules).
>
> We could do that, but we'd have more rule hashes than one might think in
> the UA level due to all the anon boxes that we treat as pseudo-elements
> (most of which we do have rules for in ua.css). We'd also have a rule
> hash per different made-up tree pseudo-element. It might be ok, though.
Instead, we could have all the pseudo-element rules in a separate
hash, where that hash contains either a simple list of rules, or a
rule hash. We'd switch from simple-list to rule-hash if we ever got
a rule that would hash as non-universal. This would mean the anon
boxes wouldn't get a rule hash.
(Still not sure about the tree pseudo elements, though.)
Boris Zbarsky
>> We could do that, but we'd have more rule hashes than one might think in
>> the UA level due to all the anon boxes that we treat as pseudo-elements
>> (most of which we do have rules for in ua.css). We'd also have a rule
>> hash per different made-up tree pseudo-element. It might be ok, though.
>
> Instead, we could have all the pseudo-element rules in a separate
> hash, where that hash contains either a simple list of rules, or a
> rule hash. We'd switch from simple-list to rule-hash if we ever got
> a rule that would hash as non-universal. This would mean the anon
> boxes wouldn't get a rule hash.
>
> (Still not sure about the tree pseudo elements, though.)
I went back to this today, and I think there's a fairly straightforward
approach here by just throwing away my attempts at minimal invasiveness.
The CSS parser knows whether its parsing a pseudo-element, an
anonymous box, or a tree thing. All the callers except
ReResolveStyleContext know what sort of pseudo they're looking for, and
ReResolveStyleContext could be taught to (either by looking it up, or by
flagging the type on the style context). Therefore, proposal:
1) Have an enum for pseudo type; the values should basically be one
per CSS pseudo-element, eAnonBox, eTreePseudo, eNotPseudo or the
like.
2) For CSS anon boxes, forbid having anything but the name of the
anon box in the selector. Make anything else a CSS parse error
(and possibly assertion). The only current consumer I can find
at a quick glance which has something else in there is
"select::-moz-scrolled-content", and that rule can just
be removed now that bug 456484 styles all -moz-scrolled-content
that way.
3) When parsing a "pseudo-element" selector, store the pseudo-type
in it. If we care, we can make sure it only takes up 16 bits
by making the member a PRUint16 so it packs well with mCombinator.
4) Change consumers to use separate APIs to get style for tree
pseudos, pseudo-elements, and anonymous boxes. The anonymous box
API should not take an element (since it wouldn't be used for
anything anyway).
5) When setting up the rule cascade, put all the eNotPseudo rules in
the rule hash as now. Have a simple atom-to-linked-list hashtable
for anonymous boxes. Do something sane for trees (either a
separate rule hash or whatever). Have an array of pointers to rule
hashes, allocated as needed, for the pseudo-elements, with just an
array index and rulehash lookup if it's not null, as needed.
6) Remove all the null-checks for the mContent of a RuleProcessorData,
since anonymous boxes will no longer use it and everything else
will have a non-null mContent.
If there are any obvious issues, please speak up; I think this is solid
enough to just go and do.
Oh, and I did some measurements today on the single-page html5 spec. On
m-c, going from display:none to display:block on its <html> element
(just the frame construction) takes 1760ms or so. With the patches in
bug 523148 and bug 523288 it's closer to 1410. Of that 1410, about
140ms are in pseudo-style probing; my quick hack to do a slightly faster
path for :before and :after brought that down to 70ms, but I think we
might be able to win more there...
-Boris
Boris Zbarsky
> Do something sane for trees (either a separate rule hash or whatever).
I just realized that we actually have a quite finite number of tree
pseudo-elements (I thought we allowed arbitrary :-moz-tree-* stuff due
to the structure of nsCSSAnonBoxes::IsTreePseudoElement). Since we in
fact do not, and since it looks like :-moz-tree-whatever does in fact
need the current handling in the rule processor, more or less, I think
we should handle :-moz-tree the same way as we do pseudo-elements: an
array of rule hashes. They will almost never get allocated in practice
for web content, or in the UA/user levels.
-Boris
Boris Zbarsky
> I think we should handle :-moz-tree the same way as we do
pseudo-elements: an
> array of rule hashes.
On the gripping hand, the tree code is very much wrapped around the
nsIAtom setup for the moment, so I might just leave this as a hashtable
of rulehashes, keyed by atom.
-Boris
L. David Baron
> 2) For CSS anon boxes, forbid having anything but the name of the
> anon box in the selector. Make anything else a CSS parse error
> (and possibly assertion). The only current consumer I can find
> at a quick glance which has something else in there is
> "select::-moz-scrolled-content", and that rule can just
> be removed now that bug 456484 styles all -moz-scrolled-content
> that way.
For the record, this matches the original idea of the anon box vs.
pseudo-element distinction. Anonymous boxes weren't associated with
any particular real element, whereas pseudo-elements were.
So this is fine with me.