1073520 - Loading Wifi panel is ridiculously slow

Status: RESOLVED FIXED

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

Description

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Video of the patch

On Flame, 256 MB there is over a second pause between clicking the Wifi tab and the animation starting to play.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Most important part of this patch is moving the subpanel rendering out of the _loadPanel. This gave the huge penalty because we basically block the event loop for a second, and nothing can happen. We now do this after the animation has been played.

Also: stop using lazy loader for panel rendering, it forces timeouts that I don't want to happen there, and we don't do any actual lazy loading as all data is already in the DOM. On top of that: play the animation as fast as possible (after the panel is rendered), gives an extra perceived speed.

You'll notice the effect on all panels, all load faster.

Comment 2

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8495933 [details] [review]
Patch

Thanks for the patch. Generally I'm fine with the direction as long as there is no major concern from the UX side. Details please check my comments in github.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8495933 [details] [review]
Patch

Thanks for your comments, they made sense. I updated the PR in GitHub and replied to your input.

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8495933 [details] [review]
Patch

I would still suggest to load all sub panels at once. By now we could remove the `data-require-sub-panels` attribute from wifi, keyboard, and appPermissions to make them be loaded faster.

As for the change to the transition, it makes the experience worse when navigating to simple panels such as the "Do not track" panel. I was thinking maybe we can define a threshold like 100ms, and if the module cannot be loaded withing the threshold, we start the transition. WDYT?

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

Jenny, we have a decision to make regarding the transitions. Under the current implementation the transition is started after all modules are loaded. In some cases where the modules take more time to load the responsiveness becomes bad. The proposed patch tries to start the transition as soon as the user clicks on the menu item. By doing this we can get the best responsiveness but the drawback is that users might observe significant UI changes. There are options that can ease the problems:

1. Hide the content of a panel during the transition until the content is ready.
2. Start the transition only when the modules cannot be loaded withing the threshold as suggested in comment 4.

If you think the current implementation is also acceptable we can also keep the current way.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Arthur Chen [:arthurcc] from comment #4)
> As for the change to the transition, it makes the experience worse when
> navigating to simple panels such as the "Do not track" panel. I was thinking
> maybe we can define a threshold like 100ms, and if the module cannot be
> loaded withing the threshold, we start the transition. WDYT?

Well:

1. If the panel is so simple, why does it take so long to render. I mean it already has some time. Otherwise we can also pre-render these simple panels anyway. Need to measure RAM consumption, but I doubt it'll be crazy.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Do you mean "Do not track" is also slow before applying the patch?

Comment 8

[Jenny Lee]

(In reply to Arthur Chen [:arthurcc] from comment #5)
> Jenny, we have a decision to make regarding the transitions. Under the
> current implementation the transition is started after all modules are
> loaded. In some cases where the modules take more time to load the
> responsiveness becomes bad. The proposed patch tries to start the transition
> as soon as the user clicks on the menu item. By doing this we can get the
> best responsiveness but the drawback is that users might observe significant
> UI changes. There are options that can ease the problems:
> 
> 1. Hide the content of a panel during the transition until the content is
> ready.
> 2. Start the transition only when the modules cannot be loaded withing the
> threshold as suggested in comment 4.
> 
> If you think the current implementation is also acceptable we can also keep
> the current way.

I think the idea behind the patch proposed here is an UX improvement, however, after actually trying it on Flame, the performance doesn't seem to surpass what we have now. In some panels, it almost feels like the content takes longer to load. I'd suggest we try Arthur's proposal and see how things work then. Thanks!

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Jenny Lee from comment #8)
> I think the idea behind the patch proposed here is an UX improvement,
> however, after actually trying it on Flame, the performance doesn't seem to
> surpass what we have now. In some panels, it almost feels like the content
> takes longer to load. I'd suggest we try Arthur's proposal and see how
> things work then. Thanks!

Did you try this on low-mem device configuration?

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Here are some numbers from master running on Flame with 256 MB configuration:

Panel             | Time until transition start | Time until fully shown
------------------------------------------------------------------
Wifi              | 994ms                       | 1454ms
SIM Manager       | 298ms                       | 803ms
Do Not Track      | 271ms                       | 674ms
Developer         | 303ms                       | 781ms

Now with this patch:

Panel             | Time until transition start | Time until fully shown (all content visible)
------------------------------------------------------------------
Wifi              | 112ms                       | 604ms
SIM Manager       | 217ms                       | 848ms
Do Not Track      | 209ms                       | 665ms
Developer         | 123ms                       | 485ms

So it's way faster on complex panels, and it has same performance on less complex panels (although the animation starts a bit faster).

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Firefox os 2.0 is barely to run on a flame device with low memory configurations[1]. IMHO it seems not practical to address this problem without a real shipping device. The numbers did get improved but it the patch actually sacrifices the user experience in other ways. The performance is much more acceptable if the device is with 273 MB or 319 MB configuration. 

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1027592#c70

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Dolphin device running master takes 1500 ms. before animation starts when clicking Wifi button.

It's not acceptable that we don't do anything for 1.5 seconds. Start the animation, play a loading animator or whatever, but this is not good.

Comment 13

[Jenny Lee]

Hi Jan,
The improvement is very impressive!
However, the main concern is when applying your method to every single panel in Settings, I don't find the overall experience better. I can have a further discussion with Arthur about the performance on low rem device, but for now, I would suggest to keep current behavior. Thanks!

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #12)
> Dolphin device running master takes 1500 ms. before animation starts when
> clicking Wifi button.

I will get a dolphin device in a few days and will check if we can improve this from other aspects.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: WIP

The main reason why entering wifi panel so slow is because it inserts eight sub panels at the same time. The patch simply removed the attribute of enabling loading sub panels from the panels that no longer need to be loaded together. It improves the performance without affecting the transition. Could you have a try?

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Yep, good first step. I still believe we want to start the animation faster, but it's a good start. Ship it!

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

EJ will help investigate delay loading some of the modules in the wifi panel to enable faster animation.

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

I will check this bug and propose some possible solutions for it, just took it.

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: WIP for master

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8517957 [details] [review]
WIP for master

Arthur, can you help me review this patch ? THanks :)

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8517957 [details] [review]
WIP for master

Thanks for the patch. What do you think if we move everything from `onInit` to `onShow`? Please check if there is any performance improvement by doing it.

Comment 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8517957 [details] [review]
WIP for master

Thanks Arthur ! This looks better now :) please help me review again !

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8517957 [details] [review]
WIP for master

r=me with the nits addressed, thanks.

Jan, would you mind test the patch seeing how it works on dolphin?

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Let's land the patch and close the issue. We can reopen it when necessary.

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all, this patch was merged into Gaia/master(2.2): https://github.com/mozilla-b2g/gaia/commit/0d2fa8ee217d4844f8f91f48a9e662d304da4bf1