1074683 - Status bar signal strength update triggers reflow/restyles

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

Attachment: 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.

Comment 1

[:gerard-majax]

Guillaume, git blame shows me this is code you introduced in bug 1042105 :)

Comment 2

[Guillaume Marty [:gmarty]]

Attachment: 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?

Comment 3

[Guillaume Marty [: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?

Comment 4

[:gerard-majax]

http://people.mozilla.org/~bgirard/cleopatra/#report=d295c79889830370a5aefcf50e7d0cfcabf32903

Comment 5

[:gerard-majax]

Second profiling, I'm not sure I see improvement :( http://people.mozilla.org/~bgirard/cleopatra/#report=c1f111d1cb897dc623444c3e620799319120727b

Comment 6

[Guillaume Marty [:gmarty]]

Comment on attachment 8497522 [details] [review]
Github PR

https://github.com/mozilla-b2g/gaia/pull/24832

Comment 7

[:gerard-majax]

Thanks, I'll try to provide feedback asap with the new patch :)

Comment 8

[:gerard-majax]

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

Comment 9

[:gerard-majax]

Vivien, needinfo? just so that you are aware of this, since it impacts reflows with the statusbar :)

Comment 10

[Guillaume Marty [:gmarty]]

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.

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8497522 [details] [review]
Github PR

Adding Etienne to the possible reviewer list.

Comment 12

[Etienne Segonzac (:etienne)]

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.

Comment 13

[Guillaume Marty [:gmarty]]

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?

Comment 14

[Guillaume Marty [:gmarty]]

Comment on attachment 8497522 [details] [review]
Github PR

https://github.com/mozilla-b2g/gaia/pull/24832

Comment 15

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

r=me with the small comments addressed

Comment 16

[Guillaume Marty [:gmarty]]

Landed: https://github.com/mozilla-b2g/gaia/commit/9b0b715d45928d865498ba90a8b99926053cb31c

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/9c3258717127c295b4c7167f846b76fd00337cf7