1179723 - Refactor the statusbar in order to use flex instead of recalculating icons width

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Status bar, Utility tray, Notification, defect)

Description

[Alberto Pastor [:albertopq]]

- Remove minimized statusbar
- Change statusbar width depending on the rocketbar
- Use flex-wrap for hiding the low priority icons

Comment 1

[Alberto Pastor [:albertopq]]

:ting, do you think you could provide some metrics about how this patch helps (or not) to the app loading issue (bug 1110625)? Thanks!

Comment 2

[Ting-Yu Chou [:ting] (away)]

Sure, but I don't see any patches here?

Comment 3

[Alberto Pastor [:albertopq]]

Attachment: Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Comment 4

[Alberto Pastor [:albertopq]]

Sorry! Patch attached :)

Comment 5

[Ting-Yu Chou [:ting] (away)]

Cool, if you don't mind I'll check this next week. Keep NI.

Comment 6

[Ting-Yu Chou [:ting] (away)]

I captured two profiles on Flame 1024MB while launching Clock app, one without the patch [1], and one with the patch [2]. Be noted followings are rough numbers as there're missing samples.

+---------------------------+---------------+---------------+
|                           | Without patch | With patch    |
+---------------------------+---------------+---------------+
| number of refresh driver  | 6:  25ms,     | 5:  120ms,    |
| ticks before titlechange  |     75ms,     |      70ms,    |
|                           |      8ms,     |      34ms,    |
|                           |     40ms,     |      33ms,    |
|                           |     28ms,     |      43ms     |
|                           |     40ms      |               |
|                           |    -----      |     -----     |
|                           |    216ms      |     300ms     |
+---------------------------+---------------+---------------+
| awf_launch()              |    130ms      |     200ms     |
| awm_switchApp()           |     20ms      |      50ms     |
| _closingTimeout()         |     75ms      |      37ms     |
|                           |    -----      |     -----     |
|                           |    225ms      |     287ms     |
+---------------------------+---------------+---------------+

With the patch, the first restyle computes style change for #windows, #utility-tray, #rocketbar, #fullscreen-software-home-button, and #lockscreen-masked-background which are not observed from the profile without patch.

For awf_launch(), the profiles lost many samples, I am not sure which part of JS causes the difference.

Gecko: f2518b8a7b97
Gaia: 31e595f86f6bf159b3a9a46816a6ac00a55ca9f9

[1] http://people.mozilla.org/~bgirard/cleopatra/#report=f63e14cc77505d5e5ae459ebfa41f80ff2f7963d
[2] http://people.mozilla.org/~bgirard/cleopatra/#report=fc52a9001eb49806da2847e9a6d008bbf105617f

Comment 7

[Wilson Page [:wilsonpage]]

I took this profile [1] and things look better to me. Statusbar is no longer causing a reflow before first paint like this profile [2] shows.

[1] https://people.mozilla.org/~bgirard/cleopatra/#report=c5fdb3127e6a9cd750a23770142395955aca0ab2
[2] https://people.mozilla.org/~bgirard/cleopatra/#report=c5fdb3127e6a9cd750a23770142395955aca0ab2

Comment 8

[Alberto Pastor [:albertopq]]

:ting, given comment #7, could you please help us to get some numbers, in order to improve the patch? What are you using for getting the numbers you posted? Thanks!

Comment 9

[Ting-Yu Chou [:ting] (away)]

Both Wilson's profiles do not have the samples in B2G process when content process receives the URL to launch, which can't see anything. It should use "-e 2000000" or some larger number to keep more entries while profiling.

I am not sure what else numbers you need beside comment 6, those numbers are from the profiles. If you try to click and drag in the samples, it will show you how long are the samples you select.

Comment 10

[Ting-Yu Chou [:ting] (away)]

Maybe start from removing the unnecessary restyles to improve the patch?

Comment 11

[Alberto Pastor [:albertopq]]

