Status bar signal strength update triggers reflow/restyles

RESOLVED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gerard, Assigned: gmarty)

Tracking

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 1 obsolete attachment)

7.83 MB, application/x-geda-symbol
Details
46 bytes, text/x-github-pull-request
etienne
: review+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 8497343 [details]
profile_captured_highcpu_1.sym

Observed by profiling B2G process on a Nexus S, on a build from sept. 25th.

Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=9f32e34cd7c2628d1f7e26a9bf8bc3a0c6fa06a5

Triggered by StatusBar._getIconWidth() call.
(Reporter)

Comment 1

4 years ago
Guillaume, git blame shows me this is code you introduced in bug 1042105 :)
Depends on: 1042105
Flags: needinfo?(gmarty)
(Assignee)

Comment 2

4 years ago
Created attachment 8497522 [details] [review]
Github PR

Unfortunately, I'm afraid there is not much I can do to fix this. When the network icon is shown or hidden, we need to update the status bar and a reflow is required.

What I did in this PR is an optimisation that avoids reflows or status bar icons reprioritisation when either 'moznetworkupload' or 'moznetworkdownload' event are fired within 500ms. I'm not sure about the frequency of these events in real life, so it may or may not have a positive impact.

Alex, can you try with this patch to see if it's any better?
Attachment #8497522 - Flags: feedback?(lissyx+mozillians)
Flags: needinfo?(gmarty)
(Assignee)

Comment 3

4 years ago
I misread the original comment and thought it was the network icon. I left the original code as it won't hurt but also applied the same technique to the signal icons (that is reprioritise all icons only if a signal icon visibility changed). From my tests, the number of reflows dropped by about 10x.

Alex, can you have a look?
(Reporter)

Comment 7

4 years ago
Thanks, I'll try to provide feedback asap with the new patch :)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8497522 [details] [review]
Github PR

http://people.mozilla.org/~bgirard/cleopatra/#report=ce70e51f0e87a1da27831b81a6dc2259e14fa9af&search=statusbar

I cannot see any reflow anymore. I tested doing this:

 0. Build and flash an updated B2G for Nexus S with the proposed PR included
 1. Reboot device, unlock, wait ~1 min for things to stabilize
 2. Start profiling with: ./profile.sh start -p b2g -t Compositor,GeckoMain,Gecko_IOThread,GonkSensors,InputReader -i 1
 3. Keep screen awake, walk in the house for a couple of time making sure you see signal strength changing on the status bar
 4. Capture the profile
Attachment #8497522 - Flags: feedback?(lissyx+mozillians) → feedback+
(Reporter)

Comment 9

4 years ago
Vivien, needinfo? just so that you are aware of this, since it impacts reflows with the statusbar :)
Flags: needinfo?(21)
(Assignee)

Comment 10

4 years ago
Comment on attachment 8497522 [details] [review]
Github PR

Vivien, can you review this patch? Alexandre's profile results are consistent with my tests.
Also, let's keep this bug to track reflows of the signal icons. I'll add more status bar icons reflow optimisations in Bug 1075482 after this patch lands.
Attachment #8497522 - Flags: review?(21)
Comment on attachment 8497522 [details] [review]
Github PR

Adding Etienne to the possible reviewer list.
Attachment #8497522 - Flags: review?(etienne)
Attachment #8497522 - Flags: review?(21)
Comment on attachment 8497522 [details] [review]
Github PR

Looking good, small comment on github. There's not good way to test this for many reasons (including heavy radio dependency).

But I think we should break the rules a little and spy on the internal `_updateIconVisibility` in a few unit tests to make sure it's not called when the hidden state doesn't change.

Otherwise this improvement might regress too easily.
Attachment #8497522 - Flags: review?(etienne)
Attachment #8497522 - Flags: review?(21)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8497522 [details] [review]
Github PR

Same patch with unit tests to make sure the icon reprioritiser is only called when there is an actual change in the signal/data icons.

Etienne, how does it look like now?
Attachment #8497522 - Flags: review?(etienne)
Created attachment 8502455 [details] [review]
Gaia PR

r=me with the small comments addressed
Attachment #8497522 - Attachment is obsolete: true
Attachment #8497522 - Flags: review?(etienne)
Attachment #8502455 - Flags: review+
Flags: needinfo?(21)
Assignee: nobody → gmarty
(Assignee)

Comment 16

4 years ago
Landed: https://github.com/mozilla-b2g/gaia/commit/9b0b715d45928d865498ba90a8b99926053cb31c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8502455 [details] [review]
Gaia PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
bug 1042105

[User impact] if declined:
Doing network heavy apps, especially streaming apps (e.g. webrtc), CPU utilization will become extremely high, causing performance and battery issues. This was discovered by a partner in bug 1057038, and is a request that we uplift for it (see https://bugzilla.mozilla.org/show_bug.cgi?id=1057038#c59).

[Testing completed]:
This patch has unit tests, has been profiled by multiple people, and has been "baking" on master for about a week now.

[Risk to taking this patch] (and alternatives if risky):
Given that this patch has been on master for about a week now without any regressions, I think the risk is rather low now.

[String changes made]: none.
Attachment #8502455 - Flags: approval-gaia-v2.1?
Attachment #8502455 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S6 (10oct)
v2.1: https://github.com/mozilla-b2g/gaia/commit/9c3258717127c295b4c7167f846b76fd00337cf7
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
You need to log in before you can comment on or make changes to this bug.