Closed Bug 1381071 Opened 7 years ago Closed 2 years ago

Consider caching getComputedStyle styles in display: none subtrees.

Categories

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

defect

Tracking

()

RESOLVED FIXED
103 Branch
Performance Impact low
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox103 --- fixed

People

(Reporter: mconley, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [Stylo])

Attachments

(3 files)

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/
Flags: needinfo?(b56girard)
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.
Flags: needinfo?(b56girard)
Hey Manish - anything actionable in here?
Flags: needinfo?(manishearth)
Whiteboard: [qf]
Emilio/heycam understand restyles better
Flags: needinfo?(manishearth)
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(cam)
(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.
Attached file 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...
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.
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 :-)
Assignee: nobody → emilio+bugs
Flags: needinfo?(emilio+bugs)
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...
Attachment #8886678 - Flags: feedback?(cam)
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).
Priority: -- → P1
Whiteboard: [qf] → [qf][Stylo]
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 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.
Flags: needinfo?(cam)
Attachment #8886678 - Flags: feedback?(cam) → feedback+
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.
(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.
(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.
Summary: Huge hang on Facebook, spending most of my time in Stylo style recalculations → stylo: Huge hang on Facebook, spending most of my time in Stylo style recalculations
(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.
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.
Whiteboard: [qf][Stylo] → [qf:p1][Stylo]
Priority: P1 → --
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?
Flags: needinfo?(emilio+bugs)
Yeah, I still want to get to this. Cameron is doing work around here for bug 1384824, so probably worth also waiting for that.
Flags: needinfo?(emilio+bugs)
Priority: -- → P2
Summary: stylo: Huge hang on Facebook, spending most of my time in Stylo style recalculations → stylo: Huge hang on Facebook, spending most of my time in style recalculations
(Clarify that this is just a style-heaving workload and isn't unique to stylo, per comment 6).
Depends on: 1389055
Depends on: 1389154
No longer blocks: stylo-site-issues
This doesn't need to block shipping stylo.
Priority: P2 → P4
Moving this bug to post 57 since it won't block QF for 57.
Whiteboard: [qf:p1][Stylo] → [qf:p2][Stylo]
Keywords: perf
status-firefox57=wontfix unless someone thinks this bug should block 57
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.
Flags: needinfo?(emilio)
Whiteboard: [qf:p2][Stylo] → [qf:i60][qf:p1][Stylo]
(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.
Whiteboard: [qf:i60][qf:p1][Stylo] → [qf:f60][qf:p1][Stylo]
Whiteboard: [qf:f60][qf:p1][Stylo] → [qf:f61][qf:p1][Stylo]
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.
Flags: needinfo?(emilio)
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
Flags: needinfo?(emilio)
Whiteboard: [qf:f61][qf:p1][Stylo] → [qf:f64][qf:p1][Stylo]
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.
Flags: needinfo?(emilio)
ok, at least not p1 anymore.
Whiteboard: [qf:f64][qf:p1][Stylo] → [qf:f64][qf:p3][Stylo]
(Retitling to make clear that the hang in comment 0 is fixed)
Summary: stylo: Huge hang on Facebook, spending most of my time in style recalculations → Consider caching getComputedStyle styles in display: none subtrees.
Whiteboard: [qf:f64][qf:p3][Stylo] → [qf:p3:f64][Stylo]
Blocks: 1469208
This optimization is so broken in WebKit... I'll land some tests for this.
Depends on: 1470087
Whiteboard: [qf:p3:f64][Stylo] → [qf:p3][Stylo]
Flags: needinfo?(emilio)
Flags: webcompat?
Whiteboard: [qf:p3][Stylo] → [qf:p3][Stylo] [webcompat]
Flags: webcompat? → webcompat+
Whiteboard: [qf:p3][Stylo] [webcompat] → [qf:p3][Stylo] [webcompat:p1]
Blocks: 1539582
Depends on: 1540220
Blocks: 1537903

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

Webcompat Priority: --- → P1
Blocks: 1590729
Whiteboard: [qf:p3][Stylo] [webcompat:p1] → [qf:p3][Stylo]

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

Webcompat Priority: P1 → ---
Performance Impact: --- → P3
Whiteboard: [qf:p3][Stylo] → [Stylo]

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.

Flags: needinfo?(emilio)

Nice :-)

Depends on: 1771564
Depends on: 1771713

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7a6d5cb7db2
Cache computed styles objects display: none subtrees. r=boris
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
Regressions: 1782071
Regressions: 1789997
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: