1381071 - Consider caching getComputedStyle styles in display: none subtrees.

Status: RESOLVED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:56.0) Gecko/20100101 Firefox/56.0

Build ID: 20170714100307

I have layout.css.servo.enabled set to true, and about:support says:

"Stylo 	true (enabled by user)"


Unfortunately, I don't have great STR for this one. I just remember being on Facebook and clicking on something[1], and then the main thread in the content process hanging for a very long time. I eventually had to kill the JS execution (via the content process hang notification bar).

Luckily I was profiling at the time. I got the last 3 seconds or so of the hang in the content process:

https://perfht.ml/2tbDYBR

We're spending an awful long time to style recalculations in our Stylo code here. We seem to be entering it via JS - probably in a loop.

BenWa at Facebook was able to decode some of the obfuscated JS that's running here. I'm needinfo'ing him to comment further if he's able regarding what code is actually running here.

This profile was initially examined during The Joy of Profiling: Episode 6[2].

[1] Could have been anything. Possible the button to access my messages. I really can't remember.
[2]: https://air.mozilla.org/the-joy-of-profiling-episode-6/

Comment 1

[Benoit Girard (:BenWa)]

The time is spent in a function checking visibility. De-minified code is:
function isVisible(element: HTMLElement): boolean {
  // Look upward in the DOM until we either reach the document or
  // find something hidden.
  let elem = element;
  while (
    elem &&
    elem !== document &&
    Style.get(elem, 'visibility') != 'hidden' &&
    Style.get(elem, 'display') != 'none') {
    elem = elem.parentNode;
  }

  // If we reached the document, the element is visible.
  return elem === document;
}


Here we assume that once we've flushed then subsequent style queries will be fast. Guessing from the symbol names it looks like Stylo is doing the work lazily for each element and so we have to reflush lazily each time we walk up to the parent.

Looks like the you were either interacting with the Status composer or the a chat message tab.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Manish - anything actionable in here?

Comment 3

[Manish Goregaokar [:manishearth]]

Emilio/heycam understand restyles better

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(In reply to Benoit Girard (:BenWa) from comment #1)
> The time is spent in a function checking visibility. De-minified code is:
> function isVisible(element: HTMLElement): boolean {
>   // Look upward in the DOM until we either reach the document or
>   // find something hidden.
>   let elem = element;
>   while (
>     elem &&
>     elem !== document &&
>     Style.get(elem, 'visibility') != 'hidden' &&
>     Style.get(elem, 'display') != 'none') {
>     elem = elem.parentNode;
>   }
> 
>   // If we reached the document, the element is visible.
>   return elem === document;
> }
> 
> 
> Here we assume that once we've flushed then subsequent style queries will be
> fast. Guessing from the symbol names it looks like Stylo is doing the work
> lazily for each element and so we have to reflush lazily each time we walk
> up to the parent.
> 
> Looks like the you were either interacting with the Status composer or the a
> chat message tab.

Yes... We should cache somewhere the style we resolve for the parent, otherwise this is going to be super-slow... Leaving ni? for now.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: testcase

FWIW this is only a perf problem if the element is actually non-visible, because we optimize out the styles in display: none subtrees.

Here's a test-case, that shows the problem: The first isVisible call only takes a few milliseconds. The second one is still running...

Comment 6

[Mike Conley (:mconley) (:⚙️)]

This doesn't appear to be a Stylo-only bug, btw. I just tested with Stylo disabled, and we get the same hang with the old style stuff too.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1381071: Cache lazily-resolved styles in the element data.

Review commit: https://reviewboard.mozilla.org/r/157460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157460/

Comment 8

[Emilio Cobos Álvarez [:emilio]]

There's a proof-of-concept patch that fixes the issue... It has some drive-by cleanup, and it breaks invariants, in general, so even though I think it works as-is, I need to prove it and document the new invariants, if any :-)

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8886678 [details]
Bug 1381071: Cache lazily-resolved styles in the element data.

Worth having some feedback from Cam when I wake up tomorrow morning... :)

Cam, wdyt about this patch? I think this is the more straight-forward and elegant way to implement a fix for this. That being said, we could also store it in an out-of-band hashtable, or something...

We'd need to resolve eager pseudo-element styles to be correct too, I guess (or add a flag, etc), but we were doing that anyway before my refactor at bug 1379505...

Comment 10

[Emilio Cobos Álvarez [:emilio]]

So, I looked at this today again, and it's tricky to get working with our XBL setup (sigh), so I'm probably not going to get it done today.

