1228877 - nsStyleContext::HasChildThatUsesGrandancestorStyle is slow

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

I just took a profile in which 5% of the time was in nsStyleContext::HasChildThatUsesGrandancestorStyle .

(I was profiling the leaflet-osm-conversion branch of https://github.com/dbaron/timezone-map .  I'd definitely expect this to have some elements with very large numbers of different children.)

This seems trivial to fix; we should just set the bit on the parent and rename the bit to NS_STYLE_CHILD_USES_GRANDANCESTOR_STYLE

Comment 1

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

Attachment: Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild

Comment 2

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

Comment on attachment 8693379 [details] [diff] [review]
Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild

I was going to put this in heycam's queue for when he gets back, but given that I can't, I'll ask xidorn to review it...

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8693379 [details] [diff] [review]
Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild

Review of attachment 8693379 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a mostly straightforward refactoring.

r=me with some nits below.

::: layout/style/nsRuleNode.cpp
@@ +1714,4 @@
>      return;
>    }
>  
>    for (;;) {

It seems this loop could now be simplified to a tranditional "for" loop to make things more compact and clearer. (And the "if" above could probably be removed as well, so that we don't have two identical assertions.)

::: layout/style/nsStyleContext.h
@@ +246,5 @@
>                     "parent mismatch");
>      }
>    }
>  
> +  // Does any child this style context, or any of its descendants, have

Does any child *of*. Probably only "Does any of this context's descendants" is enough now.

Comment 4

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> It seems this loop could now be simplified to a tranditional "for" loop to
> make things more compact and clearer. (And the "if" above could probably be
> removed as well, so that we don't have two identical assertions.)

Indeed.   This also removes the requirement that callers check that the inherited-from context is not the parent, so I removed that condition from the initial assertion, although I didn't adjust either of the callers.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ed5feb6d47&group_state=expanded

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3c509bcb25a533b0eb161f2c5604962a837c37
Bug 1228877 - Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild.  r=xidorn

Comment 7

[Carsten Book [:Tomcat]]

sorry had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/196f2f131c70 since something in this push caused https://treeherder.mozilla.org/logviewer.html#?job_id=17983411&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a0113e1a540

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddaae692ddd9a7427fe18e93334d80a1f49bac96
Bug 1228877 - Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild.  r=xidorn

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ddaae692ddd9

Comment 11

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

Comment on attachment 8693379 [details] [diff] [review]
Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild

Probably good for heycam to have a look at this now that he's back.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8693379 [details] [diff] [review]
Make nsStyleContext::HasChildThatUsesGrandancestorStyle by setting bit on grandchild's parent instead of grandchild

I probably should have looked at this earlier, but the code is now gone. :-)