After talking to :wilsonpage, we have some ideas on why this is happening, but I'm not sure I'm profiling correctly (I don't see any GPU values in cleopatra). Could you please provide the commad you use for starting the profiler? Thanks!

Comment 12

[Ting-Yu Chou [:ting] (away)]

Sure, I usually use this command to profile both b2g and preallocated process for application launch:

  ./profile.sh start -p b2g -t GeckoMain,Compositor -f stackwalk,js -e 2000000 && ./profile.sh start -p [preallocated process pid] -f stackwalk,js -e 2000000

Comment 13

[Alberto Pastor [:albertopq]]

Ok, I think I made some progress. Could you please check out again the patch and confirm that the numbers improved? Thanks a lot for your help!

Comment 14

[Ting-Yu Chou [:ting] (away)]

Do you mind profile and measure it by yourself? I'm glad to answer questions.

Comment 15

[Alberto Pastor [:albertopq]]

Sure! Are you in IRC? This is not the best channel to these questions, and it would be useful to check both the same profile. Thanks!

Comment 16

[Ting-Yu Chou [:ting] (away)]

Yes, just look for ting. You can also email me as I'm in UTC+8.

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

Comment 18

[Alberto Pastor [:albertopq]]

Comment on attachment 8661678 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

It would be great to have Etienne input on this. It seems that we have 4 reflows instead of 2 when switching apps. I guess that now that we have 1 element with different widths (instead of 2 elements that we switch visibility) it will have some impact. The code looks way more clean, though... I'm not sure either if having 4 reflows, but removing 50% of the code (that gets executed on every app startup, expand, collapse...) for the statusbar is better or worse. Any suggestion (or help) on measuring it? Thanks!

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8661678 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

I'd love to look at it but I don't think this is the right pull request :)
It's full of pin-the-web stuff and no statusbar changes...

Comment 20

[Alberto Pastor [:albertopq]]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Now with the correct patch. Thanks!

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Wow this a breath of fresh air. So much cleaner :)

Here's a few suggestions that should help with reflows:

* toggling the |.hidden| on the #statusbar should not cause a reflow but currently it is. To fix it you need to set a |transform| at all time, this way the |.hidden| class will change the transform but no "set" a transform.

Tested locally and adding
```
#statusbar {
  transform: translateY(0);
}

```
does the trick :)

* with this implemented I don't think the pause/resume mechanic is buying us anything anymore, and it's quite a bit of code/logic. I'd suggest removing it.

* with all the cleanup and the pause/resume logic removed I'm very tempted to suggest a little fast path that should help with performance most of the time for app launch.

The idea is not to toggle the |.maximized| class (which is the only expensive thing left) when there's not many icons to display.
This way during simple use we won't have to do any statusbar work on app launch / app switch.

To implement it I'd suggest checking |document.querySelectorAll('#statusbar-icons .sb-icon:not([hidden])').length| in |setAppearance|.
- if <= 5, just set a boolean flag to remember that we took the fast patch
- else, set the |.maximized| class like we normally do, clearing the flag

Then adding back a listener to |iconshown|, and if the "we took a shortcut" flag is on, toggle the |.maximized| class as needed and reset the flag when an icon is displayed.


What do you think?

Comment 22

[Alberto Pastor [:albertopq]]

Thanks a lot for the feedback! Inline:

(In reply to Etienne Segonzac (:etienne) from comment #21)
> Comment on attachment 8653957 [details] [review]
> Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941
> 
> Wow this a breath of fresh air. So much cleaner :)
> 
> Here's a few suggestions that should help with reflows:
> 
> * toggling the |.hidden| on the #statusbar should not cause a reflow but
> currently it is. To fix it you need to set a |transform| at all time, this
> way the |.hidden| class will change the transform but no "set" a transform.
> 
> Tested locally and adding
> ```
> #statusbar {
>   transform: translateY(0);
> }
> 
> ```
> does the trick :)

I thought I already did that :/

https://github.com/mozilla-b2g/gaia/pull/30941/files#diff-19076406e0c835797eb201e0143532c7R8

> 
> * with this implemented I don't think the pause/resume mechanic is buying us
> anything anymore, and it's quite a bit of code/logic. I'd suggest removing
> it.

Yeah! And that will make the code even cleaner! \o/