That being said, the reason why you found this hang on Stylo and not on Gecko is probably https://github.com/servo/servo/pull/17751.

With that fixed, Stylo should be pretty much on par with Gecko. But I still think it's worth caching the style (because the testcase in the bug is really embarrasing).

Comment 11

[Cameron McCormack (:heycam)]

Hmm, so bug 1366964 should cause us to do half the style resolution that we are doing here (since we won't re-resolve style when looking up the display value after we look up the visibility value), but that's not enough.

Storing the style we compute for elements in display:none subtrees (and their ancestors) when we call getComputedStyle on them sounds OK to me.  If we are restyling and encounter the root of a display:none subtree, will we clear element data for its descendants?  Or will we continue to restyle them?  Seems easier not to restyle them.  Though if we do (and we'd need to ensure we don't generate damage for them), we might be able to remove the inexact cached style context invalidation in nsComputedDOMStyle that bug 1366964 added, at the expense of potentially wastefully styling these elements if an nsComputedDOMStyle object no longer needs them.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8886678 [details]
Bug 1381071: Cache lazily-resolved styles in the element data.

f+ though I didn't check whether we're doing the right things with dirty bits etc.

Comment 13

[Bobby Holley (:bholley)]

Hm, at our stylo meetup in september, I remember concluding that it was really hard to keep track of whether styles in display:none subtrees are up to date. If you leave them cached, you either need to continue restyling them on subsequent restyles (which is potentially wasteful), or not restyle them, which can cause them to get stale.

IMO the easiest solution is a Gecko-side hashtable from element->stylecontext for elements in display:none subtrees. When we restyle, we clear the hashtable.

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> Hm, at our stylo meetup in september, I remember concluding that it was
> really hard to keep track of whether styles in display:none subtrees are up
> to date. If you leave them cached, you either need to continue restyling
> them on subsequent restyles (which is potentially wasteful), or not restyle
> them, which can cause them to get stale.

If we cleared them out if we would have to enter the display:none subtree to restyle, that should avoid most of the tracking trickiness.  It seems like that should work with the invalidation propagation too, as we'd propagate our dirty bit up out of the styled element in the display:none subtree, and once we come to restyle, we'll just drop that subtree's element data.

> IMO the easiest solution is a Gecko-side hashtable from
> element->stylecontext for elements in display:none subtrees. When we
> restyle, we clear the hashtable.

That might be easier, yes.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> Hm, at our stylo meetup in september, I remember concluding that it was
> really hard to keep track of whether styles in display:none subtrees are up
> to date. If you leave them cached, you either need to continue restyling
> them on subsequent restyles (which is potentially wasteful), or not restyle
> them, which can cause them to get stale.

It is not, you just need to always clear during the next restyle. And we mostly do that, we only need to clear them also when we don't recompute the style of the display: none element. WebKit and Blink also do this fwiw.

> IMO the easiest solution is a Gecko-side hashtable from
> element->stylecontext for elements in display:none subtrees. When we
> restyle, we clear the hashtable.

That makes us move all the "does the parent has style" logic to the parent context. Which is fine, but a lot harder.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> > Hm, at our stylo meetup in september, I remember concluding that it was
> > really hard to keep track of whether styles in display:none subtrees are up
> > to date. If you leave them cached, you either need to continue restyling
> > them on subsequent restyles (which is potentially wasteful), or not restyle
> > them, which can cause them to get stale.
> 
> It is not, you just need to always clear during the next restyle. And we
> mostly do that, we only need to clear them also when we don't recompute the
> style of the display: none element.

How would we find and identify them? The naive approach would be to propagate the dirty descendant flag up from the display:none root after caching display:none styles in its descendants, but that would force a subsequent restyle when none is needed.

> 
> > IMO the easiest solution is a Gecko-side hashtable from
> > element->stylecontext for elements in display:none subtrees. When we
> > restyle, we clear the hashtable.
> 
> That makes us move all the "does the parent has style" logic to the parent
> context. Which is fine, but a lot harder.

I'm not sure what "the parent context" refers to here.

Are you referring to reusing existing styles when recursively resolving styles up the tree (resolve_style_internal etc)? Seems like that code could check the hashmap.

I'm certainly not opposed to caching on the elements if we have a clear and simple strategy to know when styles are up to date, which doesn't complicate the rest of the traversal invariants. I'm just not sure what that is yet.

Comment 17

[Bobby Holley (:bholley)]

Emilio and I discussed this a bit, and I thought about it more afterwards. I think we can make the in-tree thing work if we apply the following simple rule:

Every time a display:none root is restyled, we eagerly drop all style data from descendants.

Things that this handles:
* The display:block -> display:none case.
* the display:none -> display:block case, where we may have some unstyled elements deep in the subtree that we wouldn't otherwise be able to find. This is preferable to RestyleHint::subtree because that would cause us to compute restyle damage we don't need.
* The display:none -> display:none case (with either rematching or recascading), which may invalidate styles in the subtree.

Comment 18

[Daniel Holbert [:dholbert]]

I'm retriaging [qf:p1] bugs that haven't been touched in a little while -- Emilio, is this looking like it'll be doable for 57?

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Yeah, I still want to get to this. Cameron is doing work around here for bug 1384824, so probably worth also waiting for that.

Comment 20

[Bobby Holley (:bholley)]

(Clarify that this is just a style-heaving workload and isn't unique to stylo, per comment 6).

Comment 21

[Bobby Holley (:bholley)]

This doesn't need to block shipping stylo.

Comment 22

[Jean Gong :jgong]

Moving this bug to post 57 since it won't block QF for 57.

Comment 23

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 24

[Daniel Holbert [:dholbert]]

emilio, is this looking like something you'll have cycles for?  Sounds like the hangs here can be pretty painful, per comment 0, so it'd be great to get this fixed as part of this current QF iteration (targeting Firefox 60) if we can.

Comment 25

[Emilio Cobos Álvarez [:emilio]]

(In reply to Daniel Holbert [:dholbert] from comment #24)
> emilio, is this looking like something you'll have cycles for?  Sounds like
> the hangs here can be pretty painful, per comment 0, so it'd be great to get
> this fixed as part of this current QF iteration (targeting Firefox 60) if we
> can.

To be clear, the main issue here (and regression re. the old style system) was fixed in https://github.com/servo/servo/pull/17751.

It'd stil be nice to have the optimization, and should be easier now that I got rid of a bunch of "unstyled children only" traversal stuff.

But we'd need to be really careful in a couple of cases. I can still take a look at this.

Comment 26

[Emilio Cobos Álvarez [:emilio]]

So I thought a bit more about this today, and this is kinda hard to make sound in presence of XBL / Shadow DOM.

In particular, we use the presence of servo data to avoid tracking dynamic changes down display: none subtrees, and we never store data for elements not out of the flat tree and such.

Thus, we'd need to remove all the HasServoData checks that are used as short-cuts, or to replace them to something like IsInDisplayNoneSubtree() which also looks at the styles or what not...

That doesn't sound undoable, I'm going to try to remove a few and then stop relying on HasServoData so much. If I manage to do it then we can land the proposed patch almost without change.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Emilio, what is left here, and do you think that is worth for p1? In comment 25 you said the part of this was fixed already.
For now, p1

Comment 28

[Emilio Cobos Álvarez [:emilio]]

Comment 26 is what's left... I'm not sure it warrants a P1 unless we find a case where the current status is slow.

Self-reminder that in addition to comment 26 we also need to audit Servo_Element_IsDisplayNone calls on top of HasServoData.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

ok, at least not p1 anymore.

Comment 30

[Emilio Cobos Álvarez [:emilio]]

(Retitling to make clear that the hang in comment 0 is fixed)

Comment 31

[Emilio Cobos Álvarez [:emilio]]

This optimization is so broken in WebKit... I'll land some tests for this.

Comment 32

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating webcompat priority whiteboard tags to project flags.

Comment 34

[Karl Dubost💡 :karlcow]

The webcompat issue linked in that bug is not reproducing anymore. It's just a perf issue (maybe) for some unknown sites.

Comment 36

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1381071 - Cache computed styles objects display: none subtrees. r=boris,dholbert

This reuses our existing undisplayed style generation, but in a
per-document rather than per-nsComputedDOMStyle object, which means that
we can avoid re-resolving styles of elements in display: none subtrees
much more often.

This brings the test-case in the bug to par with other browsers or
better, and is much simpler than the initial approach I tried back in
the day.

Comment 37

[Bobby Holley (:bholley)]

Nice :-)

Comment 38

[Emilio Cobos Álvarez [:emilio]]

I had to fix a couple pre-existing under-invalidation issues, but should be green now:

https://treeherder.mozilla.org/jobs?repo=try&revision=0850057c9752cad29bbff59dbcf74cfbbad3899e

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7a6d5cb7db2
Cache computed styles objects display: none subtrees. r=boris

Comment 40

[Cristina Cozmuta (:CrissCozmuta)]

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