1102715 - Stop refresh driver blocking the main thread of b2g process during app launching for gallery case (~150ms).

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect, P1)

Description

[Thinker Li [:sinker]]

According https://bugzilla.mozilla.org/show_bug.cgi?id=1094010#c10 (gallery case), for some reason, the refresh driver of b2g process is kicked when an app is launching.  It blocks content process for 200+ms. (see gallery case in the comment).

We need to find a way to avoid it.  Maybe, we should pause refresh driver of b2g process for awhile.  Maybe, there are better ideas.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

New Gallery startup profile on Nexus4 based on rev:

Build ID               20141217185235
Gaia Revision          ad5580b8e6529e5eebf8517026286d0a8350d5fd
Gaia Date              2014-12-15 01:49:16
Gecko Revision         8bacfeb968aeda8579419ce4a73f4cc6c2892082
Gecko Version          37.0a1
Device Name            mako
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.kanru.20141107.093658
Firmware Date          Fri Nov  7 09:37:09 CST 2014
Bootloader             MAKOZ20i

https://people.mozilla.org/~bgirard/cleopatra/#report=92b14dc2d11485c60302821ef2b5383e03a03635

I don't see the blocking refresh driver before documentfirstpaint. However I found that we spent about 158ms on evaluating JavaScript. 96ms of that is spent on calculating Gaia header.

We also spent about 140ms waiting sync IPC PContent::SendGetVolumes.

The total time from loadstart to documentfirstpaint is 573ms

Comment 2

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

(In reply to Kan-Ru Chen [:kanru] from comment #1)
> https://people.mozilla.org/~bgirard/cleopatra/
> #report=92b14dc2d11485c60302821ef2b5383e03a03635

There're two nsRefreshDriver::Tick(): [2480,2570], [2600,2670].

Comment 3

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

The profile at https://bugzilla.mozilla.org/show_bug.cgi?id=1094010#c10 was captured from a image built with MOZ_USE_SYSTRACE=1, and I just realized somehow it causes the main thread IO Kanru noticed (even systrace.py is not running). Without the option, the profile looks similar to the one in comment 1, which the first two nsRefreshDriver::Tick() take ~80ms and ~70ms.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

I suspect the first refresh driver tick + reflow is caused by inserting the new AppWindow to the DOM tree. We could probably avoid this costly style calculation at app launch by preallocate a empty AppWindow in the DOM tree and use that at app launch time.

Comment 5

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

Note Gallery takes longer than the other apps, for instance SMS:

  https://people.mozilla.org/~bgirard/cleopatra/#report=72af6a93fad77360a40c34f7cba98d54a4a7b6c5

The first two nsRefreshDriver::Tick() take 7ms [1764,1772] and 28ms [1827,1856], though there's a 27ms nsPresShell::Flush() [1800,1828].

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Tried various way to remove the first style flush. Didn't work.

I tried

 - Pre-render DOM elements
 - Remove all app opening animations
 - Remove gaia-progress web component
 - Remove statusbar style change

Maybe it's caused by large style changes being applied at app opening.

I think this bug is largely related to bug 1110625.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

I found the first big restyling is coming from https://github.com/mozilla-b2g/gaia/blob/8a7377c1a75c25b90313f4d4606c7c25bab3a03a/apps/system/js/app_window_manager.js#L830

Maybe #screen has too many and complex CSS rules? Remove that line reduced the restyling time from 120ms to 8ms

Before:
https://people.mozilla.org/~bgirard/cleopatra/#report=61b79f8907899ff05b5aab76fa6f17886e8756ac
After:
https://people.mozilla.org/~bgirard/cleopatra/#report=f46d9925b122f2449176b91aee364f1c5a114445

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Alive, any idea?

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

We had a tracking item to clean the #screen element styling overwhelm in bug 1110659,
but according to this, it seems we need to clean fullscreen-app at first to unblock the main thread.

Investigating if we could just remove the fullscreen-app.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

For the record, the remaining 8ms restyling are from edge_swipe_detector.js and statusbar.setAppearance

Comment 11

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

FWIW, the hint eRestyle_Subtree is set here https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#2902 for selectors like: "#screen:not(.locked)", ".fullscreen-app:not(.minimized-tray)".

Comment 12

[GitHub Autolander Bot]

Attachment: [PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Could you guys check if this help?

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> Comment on attachment 8546447 [details] [review]
> [PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master
> 
> Could you guys check if this help?

It does! However I find you didn't change the 'fullscreen-layout-app' case. Maybe it's enough for this bug for now.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

https://people.mozilla.org/~bgirard/cleopatra/#report=c162e1fe1d11a29ae715704c134933a8cdbb5106

New profile after patch from comment 13

Comment 16

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

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Per comment 14 and 15 it looks positive.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

Stealing this bug and work on tests from now on.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

The second restyle & reflow is caused by setFrameBackground. Most time is spent on decoding icon image and reflow identificationTitle text.

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Deprecate #screen.fullscreen-app rules to unblock main thread from rules matching.
* fullscreen-app-layout is not touched in this patch
* permission dialog is permanent fullscreen in master, so I am leaving it as is so I just removed the rules.

A bad part is system dialog now needs to know the appWindow states but I think it is adoptable because doing #screen.fullscreen-app #dialog-overlay in styles is also a hidden coupling.

Comment 20

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

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Overall this is a good patch. We should probably do something about fullscreen layout too, either support it or remove it completely since it was a Tako requirement and is no longer a priority.

I did notice one problem with the lockscreen camera when passcode is enabled. Fix that, and you get my r+ :)

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Fixing the secureWindow case. Canceling request.

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

After thinking I'd rather to have bug 1117633's topmostwindowchanged event to handle the secureWindow case or re-implement it here.

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

End up keeping the .secure-app.fullscreen-app because this is a potential v2.2 blocker so let's not change too much but just fix the normal app case.

Will follow up after topmostwindowchanged is landed.

Comment 24

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

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Thanks!

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Try is green, I am landing this soon; Etienne if you have concern I could followup.

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/commit/f812440a66a16c7220d0deecc3fb0b7ec8e8d125

Comment 27

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not regression; should be there from long time ago.
[User impact] if declined: Performance issue when opening fullscreen app
[Testing completed]: y
[Risk to taking this patch] (and alternatives if risky): N
[String changes made]: NaN

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alive, It would be better if you could give out more detail on the risk assessment.

I also don't understand why this should block, since we shop with this performance issue since at least v1.0? Bobby?

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #27)
> [Risk to taking this patch] (and alternatives if risky): N

What this patch does is simply moving the screen.fullscreen-app rules into each UI modules who needs it to coordinate its layout: statusbar/system dialog(but not permission dialog). Also remove some unused dead rules. I don't see there will be any risky part for doing so.

Comment 30

[Bobby Chien]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #28)
> Alive, It would be better if you could give out more detail on the risk
> assessment.
> 
> I also don't understand why this should block, since we shop with this
> performance issue since at least v1.0? Bobby?

Tim, as discussion with Kevin, we will mark feasible performance enhancement as blocking-b2g. This bug was originally marked as feature-b2g to v2.2. So we put it as blocking-b2g.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/b21807233a8ee8d9b7dd0a932a5d36982eeb1369