> 
> * with all the cleanup and the pause/resume logic removed I'm very tempted
> to suggest a little fast path that should help with performance most of the
> time for app launch.
> 
> The idea is not to toggle the |.maximized| class (which is the only
> expensive thing left) when there's not many icons to display.
> This way during simple use we won't have to do any statusbar work on app
> launch / app switch.
> 
> To implement it I'd suggest checking
> |document.querySelectorAll('#statusbar-icons
> .sb-icon:not([hidden])').length| in |setAppearance|.
> - if <= 5, just set a boolean flag to remember that we took the fast patch
> - else, set the |.maximized| class like we normally do, clearing the flag
> 
> Then adding back a listener to |iconshown|, and if the "we took a shortcut"
> flag is on, toggle the |.maximized| class as needed and reset the flag when
> an icon is displayed.
> 
> 
> What do you think?

I like the idea! I would suggest an alternative way to implement it, though:

- Calculate once the (conservative) mean number of icons in a minimized statusbar (to avoid getting tied to an specific number like 5)
- Keep a counter on every iconshown/iconhidden
- Compare the counter with the number of icons calculated in the first place. Add the maximized class or not, depending on that check.

That way we avoid querySelectors + it would work all future device sizes.

What do you think?

Comment 23

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #22)
> Thanks a lot for the feedback! Inline:
> 
> (In reply to Etienne Segonzac (:etienne) from comment #21)
> > Comment on attachment 8653957 [details] [review]
> > Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941
> > 
> > Wow this a breath of fresh air. So much cleaner :)
> > 
> > Here's a few suggestions that should help with reflows:
> > 
> > * toggling the |.hidden| on the #statusbar should not cause a reflow but
> > currently it is. To fix it you need to set a |transform| at all time, this
> > way the |.hidden| class will change the transform but no "set" a transform.
> > 
> > Tested locally and adding
> > ```
> > #statusbar {
> >   transform: translateY(0);
> > }
> > 
> > ```
> > does the trick :)
> 
> I thought I already did that :/
> 
> https://github.com/mozilla-b2g/gaia/pull/30941/files#diff-
> 19076406e0c835797eb201e0143532c7R8

Weird, let's keep an eye out for this in the final version of this patch.

> 
> > 
> > * with this implemented I don't think the pause/resume mechanic is buying us
> > anything anymore, and it's quite a bit of code/logic. I'd suggest removing
> > it.
> 
> Yeah! And that will make the code even cleaner! \o/
> 
> > 
> > * with all the cleanup and the pause/resume logic removed I'm very tempted
> > to suggest a little fast path that should help with performance most of the
> > time for app launch.
> > 
> > The idea is not to toggle the |.maximized| class (which is the only
> > expensive thing left) when there's not many icons to display.
> > This way during simple use we won't have to do any statusbar work on app
> > launch / app switch.
> > 
> > To implement it I'd suggest checking
> > |document.querySelectorAll('#statusbar-icons
> > .sb-icon:not([hidden])').length| in |setAppearance|.
> > - if <= 5, just set a boolean flag to remember that we took the fast patch
> > - else, set the |.maximized| class like we normally do, clearing the flag
> > 
> > Then adding back a listener to |iconshown|, and if the "we took a shortcut"
> > flag is on, toggle the |.maximized| class as needed and reset the flag when
> > an icon is displayed.
> > 
> > 
> > What do you think?
> 
> I like the idea! I would suggest an alternative way to implement it, though:
> 
> - Calculate once the (conservative) mean number of icons in a minimized
> statusbar (to avoid getting tied to an specific number like 5)

sure

> - Keep a counter on every iconshown/iconhidden

yeah that's better than the querySelector

> - Compare the counter with the number of icons calculated in the first
> place. Add the maximized class or not, depending on that check.

We still need a way to remember that we skipped adding the maximized class so that we do it on iconshown if needed.

Comment 24

[Alberto Pastor [:albertopq]]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Ok, I think this is ready for a first round! \o/ One test is failing, but it seems to be failing in master for me as well. I'll investigate that in parallel. Thanks!

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

This is amazing! Can't wait to land this, I definitely want it for 2.5.
I made small comments on github, a tiny code change and some missing tests but it should be quick to address! :)

Also, can you run a quick Raptor test on a stable app like "Clock", I have a feeling the results could be interesting :)

PS: I'm flagging Yura for feedback because I don't want my enthusiasm for this patch to cause an accessibility regression.

Cheers!

Comment 26

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Couple of comments, that are not necessarily part of this bug but would be very nice to include them:
* Order of visible items is different from DOM order which is really not screen reader friendly
* Statusbar top container element absolutely must be above windows container because it is again in a wrong order for screen reader , than it is visually.

Otherwise, the changes do not seem to introduce any problems.

Comment 27

[Alberto Pastor [:albertopq]]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Comments addressed! Regarding the raptor results, that's what I get with 100 runs:

