Loading Wifi panel is ridiculously slow

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: janjongboom, Assigned: eragonj)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Created attachment 8495932 [details]
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.
Created attachment 8495933 [details] [review]
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.
Attachment #8495933 - Flags: review?(arthur.chen)
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.
Attachment #8495933 - Flags: review?(arthur.chen)
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.
Attachment #8495933 - Flags: review?(arthur.chen)
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?
Attachment #8495933 - Flags: review?(arthur.chen)
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.
Flags: needinfo?(jelee)
(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.
Do you mean "Do not track" is also slow before applying the patch?

Comment 8

4 years ago
(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!
Flags: needinfo?(jelee)
(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?
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).
Flags: needinfo?(jelee)
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
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

4 years ago
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!
Flags: needinfo?(jelee)
(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.
Created attachment 8510819 [details]
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?
Flags: needinfo?(janjongboom)
Yep, good first step. I still believe we want to start the animation faster, but it's a good start. Ship it!
Flags: needinfo?(janjongboom) → needinfo?(arthur.chen)
EJ will help investigate delay loading some of the modules in the wifi panel to enable faster animation.
Flags: needinfo?(arthur.chen)
I will check this bug and propose some possible solutions for it, just took it.
Assignee: janjongboom → ejchen
Comment on attachment 8517957 [details] [review]
WIP for master

Arthur, can you help me review this patch ? THanks :)
Attachment #8517957 - Flags: review?(arthur.chen)
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.
Attachment #8517957 - Flags: review?(arthur.chen)
Comment on attachment 8517957 [details] [review]
WIP for master

Thanks Arthur ! This looks better now :) please help me review again !
Attachment #8517957 - Flags: review?(arthur.chen)
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?
Attachment #8517957 - Flags: review?(arthur.chen)
Attachment #8517957 - Flags: review+
Attachment #8517957 - Flags: feedback?(janjongboom)
Let's land the patch and close the issue. We can reopen it when necessary.
Thanks all, this patch was merged into Gaia/master(2.2): https://github.com/mozilla-b2g/gaia/commit/0d2fa8ee217d4844f8f91f48a9e662d304da4bf1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
Attachment #8517957 - Flags: feedback?(janjongboom)
You need to log in before you can comment on or make changes to this bug.