494117 - Don't rerun selector matching on the whole subtree unless we need to

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is spun off from bug 493651.  The basic issue is that right now any time the set of rules matched by a node has (possibly) changed we redo selector matching on that node and all descendants.  That seems rather unnecessary.  

https://bugzilla.mozilla.org/attachment.cgi?id=378394 is a quick hack prototype of an alternate setup, where we only reresolve descendants if the selector fragment that maybe changed whether it matches wasn't the rightmost in its selector.  That's similar to the way we handle restyling later children.  We do still need to give the kids style contexts with the correct new parent, but we can do that by just using the new parent and old rulenode; no need for new selector matching.

Obvious issues with that patch:

1)  The mIsNextOfSomething hack.  Ideally that would be a combinator test, but
    we use PRUnichar(0) as both the descendant combinator and the default
    combinator in right-most selectors.  Ideally we'd use different values for
    the two cases (a separate bug blocking this one?).
2)  The ReResolveStyleContext changes were a very quick hack; they might not be
    right.  Need careful thought there.
3)  There is a bug _somewhere_ in the undisplayed content handling; I got some
    asserts when just browsing zimbra's web UI with that patch in my tree.
4)  It's not clear to me whether this will correctly handle cases when a
    descendant also wants a style reresolve in terms of computing the correct
    change hint.  Needs some careful thought there as well.

The change does seem, in preliminary tests, to be effective in reducing the number of calls to SelectorMatches.  At least on the zimbra test suite.

I'm not sure how this would interact with the proposal in bug 479655.  It seems that they should get along ok, and that bug's approach might even make it easier to address item 4 in the list above.  But maybe we can just make this work independently of that change too.

Obvious questions:

* Worth doing?
* Zach: are you interested in taking it on?  ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

And a particular concern with #4 in that list is that for some cases the data returned by the old rulenode on descendants is more or less guaranteed to be bogus; hence my concerns about incorrect change computation....  Later we'll also reresolve style on the node in question, and compute a new change hint, but is that good enough?

Comment 2

[Zack Weinberg (:zwol)]

Attachment: slightly updated patch

Here's a slight update on Boris' original patch.  I took the suggestion of distinguishing descendant from end combinators in mOperator (point 1) so this  requires the change in bug 508466 to compile.  I am throwing both patches at the try server and will report, although I kinda doubt our unit tests cover this area very well.

There is some refactory in here that feels like it might be properly pulled out to separate patches -- 1) is done, 2 and 3 I'd like some advice on first.

1) Defined a new constant eReStyle_None instead of the obscure nsReStyleHint(0).
2) Most places spell it "Restyle", the use of "ReStyle" for nsReStyleHint and its enumerators tripped me up in four or five places.  Can we downcase the S?

3) Every caller of the two-argument GetContext() checks whether there is a pres context available, first.  Is that really necessary?  If it weren't, almost all of those functions would not need to look up the pres context.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Can we downcase the S?

Please! "Re" is not the word we want here :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 1) Defined a new constant eReStyle_None instead of the obscure
> nsReStyleHint(0).

Historically, dbaron's been opposed to that for bitfields... might wantto check with him.

> Can we downcase the S?

+1

> Is that really necessary?

For places where we got it using PresContext(), no.  That function never returns null anyway.

Comment 5

[Zack Weinberg (:zwol)]

> For places where we got it using PresContext(), no.  That function never
> returns null anyway.