clock.gaiamobile.org   base: mean  1: mean  1: delta  1: p-value
---------------------  ----------  -------  --------  ----------
navigationLoaded             1967     1951       -16        0.16
navigationInteractive        2039     2025       -14        0.20
visuallyLoaded               2126     2113       -12        0.27
contentInteractive           2126     2114       -12        0.26
fullyLoaded                  2126     2114       -12        0.26
rss                        45.509   45.509     0.000        0.99
uss                        23.263   23.262    -0.000        0.95
pss                        28.205   28.205    -0.000        0.98

That shows a pretty stable gain of ~15ms. I'm pretty sure we can still improve it by deeply profiling the css, but is a start! The fact the code got that reduced, makes it so much easier to read and understand, and that's already a win.

Comment 28

[Alberto Pastor [:albertopq]]

Let's ask QA to do a round of tests on this branch before landing. I tested as much as I could, and all the conflictive cases with the statusbar are already covered (and passing) by the UI tests, but better to play safe here.

Comment 29

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #28)
> Let's ask QA to do a round of tests on this branch before landing. I tested
> as much as I could, and all the conflictive cases with the statusbar are
> already covered (and passing) by the UI tests, but better to play safe here.

Sure but I think we need to use "qa-approval" for this.

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

As always, feel free to forward :)

Comment 31

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #27)
> Comment on attachment 8653957 [details] [review]
> Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941
> 
> Comments addressed! Regarding the raptor results, that's what I get with 100
> runs:
> 
> clock.gaiamobile.org   base: mean  1: mean  1: delta  1: p-value
> ---------------------  ----------  -------  --------  ----------
> navigationLoaded             1967     1951       -16        0.16
> navigationInteractive        2039     2025       -14        0.20
> visuallyLoaded               2126     2113       -12        0.27
> contentInteractive           2126     2114       -12        0.26
> fullyLoaded                  2126     2114       -12        0.26
> rss                        45.509   45.509     0.000        0.99
> uss                        23.263   23.262    -0.000        0.95
> pss                        28.205   28.205    -0.000        0.98
> 
> That shows a pretty stable gain of ~15ms. I'm pretty sure we can still
> improve it by deeply profiling the css, but is a start! The fact the code
> got that reduced, makes it so much easier to read and understand, and that's
> already a win.

That's a start :)
And it should help more with heavier apps / lower end devices since the bulk of the work was done on appopened.

Comment 32

[Alberto Pastor [:albertopq]]

Plus all the time/cpu/battery we are saving by not calculating the iconsWidth on every iconshown/hidden and when expanding/collapsing the rocketbar :)

Comment 33

[Etienne Segonzac (:etienne)]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

Awesome! One last (very easy) comment on the tests but r=me.
Lets not forget to file the accessibility improvement follow ups while QA takes a look at this.

Can't wait to have this on my foxfooding device!

Comment 34

[Alberto Pastor [:albertopq]]

I already put the statusbar before the windows div, but regarding the second suggestion, I'm not sure what we can do. The icons are rendered dynamically and we use flex priority for sorting them out, so I can't think on a way to improve that situation. That said, the order wasn't following the DOM before the patch either.

Comment 35

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #34)
> I already put the statusbar before the windows div, but regarding the second
> suggestion, I'm not sure what we can do. The icons are rendered dynamically
> and we use flex priority for sorting them out, so I can't think on a way to
> improve that situation. That said, the order wasn't following the DOM before
> the patch either.

Yes that's why it's an improvement follow up :)
Not entirely sure what can be done either, but we can continue the discussion on the other bug.

Comment 36

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1) when taking a picture from the camera and hitting home within a split second, the audio of the camera will go off on the homescreen instead of taking the picture and then going to homescreen.  (this was on flame)

2) There was some wierd gradual slowness in performance when I enabled a bunch of things on the flame device.

3) I need to do some further testing; basically, I think there's some issues in making sure activities finish before switching to the home screen seems to be a general issue.

Comment 37

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

To clarify "bunch of things", enabling bluetooth, NFC, wifi, recording, geolocation, etc.  Loading the status bar with many items.

Comment 38

[Alberto Pastor [:albertopq]]

Mhm, I'll take a look, but I would be surprised if those errors come from the patch. Does that not happen with current master on the flame? I'll start profiling a Flame now. Thanks!

