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

RESOLVED FIXED in Firefox OS v2.2

Status

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sinker, Assigned: alive)

Tracking

({perf})

unspecified
2.2 S4 (23jan)
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Summary: Stop blocking the main thread of b2g process during app launching. → Stop blocking the main thread of b2g process during app launching for gallery case (~200ms).
Blocks: 1110590
No longer blocks: 1093564
(Reporter)

Updated

4 years ago
Blocks: 1086963
(Reporter)

Updated

4 years ago
Summary: Stop blocking the main thread of b2g process during app launching for gallery case (~200ms). → Stop refresh driver blocking the main thread of b2g process during app launching for gallery case (~200ms).
Assignee: nobody → kchen
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
(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].
(Reporter)

Updated

4 years ago
feature-b2g: --- → 2.2?
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.
Summary: Stop refresh driver blocking the main thread of b2g process during app launching for gallery case (~200ms). → Stop refresh driver blocking the main thread of b2g process during app launching for gallery case (~150ms).

Updated

4 years ago
Keywords: perf

Updated

4 years ago
feature-b2g: 2.2? → 2.2+
Priority: -- → P1

Updated

4 years ago
Status: NEW → ASSIGNED
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.
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].
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.
Alive, any idea?
Flags: needinfo?(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.
Flags: needinfo?(alive)
For the record, the remaining 8ms restyling are from edge_swipe_detector.js and statusbar.setAppearance
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)".

Updated

4 years ago
Target Milestone: --- → 2.2 S6 (20feb)
Created attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master
Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Could you guys check if this help?
Attachment #8546447 - Flags: feedback?(tchou)
Attachment #8546447 - Flags: feedback?(kchen)
(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 on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Per comment 14 and 15 it looks positive.
Attachment #8546447 - Flags: feedback?(tchou)
Stealing this bug and work on tests from now on.
Assignee: kchen → alive
The second restyle & reflow is caused by setFrameBackground. Most time is spent on decoding icon image and reflow identificationTitle text.
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.
Attachment #8546447 - Flags: review?(mhenretty)
Attachment #8546447 - Flags: review?(etienne)
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+ :)
Attachment #8546447 - Flags: review?(mhenretty)
Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Fixing the secureWindow case. Canceling request.
Attachment #8546447 - Flags: review?(etienne)
Attachment #8546447 - Flags: feedback?(kchen)
After thinking I'd rather to have bug 1117633's topmostwindowchanged event to handle the secureWindow case or re-implement it here.

Updated

4 years ago
blocking-b2g: --- → 2.2?
feature-b2g: 2.2+ → ---
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.
Attachment #8546447 - Flags: review?(mhenretty)
Attachment #8546447 - Flags: review?(etienne)

Updated

4 years ago
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8546447 [details] [review]
[PullReq] alivedise:bugzilla/1102715/wip to mozilla-b2g:master

Thanks!
Attachment #8546447 - Flags: review?(mhenretty) → review+
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.
Attachment #8546447 - Flags: review?(etienne)
https://github.com/mozilla-b2g/gaia/commit/f812440a66a16c7220d0deecc3fb0b7ec8e8d125
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Resolution: --- → FIXED
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
Attachment #8546447 - Flags: approval-gaia-v2.2?
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?
Flags: needinfo?(bchien)
Flags: needinfo?(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.
Flags: needinfo?(alive)

Comment 30

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

Updated

4 years ago
Attachment #8546447 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/b21807233a8ee8d9b7dd0a932a5d36982eeb1369
status-b2g-v2.2: affected → fixed
Target Milestone: 2.2 S6 (20feb) → 2.2 S4 (23jan)
You need to log in before you can comment on or make changes to this bug.