835013 - AppProtocolHandler.js and related code taking ~50ms on the critical startup path

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This code runs under PBrowser::RecvLoadURL, for example in this profile

http://people.mozilla.com/~bgirard/cleopatra/#report=8e2d5b4d6c221d60afc436f2fe399c329857e3c9

It's a little tricky for me to account for the js::RunScript samples, but there's a significant amount of time.  A lot of it is spent blocked on a sync IPC query to the b2g process, so b2g being busy might be part of the problem.

Comment 1

[[:fabrice] Fabrice Desré]

Yes, this is https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/app/AppProtocolHandler.js#39

I may have a solution to avoid that.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: patch

This patch removes the IPC sync call from AppProtocolHandler.js

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 706854 [details] [diff] [review]
patch

This removes about half the samples under RecvLoadURL() from the profile.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Which should be a 30-40ms win.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Which should be a 30-40ms win.

That's very coherent with timings I did of sync IPC with small payloads.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I think most of the problem here isn't the sync IPC per se, but the problem that this code runs while the b2g process is quite busy.  There are several trivial sync IPCs that are showing up in early startup as relatively expensive, but then other trivial sync IPCs show up vastly cheaper later in startup when the b2g process is less busy.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Comment 8

[[:fabrice] Fabrice Desré]

This patch takes advantage of the framework we already have in place to access various webapps properties from both the content and parent process through nsIAppsService. When on the content side, we cache all we need.

So instead of doing an explicit sync IPC call, we now just lookup the value in the cache.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 706854 [details] [diff] [review]
patch

(Not my front yard, but fabrice explained the patch and I understand what it's doing.)

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf98cf3585ab

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf98cf3585ab

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/fc2ad2c37df4
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/62f5f431675e