Comment 39

[Alberto Pastor [:albertopq]]

These are the results with a Flame and 8 icons for opening the clock app:


clock.gaiamobile.org   base: mean  1: mean  1: delta  1: p-value
---------------------  ----------  -------  --------  ----------
navigationLoaded              949      933       -16      * 0.00
navigationInteractive        1084     1069       -15      * 0.00
visuallyLoaded               1276     1268        -8        0.11
contentInteractive           1276     1268        -8        0.11
fullyLoaded                  1277     1269        -8        0.11
uss                        15.027   15.084     0.057      * 0.00
rss                        34.711   34.715     0.004        0.79
pss                        19.459   19.496     0.037      * 0.04

Again, around ~15ms win with the patch

Comment 40

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Performance is better with this patch, we need a few follow up bugs when this patch lands:
1) A more practical application of why the activities have to end before switching to the home screen is "pin the web"

ie go to a website -> pin the web site -> quickly hit home.
Instead of the website being pinned, the homescreen is pinned and will open to the homescreen

Without this patch, you can still do this, however the pin page will not open at all.  With this patch, the browser opens to a app://homescreen.gaiamobile.org/index... and a white screen.

Either cases, this needs a fix.

2) using 512 flame, placing the device in landscape mode, going to maps.google.com and then hitting home has a pretty long delay; this can be repeated with trulia.com and other web sites; Graphic glitches are easily seen by going to youtube -> full screening a video and hitting home.   This does happen without the patch. 

3) somehow I got the clock to switch to the wrong time/timezone:
Before it was set to the right time so that I can download apps from the marketplace.
I noticed that the time was at 1:17 in the upper corner so I checked the time once again.  Time zone was off.
nhirata-19333:gaia_master nhirata$ adb shell date
Fri Oct 23 18:17:36 UTC 2015
after going to clock app:
nhirata-19333:gaia_master nhirata$ adb shell date
Fri Oct 23 21:17:59 EDT 2015

This is happening every reboot. This might be https://bugzilla.mozilla.org/show_bug.cgi?id=1217277 ; I'm not sure.  I will have to follow up on this part, though without the patch, it's not acting up...

4) found out that FOTA can be downloaded via SIM.  (separate to this bug)  Follow up later.

Basically if 3 isn't with this patch, then we should be fine.  I should also follow up with 1, 2, 3, 4.

Comment 41

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

To note, I can't seem to reproduce the slowdown.  It may be because I was using 319 MB originally and switch to 512 MB.

Comment 42

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8653957 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30941

I'm approving this on two condition; 

1) Alberto, can you verify that you get the right time after rebooting the device?

From the patch, I don't see anything that would cause the timezone not to keep after a reboot.  Maybe I missed something.  Otherwise I will try to follow up after this patch lands and see if that becomes a reproducible issue.

2) that the follow up bugs that I mentioned also are taken a look at.

Comment 43

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Follow up bugs :
1) bug 1218045
2) bug 1214388 (performance is not mentioned, not sure if a new bug needs to be filed)

Other issues:
3) may need to file a timezone issue if reproduced and block bug 1217277
4) FOTA updating via SIM : bug 1218044

Comment 44

[Alberto Pastor [:albertopq]]

I took a look to:

1) bug 1218045 -> Can't repro (with or without the patch). Do I need to test in an specific device? Asked QA for repro steps.

2) bug 1214388 I don't think the statusbar has something to do with that bug. It probably can help a little bit, but not sure how is related.

3) Tried to reboot several times, and the Timezone is kept without problems.

4) bug 1218044 Doesn't seem related to this patch either. Is that only repro with this patch?

Comment 45

[Alberto Pastor [:albertopq]]

Merging, based on #c42. Time issue is not happening to me and I took a look to all the follow up bug. I'll keep an eye on them.


master: https://github.com/mozilla-b2g/gaia/commit/cae55c02dafde7786862af9497480d5c74afe439

Comment 47

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1) I was able to reproduce bug 1218045 on aries.  tap the homescreen after tapping pin with your other thumb.
2) fair enough; seems like something else and isn't related.
3+4) I agree. It's not something that's caused by this bug.  Just coincidence.  Possibly due to my gecko version.

The only other issue is what martijn and nojun are working on  (the python test associated; I guess I should have ran that too...)

Seems good otherwise.