That would be all of them except nsStyleSet::ReParentStyleContext() which gets it from the caller (and has an NS_ASSERTION to ensure it's valid).  nsStyleSet::ReParentStyleContext is only called by nsFrameManager::ReParentStyleContext, which uses GetPresContext() and does not check that it returned anything; however, it appears to me that nsFrameManager::GetPresContext can never return null.

It's tempting to zap all of that; the only thing that makes me hesitate is, for any given nsFrameManager object F, will F->GetPresContext() always return the *same* prescontext as F->mStyleSet->PresContext()?  I'm guessing yes, but the setup code for these objects is a maze.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, it will.

Comment 7

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

(In reply to comment #4)
> > 1) Defined a new constant eReStyle_None instead of the obscure
> > nsReStyleHint(0).
> 
> Historically, dbaron's been opposed to that for bitfields... might wantto check
> with him.

If you want a constant for the 0 case here, I'd at least insist that it be named in a pattern substantially different from the constants that are actually bits.  Otherwise people are tempted to use | to combine 0 with other bits; I've seen a number of cases where people did that and the names of the constants made it look like something that ought to work (although that's a little less likely here).

Comment 8

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

That said, I think nsRestyleHint(0) is a good way of saying "a restyle hint with no bits set".

Comment 9

[Zack Weinberg (:zwol)]

Attachment: mechanical replacement of 'ReStyle' with 'Restyle' [Checkin: comment 11]

I'm updating this bug's patch to the current trunk now that bug 508466 has finally landed.

As proposed in comment 2, this preliminary refactor changes 'nsReStyleHint' to 'nsRestyleHint' and 'eReStyle_Whatever' to 'eRestyle_Whatever'.  It is entirely mechanical:

$ find layout -type f -print0 | xargs -0 grep -El 'ReStyle(_|Hint)' | xargs perl -pi -e 's/ReStyle/Restyle/g'

Compiles fine, and I eyeballed the result for absurdities.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: revised patch

This is the meat of the change.  Still functionally the same as before.  I dropped the addition of eRestyleHint_None and cleaned up the callers of nsStyleSet::GetContext().

Comment 11

[Zack Weinberg (:zwol)]

Comment on attachment 436360 [details] [diff] [review]
mechanical replacement of 'ReStyle' with 'Restyle' [Checkin: comment 11]

Landed the downcasing refactor: http://hg.mozilla.org/mozilla-central/rev/bf065b08ef9f  Thanks for the quick review!

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think we really do need to change restyle processing per the proposal in bug 479655 before the patch in this bug will be safe...

Comment 13

[Zack Weinberg (:zwol)]

Comment on attachment 436369 [details] [diff] [review]
revised patch

(In reply to comment #12)
> I think we really do need to change restyle processing per the proposal in bug
> 479655 before the patch in this bug will be safe...

Yeah, my try server run blew up big time.

Clearing review for now.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: removal of prescontext argument to nsStyleSet::GetContext

I'm interested in working on bug 479655 but I have other things on my plate that are closer to being done and maybe I should do them first. :)  But we can at least pull the rest of the refactoring out and land it now.

This gets rid of the PresContext argument to nsStyleSet::ReParentStyleContext and nsStyleSet::GetContext.  (And downcases 'ReParentStyleContext' to 'ReparentStyleContext' in passing.)

Comment 15

[Zack Weinberg (:zwol)]

Attachment: mechanical replacement of 'ReParent' with 'Reparent'

I noticed a similar ugly and inconsistent use of 'ReParent' instead of 'Reparent' in a bunch of places while preparing attachment 436564 [details] [diff] [review], so this mechanical follow-up downcases the P throughout the tree.  Done with

$ grep -lr ReParent * | xargs perl -pi -e 's/ReParent/Reparent/g'

except that one occurrence (in toolkit/xre/nsNativeAppSupportWin.cpp) I just deleted, because it was a vestigial declaration of a function that was defined nowhere and used nowhere.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: revised main patch

Here's the main patch for this bug re-diffed atop the above two refactors.  This still doesn't work, of course, but it may save me (or whoever comes after) some hassle down the road.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436564 [details] [diff] [review]
removal of prescontext argument to nsStyleSet::GetContext

r=bzbarsky

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436566 [details] [diff] [review]
mechanical replacement of 'ReParent' with 'Reparent'

r=bzbarsky

Comment 19

[Zack Weinberg (:zwol)]

Landed http://hg.mozilla.org/mozilla-central/rev/0838e2acf420 and http://hg.mozilla.org/mozilla-central/rev/399da4539fe0

I'm going to bed - don't hesitate to back out if it causes trouble.

Comment 20

[Johnny Stenback (:jst)]

There's work left to be done here, right?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yes.

Comment 22

[Johnny Stenback (:jst)]

Attachment: Above revised patch "merged" to trunk.

This is what I've been testing with here (results to come), but I had to mess with nsStyleSet::ResolveStyleForNode() to get this to compile, and I have no idea if I got that right or not.

Comment 23

[Johnny Stenback (:jst)]

http://spreadsheets.google.com/pub?key=tXLptSQmdXKwqCMIjKRTP5w&output=html shows how the attached patch moves the needle on the Zimbra tests (that's only a subset of the tests, but one where there's a big difference between Firefox and Google Chrome). The rest of the tests seem virtually unaffected by this patch. Zack, what more do we need to do to the patch to get it ready? This patch alone gets us way closer to Google Chrome perf on these tests in Zimbra.

Comment 24

[Zack Weinberg (:zwol)]

jst: bug 479655 is the thing that needs doing first, AFAIK.

Comment 25

[Johnny Stenback (:jst)]

We need to work on bug 479655 first for correctness reasons, or to get even more out of the optimizations in this bug?

Comment 26

[Johnny Stenback (:jst)]

I found the answer to my own question in comment 12.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Main patch merged to trunk and on top of bug 479655

This is ready for review.

Comment 28

[Boris Zbarsky [:bzbarsky]]

If really desired I could split out the part which introduces the new hints from the different handling in the frame manager, but that's the only obvious way to split this patch while having working intermediate changesets....

Comment 29

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

Comment on attachment 447818 [details] [diff] [review]
Main patch merged to trunk and on top of bug 479655

nsHTMLStyleSheet::ImplLinkColorSetter seems like a nice fix -- how is it
related to this patch, though?  However, it seems like restyling the
root element (eRestyle_Subtree) would be better than a reconstruct
(which destroys a lot of cached data too), and ought to be equally
simple.

Also, fix the indent of the return here:
>-  if (mLinkRule) {
>-    if (mLinkRule->mColor == aColor)
>+  if (aRule && aRule->mColor == aColor) {
>       return NS_OK;
>   }




I presume you audited all callers of eRestyle_Self?  (I think this would
be easier to review if you renamed eRestyle_Self as well...  and,
perhaps, had a patch on top to change the name right back.)  I think
taking blame for those lines is fine, since eRestyle_Self is getting a
semantic change; its old semantics are being assigned to
eRestyle_Subtree.  (Or, perhaps even better, have a patch underneath
this one that renamed eRestyle_Self to eRestyle_Subtree, and then made
this patch reintroduce eRestyle_Self.)

I'd sort of like to review the output of:
  find . -regex ".*\.\(cpp\|h\)" -exec grep -H eRestyle_Self {} \;
run in layout/ with this patch applied, though.


nsChangeHint.h:

>- * and |HasAttributeDependentStyle|.  All values have an implied "and
>- * descendants."  When no restyling is necessary, use |nsRestyleHint(0)|.
>+ * and |HasAttributeDependentStyle|.  When no restyling is necessary, use |nsRestyleHint(0)|.

wrap at less than 80

nsFrameManager.cpp:

In nsFrameManager::ReResolveStyleContext, maybe better to just
not declare |childRestyleHint| until after aRestyleHint is adjusted
after the call to GetRestyleData (i.e., right where you adjust it
to remove eRestyle_Self)?

nsFrameManager.h:

Maybe drop "Bit of a hack;" ?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> how is it related to this patch, though?

The existence of the function is not, per se; I just didn't want to add 3 copies of the same code.  The "make sure we restyle any links" hunk, however, is needed for correctness.  The link color stuff is set up when style is resolved on the body, and used to assume that if we're restyling the body we will of course redo rule matching on all its descendant links.  With this patch, that's no longer true, though.

> However, it seems like restyling the root element (eRestyle_Subtree) would be
> better than a reconstruct

Yes, indeed.  Will make that change.

> I presume you audited all callers of eRestyle_Self?

Yes.

> Or, perhaps even better, have a patch underneath this one that renamed
> eRestyle_Self to eRestyle_Subtree, and then made this patch reintroduce
> eRestyle_Self.

Good idea.  I'll do that and post the resulting patches here.

> I'd sort of like to review the output of:

./style/nsCSSRuleProcessor.cpp:  return eRestyle_Self;
./style/nsHTMLStyleSheet.cpp:    return eRestyle_Self;
./style/nsHTMLStyleSheet.cpp:    return eRestyle_Self;
./style/nsHTMLStyleSheet.cpp:    return eRestyle_Self;
./style/nsHTMLCSSStyleSheet.cpp:    return eRestyle_Self;
./style/nsStyleSet.cpp:    data->mHint = eRestyle_Self;
./base/nsChangeHint.h:  eRestyle_Self = 0x1,
./base/nsPresShell.cpp:  // eRestyle_Self is ok here because animations are always tied to a
./base/nsPresShell.cpp:  FrameConstructor()->PostAnimationRestyleEvent(aElement, eRestyle_Self,
./base/nsFrameManager.cpp:    if (childRestyleHint == eRestyle_Self) {
./base/nsFrameManager.cpp:                                eRestyle_Subtree : eRestyle_Self,
./base/nsFrameManager.h:  // Bit of a hack; use eRestyle_Self for the aRestyleHint argument to
./base/RestyleTracker.cpp:  if (aRestyleHint & (eRestyle_Self | eRestyle_Subtree)) {
./base/RestyleTracker.h:  if ((aRestyleHint & (eRestyle_Self | eRestyle_Subtree)) ||

> wrap at less than 80

Done.

> maybe better to just not declare |childRestyleHint| until after aRestyleHint
> is adjusted after the call to GetRestyleData (i.e., right where you adjust it
> to remove eRestyle_Self)?

Indeed.  For some reason I thought there was useful stuff in that method outside the |if (oldContext)| block, but there isn't.  Done.

> Maybe drop "Bit of a hack;" ?

Done.

Comment 31

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

That list of remaining eRestyle_Self looks good.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Pushed rename:
  http://hg.mozilla.org/mozilla-central/rev/d594bc58ca6b

and the actual patch:
  http://hg.mozilla.org/mozilla-central/rev/dee1e84a95aa