1354612 - Remove synchronous style flush related to the tab-audio indicator

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, enhancement, P2)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Follow-up from bug 1345440:

(In reply to :Ehsan Akhgari (busy) from comment #22)
> Jared, restyling browser.xul is *extremely* expensive and in fact that is
> often times the majority of the cost of things like sync reflows in profiles.
> 
> It turns out that we currently have some overhead specific to the Gecko
> profiler that inflates the cost of restyles (bug 1354255) which Markus is
> fixing, so unfortunately I cannot assert the above 100% confidently.  :-) 
> But we'll know as soon as that patch hits Nightly.  If it turns out my
> current measurements still hold after that, then I'd like to suggest that we
> should back this out.  Restyling the UI can easily take 30-40ms in profiles
> on my fast machine.  :-(
> 
> What do you think?  Thanks for considering this.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> Thanks, I'll note that since this landed we did a follow-up in bug 1352694
> (spoiler: still doing the style flush).
> 
> As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1345440#c3 I'm
> OK with wontfixing this bug and backing out the changeset.
> 
> However it's too bad there is no way to make this style "take effect"
> without the style flush. We will probably encounter this in other areas of
> animations within the Firefox UI and we don't have a solution that avoids
> the full style flush.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1354612 - Remove synchronous style flush since the performance cost of it is not worth the minor bug it is fixing.

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8855879 [details]
Bug 1354612 - Remove synchronous style flush since the performance cost of it is not worth the minor bug it is fixing.

https://reviewboard.mozilla.org/r/127752/#review130460

I mean, r=me, but is there no other way we can avoid this? Either by something other than a style flush (can we make it just affect that button/image/tab?) or by using some other mechanism to handle showing/hiding this button/img?

Also, 40ms for layout of just that bit of chrome, several orders of magnitude smaller than a typical webpage, seems excessive - even more so when it's really only the tab in question that needs restyling. Is someone looking at that? Especially partial style flushes should really be taking less time, plus this isn't actually *changing* any of the sizing of things outside of the tab itself.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I looked at this and left a comment in bug 1345440:
> FYI, I looked at a different route to fix this, which would be removing the
> transition-delay and using a setTimeout in its place. This is doable and
> would work, but I don't have the time to fix up the tests and potentially
> fix any regressions that come from that change.

But yes, I agree we should have a low-cost way to flush styles that are local. But maybe that is a "hard" problem with CSS due to styles affecting outer boxes? Note that the 40ms is likely larger than reality due to bug 1354255. We could re-measure before landing this to see if it is actually worth backing out.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

ni-myself to see if this is still very expensive now that some of the profiler-overhead has been reduced.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Where can I look on this profile to see the cost of the style flush?

https://perfht.ml/2o4p7T2

This is a profile without the attached patch applied, but after bug 1354255 has landed. I'm trying to see just how expensive this style flush is because I'd rather not re-introduce the bugs that this style flush fixes.

Comment 6

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

It's kinda hard to see where the style flush occurred. I see _some_ style flushes occurring - most around ~2-3ms, but I can't be sure which one is the one in question.

What you might want to do in this case, while we wait for the perf.html team to show us markers, is to manually instrument using window.performance.mark() and window.performance.measure() to get a delta (or window.performance.now() and do a subtraction) to get a sense of how long it took.

perf.html will, hopefully, be better at answering these sorts of questions in the short term.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

I'm calling window.performance.now() before and after the style flush and dumping the difference to the console.

```JS
if (modifiedAttrs.length) {
  // Flush style so that the opacity takes effect immediately, in
  // case the media is stopped before the style flushes naturally.
  let start = window.performance.now();
  getComputedStyle(tab).opacity;
  let end = window.performance.now();
  Services.console.logStringMessage(`style flush took ${end - start}ms`);
}
```

These are the numbers I'm getting with two tabs open:
style flush took 0.4249999999992724ms
style flush took 0.29500000000189175ms
style flush took 0.3150000000023283ms
style flush took 0.21000000000640284ms
style flush took 0.2550000000046566ms
style flush took 0.23500000000058208ms
style flush took 0.27499999999417923ms

These are the numbers I'm getting with two tabs with pages loaded and three about:newtabs:
style flush took 0.5350000000034925ms
style flush took 0.2649999999994179ms
style flush took 0.23500000000058208ms
style flush took 0.21000000000640284ms
style flush took 0.27999999999883585ms
style flush took 0.1900000000023283ms
style flush took 0.24500000002444722ms

Since these are averaging about .25ms, I would rather wontfix this bug it is actually fixing an issue in our code and only happens when media begins playing. @mconley, do you concur with wontfix?

Comment 8

[(no longer active)]

Thanks a lot for the measurements, Jared.  These make me feel a bit more comfortable.  I do suspect that what I had seen before was indeed inflated due to the profiler overhead I mentioned earlier.  I think wontfixing here sounds good.