Closed Bug 1074683 Opened 6 years ago Closed 5 years ago

Status bar signal strength update triggers reflow/restyles

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.1 S6 (10oct)
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: gerard-majax, Assigned: gmarty)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

7.83 MB, application/x-geda-symbol
Details
46 bytes, text/x-github-pull-request
etienne
: review+
Details | Review
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.
Guillaume, git blame shows me this is code you introduced in bug 1042105 :)
Depends on: 1042105
Flags: needinfo?(gmarty)
Attached file Github PR (obsolete) —
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)
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?
Thanks, I'll try to provide feedback asap with the new patch :)
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+
Vivien, needinfo? just so that you are aware of this, since it impacts reflows with the statusbar :)
Flags: needinfo?(21)
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)
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)
Attached file 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
Landed: https://github.com/mozilla-b2g/gaia/commit/9b0b715d45928d865498ba90a8b99926053cb31c
Status: NEW → RESOLVED
Closed: 5 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)
You need to log in before you can comment on or make changes to this bug.