Closed Bug 1419096 Opened 2 years ago Closed 2 years ago

throbber consumes a lot of CPU and even more CPU when hidden

Categories

(Core :: Layout, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: f, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171115094908

Steps to reproduce:

Me and a friend use firefox with the tab bar hidden in userChrome.css, using:

#TabsToolbar {
  visibility: collapse;
}

We had already noticed that the throbber both in Tree Style Tabs tabs and in normal tabs consumes a lot of CPU, but in addition we also noticed that _more_ CPU is consumed when the tab bar is hidden. You can find more detailed information here, the impact on battery life is particularly disconcerting: https://github.com/piroor/treestyletab/issues/1384#issuecomment-345475606 .


Actual results:

High CPU and energy consumption when throbber active, and even more CPU when throbber active and hidden


Expected results:

The throbber should have limited impact on the CPU and should definitely not consume more CPU when the tab bar is hidden.
Component: Untriaged → Theme
We only track one issue per bug. Can you please clarify whether you want this bug to be about "throbber consumes a lot of CPU" or "throbber consumes more CPU when hidden"?

FWIW, visibility: collapse works like visibility: hidden under the hood, not like display: none. You shouldn't expect a performance win from this.
Flags: needinfo?(f)
(In reply to Dão Gottwald [::dao] from comment #1)
> We only track one issue per bug. Can you please clarify whether you want
> this bug to be about "throbber consumes a lot of CPU" or "throbber consumes
> more CPU when hidden"?

It does not make a difference to me, I can create a separate ticket if you like.

> FWIW, visibility: collapse works like visibility: hidden under the hood, not
> like display: none. You shouldn't expect a performance win from this.

The same problem exists with display: none, see <https://github.com/piroor/treestyletab/issues/1384#issuecomment-339512601>.
Flags: needinfo?(f)
jaws, can you make sense of this bug?
Blocks: 1352119
Flags: needinfo?(jaws)
Feels like having 2 performance profiles (see https://perf-html.io/ ), one with the indicators visible and one with the indicators hidden (probably best to check display:none) would help make sense of this.
This is because of bug 1237454.

(In reply to Francesco Mazzoli from comment #2)
> (In reply to Dão Gottwald [::dao] from comment #1)
> > We only track one issue per bug. Can you please clarify whether you want
> > this bug to be about "throbber consumes a lot of CPU" or "throbber consumes
> > more CPU when hidden"?
> 
> It does not make a difference to me, I can create a separate ticket if you
> like.
> 
> > FWIW, visibility: collapse works like visibility: hidden under the hood, not
> > like display: none. You shouldn't expect a performance win from this.
> 
> The same problem exists with display: none, see
> <https://github.com/piroor/treestyletab/issues/1384#issuecomment-339512601>.

CSS animations on elements in display:none subtree, so they shouldn't consume CPU.  But...  The throbber animation isn't a CSS animations? It's script animation?
Thank you, Simon.  Then the animation should stop when the element is in display:none subtree.  odd..
Component: Theme → Layout
Product: Firefox → Core
Priority: -- → P3
I don't see anything special in both profiles, but I've noticed the throbber animation in Tree Style Tabs does not specify 0% keyframe value, so if it could specify the 0% value, the high CPU usage should be fixed.  I did leave a comment in their issue, https://github.com/piroor/treestyletab/issues/1384#issuecomment-347713777 .
If I am seeing the correct @keyframes for the throbber animation, we don't specify 0% value either [1]

Jared, can we specify 0% value there?  transform: translateX(0px) is a good value, (we shouldn't use transform:none there, you can read the reason in bug 1419079 comment 35.)  Anyway, I think the high CPU usage is caused by bug 1419079. :/  Sorry for the inconvenience.

[1] https://hg.mozilla.org/mozilla-central/file/cb9092a90f6e/browser/themes/shared/tabs.inc.css#l188
See Also: → 1419079
Flags: needinfo?(jaws)
Yes, patch on the way. Though we didn't specify 0% value before because animations worked fine without it being specified as it would use the default value. We will now have to investigate our other @keyframes to make sure we have a 0% and 100% value explicitly specified.
Assignee: nobody → jaws
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jaws)
I filed bug 1421672 to fix the other instances of this in the tree.
Comment on attachment 8932937 [details]
Bug 1419096 - Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints.

https://reviewboard.mozilla.org/r/203942/#review209420

::: commit-message-cad9c:3
(Diff revision 1)
> +Bug 1419096 - Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints. r?dao
> +
> +See https://bugzilla.mozilla.org/show_bug.cgi?id=1419079#c22 for more information.

I'm not sure how to read this. This must be a bug, right? Will this go away with bug 1419079?
From my understanding, this optimization requires us to specify the 0% and 100% values and won't go away with bug 1419079. Hiro, can you confirm?
Flags: needinfo?(hikezoe)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> From my understanding, this optimization requires us to specify the 0% and
> 100% values and won't go away with bug 1419079. Hiro, can you confirm?

I can't answer whether bug 1419079 will fix this case or not as of now, especially for transform animation cases.  Currently I have the only one awkward approach in my mind to fix the transform animation cases, and I am not sure we take the approach in bug 1419079.
Flags: needinfo?(hikezoe)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> From my understanding, this optimization requires us to specify the 0% and
> 100% values

Yes, specifying 0% and 100% values is a prerequisite for offscreen animation optimizations.  Though there are many CSS properties that can't be optimized even if 0% and 100% values are specified, e.g. width, height, or some others which might affect layout.
ni?ing myself to make sure this gets added to https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers and the other front-end engineers are alerted to it.
Flags: needinfo?(mconley)
Comment on attachment 8932937 [details]
Bug 1419096 - Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints.

https://reviewboard.mozilla.org/r/203942/#review209702

I still don't understand the optimization and why the style system couldn't infer that information itself, but r+ for now
Attachment #8932937 - Flags: review?(dao+bmo) → review+
Comment on attachment 8932937 [details]
Bug 1419096 - Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints.

https://reviewboard.mozilla.org/r/203942/#review209702

I agree.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fcdfe8b0a97
Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints. r=dao
https://hg.mozilla.org/mozilla-central/rev/8fcdfe8b0a97
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Should we uplift this to 58, since it's a really simple fix?
(In reply to Marco Castelluccio [:marco] from comment #24)
> Should we uplift this to 58, since it's a really simple fix?

yes
Comment on attachment 8932937 [details]
Bug 1419096 - Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints.

Approval Request Comment
[Feature/Bug causing the regression]: performance regression noticed since optimizations require start/end values and we only specified one of the two
[User impact if declined]: extra CPU usage when loading pages
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: very simple CSS addition that explicitly specifies the default values
[String changes made/needed]: none
Flags: needinfo?(jaws)
Attachment #8932937 - Flags: approval-mozilla-beta?
Comment on attachment 8932937 [details]
Bug 1419096 - Explicitly declare 0% value in @keyframes animation because start/end values are needed to calculate cumulative change hints.

Fix a performance regression. Beta58+.
Attachment #8932937 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Redirecting ni? to jaws to get something added to https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers about this optimization.
Flags: needinfo?(mconley) → needinfo?(jaws)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #29)
> Redirecting ni? to jaws to get something added to
> https://developer.mozilla.org/en-US/Firefox/
> Performance_best_practices_for_Firefox_fe_engineers about this optimization.

Added to the doc at https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers#Explicitly_define_start_and_end_animation_values
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.