Closed Bug 1110624 Opened 10 years ago Closed 6 years ago

[Meta] Let content process to start loading as early as possible (>40ms)

Categories

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

Product:
Firefox OS Graveyard
Component:
Performance
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: ting, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

package-preload.patch
5.83 KB, patch
Details | Diff | Splinter Review 
Currently B2G process knows what app to launch in Webapps.jsm doLaunch(), but SendLoadURL() is done in nsFrameLoader::ReallyStartLoadingInternal() when system app set iframe's src, and RecvLoadURL() can happen much later because of some sync IPCs before it. This is a waste.

I have traced nsJARChannel, the inflate is usually done off main thread, and parsing read data is done on the main thread, so we will be able to make at least 3 cores busy from the beginning, hopefully.
ting-yu: could you estimate how much time is wasted for waiting?
Based on bug 1094010 comment 8, SendLoadURL() is 44ms after Webapps.jsm doLaunch(), and RecvLoadURL() is 67ms after SendLoadURL().
Summary: Let content process to RecvLoadURL() from the beginning → Let content process to RecvLoadURL() from the beginning (>40ms)
feature-b2g: --- → 2.2?
The goal is to let content process to receive the URL and start loading as early as possible, e.g., when B2G process receives "Webapps:Launch".

Here're the ideas now:

- Gecko to new an iframe and set the URL, later pass it along with "webapps-launch" to system app.
- Let preallocated process to become the app earlier.

Some concerns:

- What if there's no preallocated process?
- What if the src system app sets is not the URL?
- Can desktop benefit from this?
Gecko doesn't know enough in some cases: the system app is the one that can decide whether to create a remote iframe or not, if that needs to be a private browsing frame, etc.

I'm curious to know why "Webapps.jsm doLaunch(), but SendLoadURL() is done in nsFrameLoader::ReallyStartLoadingInternal() when system app set iframe's src, which is 44ms later". That looks like a awful long time for things that all happen in the parent.
Fabrice's comment reminds me that maybe we can make current path faster. 

The profile at bug 1094010 comment 5 from a Nexus-4:

  http://people.mozilla.org/~bgirard/cleopatra/#report=4bd449e91e5592dd8181d26f5dd04b81d0702fa6&select=1592,1637

[1592,1601] doLaunch()
[1617,1637] nsFrameLoader::ReallyStartLoading

Since receiving "Webapps:Launch" is the first thing of doLaunch(), and SendLoadURL() is the last function call of ReallyStartLoadingInternal() when it is a remote frame, so I count it 44ms (1637-1592-1), and in this 44ms:

  %18 doLaunch()
  %81 webapps_observe() @ shell.js
    %72 awf_launch() @ app_window_factory.js   
      %9  aw_reConfig() @ app_window.js
      %50 aw__render() @ app_window.js
        %43 nsFrameLoader::ReallyStartLoading
          %22 BrowserElementParentFactory.observe()
          %6  nsFrameLoader::CreateRemoteBrowser()
          %6  errorPageObserve()

Note there're some missing samples, the numbers may be not that accurate.
feature-b2g: 2.2? → 2.2+
Priority: -- → P1
The profile at comment 5 is a bit outdated since BrowserElementParent.js has been changed, here's a recent profile:

https://people.mozilla.org/~bgirard/cleopatra/#report=72af6a93fad77360a40c34f7cba98d54a4a7b6c5&select=1642,1718
From the profile at comment 6:

  [1642, 1648] DOMApplicationRegistry.doLaunch() @ Webapps.jsm
  [1657, 1758] awf_launch() @ app_window_factory.js
    [1659, 1670] aw_reConfig() @ app_window.js
    [1670, 1723] aw__render() @ app_window.js
      [1680, 1719] nsFrameLoader::ReallyStartLoading

  [1719, 1726] PBrowser::RecvShow()
  [1725, 1736] PBrowser::RecvLoadURL()

I categorized the flow before RecvLoadURL() into three pieces:

    / 1. Webapps:Launch                 / 3. nsFrameLoader::ReallyStartLoading()
  |-+---------------|-+---------------|-+---------------|
                      \ 2. webapps-launch

  1. Webapps API handles Webapps:Launch, 5ms [1642, 1648] 
  2. B2G system app handles webapps-launch, 27ms [1653, 1681] 
  3. nsFrameLoader starts loading after B2G system app set iframe's src, 45ms [1680, 1726] 

I was hoping to let content process to receive the url when Webapps API finish permission checking at 1, but there're some concerns like comment 4. So probably receiving at 2 or early 3 is more reasonable goal.

Note from the profile, the fixes of bug 1003848, 1085655, 1107259 shorten the RecvShow() to 6ms, which RecvLoadURL() is called 7ms after the last sample of nsFrameLoader::ReallyStartLoading().
Assignee: nobody → tchou
Smaug, do you know whom should I talk to to have better ideas on how can I let remote browser LoadURL() earlier in nsFrameLoader::ReallyStartLoadingInternal()?
Flags: needinfo?(bugs)
What you mean with "earlier in nsFrameLoader::ReallyStartLoadingInternal()"? We do call LoadURL
pretty early in remote case.
What could be done, if needed, is to let PBrowser::show() to take url param so that we don't
need both show() and LoadURL().
But not sure that helps much.

Is it remote-browser-pending notification which takes time?
Could we perhaps have another notification which is fired after LoadURL() ?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #9)
> What you mean with "earlier in nsFrameLoader::ReallyStartLoadingInternal()"?
> We do call LoadURL
> pretty early in remote case.

The profile of comment 6 shows 3 big parts in ReallyStartLoadinInternal():

  remote-browser-pending [1680, 1693],
  TryRemoteBrowser()     [1693, 1704],
  remote-browser-shown   [1711, 1718]

LoadURL() is called after those. I wonder is it possible to move LoadURL() earlier in this sequence, for example, TryRemoteBrowser() and LoadURL(), then the notifications. Or is it possible to LoadURL() even earlier, maybe preload in parent before remote browser is created?
Status: NEW → ASSIGNED
See Also: → 786818
LoadURL() earlier probably can't fix the issue thoroughly, RecvCacheFileDescriptor() is required as well.
Some notes from JAR reading:

- Parent process reads application archive for the splash icon, but I am not sure is the opened file cached/reused for child process.
- Child process has two ways to get the file descriptor from parent: 1) TabChild::RecvCacheFileDescriptor(), and 2) RemoteOpenFileChild::Recv__delete__(). I am not sure why it needs both.
(In reply to Ting-Yu Chou [:ting] from comment #10)
> LoadURL() is called after those. I wonder is it possible to move LoadURL()
> earlier in this sequence, for example, TryRemoteBrowser() and LoadURL(),
> then the notifications. Or is it possible to LoadURL() even earlier, maybe
> preload in parent before remote browser is created?

Just moving LoadURL before those notifications would be a major change.
It is the users of the notifications you need to look at and see
whether they can deal with that case. That is why I suggested adding yet another notification so that some of the
work could be done after LoadURL.
And what would LoadURL before remote browser creation even mean?


Why does it take so much time to handle the notifications currently?
Should BrowserElementParent.js be rewritten in C++?
Or could we do some of the work there asynchronously so that LoadURL gets processed earlier?


Would it be possible to get a profile which which uses stackwalking and not only pseudostacks?
(In reply to Olli Pettay [:smaug] from comment #13)
> Just moving LoadURL before those notifications would be a major change.
> It is the users of the notifications you need to look at and see
> whether they can deal with that case. That is why I suggested adding yet
> another notification so that some of the
> work could be done after LoadURL.

remote-browser-pending
- nsBrowserElement::InitBrowserElementAPI() creates an instance of BrowserEelementParent and calls SetFrameLoader() which:
  - Set browser-element-api:call message listener.
  - Calls DOMApplicationRegistry.registerBrowserElementParentForApp().

remote-browser-shown
- Keyboard.jsm adds message listener for 'Forms:*' such as Forms:SelectionChange.
- ErrorPage.jsm adds mozbrowsererror event listener to the frame element

I am not sure what are safe to be done later after LoadURL, maybe only the keyboard things?

> And what would LoadURL before remote browser creation even mean?

I mean something like read or inflate the zip ahead (index.html for example) in parent. But probably this is too agressive for now.

> 
> Why does it take so much time to handle the notifications currently?
> Should BrowserElementParent.js be rewritten in C++?

Could be an option, but do you have any ideas how much time (roughly) could be safed from rewriting? Is 40%~60% a reasonable guess?

> Or could we do some of the work there asynchronously so that LoadURL gets
> processed earlier?
> 

Thanks for the valuable suggestions. Improving current path to gets LoadURL processed earlier is one solution, another is to create a new path so that content process can receive the URL earlier.

> 
> Would it be possible to get a profile which which uses stackwalking and not
> only pseudostacks?

http://people.mozilla.org/~bgirard/cleopatra/#report=d15dd2d641263ede87fa559e5b752c292a4ed65d

Command used: |$ ./profile.sh start -p b2g -f js,stackwalk && ./profile.sh start -p [preallocated pid] -f js,stackwalk; sleep 5; ./profile.sh capture|
(In reply to Ting-Yu Chou [:ting] from comment #14)
> > And what would LoadURL before remote browser creation even mean?
> 
> I mean something like read or inflate the zip ahead (index.html for example)
> in parent. But probably this is too agressive for now.

Oh, that would mean something very different than LoadURL which is a message to child process.
Might want to ask Necko devs if there is a way to initiate read/inflate of a zip ahead and then when
child process asks for the url, it is there ready already.
Sounds very much doable to me, but ask the necko devs.


> > Why does it take so much time to handle the notifications currently?
> > Should BrowserElementParent.js be rewritten in C++?
> 
> Could be an option, but do you have any ideas how much time (roughly) could
> be safed from rewriting? Is 40%~60% a reasonable guess?
Of the time spent in ReallyStartLoading, I'd guess 25-50%.
(But there is something odd in the profile. Why are we reporting some error.
I'm seeing nsXPCComponents_Utils::ReportError there.)

> 
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=d15dd2d641263ede87fa559e5b752c292a4ed65d
thanks.
I tried the "preload the zip as soon as possible" and that didn't help, but maybe I didn't do it right. Here's the patch I used.
(In reply to Fabrice Desré [:fabrice] from comment #16)
> I tried the "preload the zip as soon as possible" and that didn't help, but
> maybe I didn't do it right. Here's the patch I used.

Maybe it's because RecvShow() is now only a few (<10) ms before RecvLoadURL()?
(In reply to Ting-Yu Chou [:ting] from comment #17)
> (In reply to Fabrice Desré [:fabrice] from comment #16)
> > I tried the "preload the zip as soon as possible" and that didn't help, but
> > maybe I didn't do it right. Here's the patch I used.
> 
> Maybe it's because RecvShow() is now only a few (<10) ms before
> RecvLoadURL()?

Yes, maybe! to be honest I don't remember when if I tried this patch before or after the removal of the various synchronous calls.
(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Ting-Yu Chou [:ting] from comment #14)
> > I mean something like read or inflate the zip ahead (index.html for example)
> > in parent. But probably this is too agressive for now.
> 
> Oh, that would mean something very different than LoadURL which is a message
> to child process.
> Might want to ask Necko devs if there is a way to initiate read/inflate of a
> zip ahead and then when
> child process asks for the url, it is there ready already.
> Sounds very much doable to me, but ask the necko devs.

This now seems the best option to me since it can start inflating "launch_path" of the app when parent receives Webapps:Launch, which is the earliest point.

I haven't traced all the pieces of nsJARChannel and nsJARProtocolHandler, but parent does not share JAR cache to the child.
Inflate index.html (24135 bytes) from SMS's archive takes ~10ms, so probably doing this earlier in parent is not enough.
Not enough, but it would help too, no?
It definitely helps, just still need some other means to get better result.
I have another idea to eliminate the remote-browser-pending and remote-browser-shown things when lauch an app. What if we pre-create an iframe in system app and set the src "about:blank"? This should let all the initialization work been done in a different timing, so LoadURL() can go directly when app startup.

Seems bug 939695 is about this, but for different purpose.
Target Milestone: --- → 2.2 S6 (20feb)
(In reply to Ting-Yu Chou [:ting] from comment #12)
> - Parent process reads application archive for the splash icon, but I am not
> sure is the opened file cached/reused for child process.

It is not used. Neither OpenFileAndSendFDRunnable nor RemoteOpenFileParent::OpenSendCloseDelete() checks the JAR cache. Though currently JAR cache does not keep the fd by default, but it will once bug 1069081 is landed, then the fd can be a parameter in SendLoadURL for child process.
(In reply to Ting-Yu Chou [:ting] from comment #24)
> (In reply to Ting-Yu Chou [:ting] from comment #12)
> > - Parent process reads application archive for the splash icon, but I am not
> > sure is the opened file cached/reused for child process.
> 
> It is not used.

Filed bug 1119692.
(In reply to Ting-Yu Chou [:ting] from comment #23)
> I have another idea to eliminate the remote-browser-pending and
> remote-browser-shown things when lauch an app. What if we pre-create an
> iframe in system app and set the src "about:blank"? This should let all the
> initialization work been done in a different timing, so LoadURL() can go
> directly when app startup.

I tried following:

- Pre-create an iframe (BrowserFrame), but later when attach it to an AppWindow's browser-container div, it will be unbinded from old parent and destroyed.
- Pre-create an AppWindow, it will have permission issues later when launch the app, haven't figured out how is it happened.
(In reply to Ting-Yu Chou [:ting] from comment #26)
> - Pre-create an iframe (BrowserFrame), but later when attach it to an
> AppWindow's browser-container div, it will be unbinded from old parent and
> destroyed.
Yes, this is very much expected. The contents of iframe get unloaded when iframe is removed from the document.
(And moving elements in DOM is effectively 'remove - add')
(In reply to Ting-Yu Chou [:ting] from comment #26)
> (In reply to Ting-Yu Chou [:ting] from comment #23)
> > I have another idea to eliminate the remote-browser-pending and
> > remote-browser-shown things when lauch an app. What if we pre-create an
> > iframe in system app and set the src "about:blank"? This should let all the
> > initialization work been done in a different timing, so LoadURL() can go
> > directly when app startup.
> 
> I tried following:
> 
> - Pre-create an iframe (BrowserFrame), but later when attach it to an
> AppWindow's browser-container div, it will be unbinded from old parent and
> destroyed.
> - Pre-create an AppWindow, it will have permission issues later when launch
> the app, haven't figured out how is it happened.

I think Vivien had a patch doing that a while ago. Vivien, do you remember which bug that was in?
Flags: needinfo?(21)
(In reply to Fabrice Desré [:fabrice] from comment #28)
> I think Vivien had a patch doing that a while ago. Vivien, do you remember
> which bug that was in?

Should be bug 939695.
(In reply to Ting-Yu Chou [:ting] from comment #26)
> - Pre-create an AppWindow, it will have permission issues later when launch
> the app, haven't figured out how is it happened.

Since nsDocShell::GetAppId() returns the appId of system app set earlier by TabChild::NotifyTabContextUpdated() when TabChild::Init(), nsPermissionManager::CommonTestPermission() failed permission checking. To make this solution work, will need to update the tab context of pre-create remote iframe.
(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Ting-Yu Chou [:ting] from comment #14)
> > > And what would LoadURL before remote browser creation even mean?
> > 
> > I mean something like read or inflate the zip ahead (index.html for example)
> > in parent. But probably this is too agressive for now.
> 
> Oh, that would mean something very different than LoadURL which is a message
> to child process.
> Might want to ask Necko devs if there is a way to initiate read/inflate of a
> zip ahead and then when
> child process asks for the url, it is there ready already.
> Sounds very much doable to me, but ask the necko devs.

Created bug 1121310 for this.
(In reply to Ting-Yu Chou [:ting] from comment #10)
> earlier in this sequence, for example, TryRemoteBrowser() and LoadURL(),
> then the notifications.

Thinker reminds me this, the reorder seems work after disabling the mShown checking in TabParent::LoadURL() [1]. But as comment 11 stated, it still needs the fd to start loading, which depends on bug 1119692.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#564
(In reply to Ting-Yu Chou [:ting] from comment #7)
> From the profile at comment 6:
>     [1659, 1670] aw_reConfig() @ app_window.js

Filed bug 1121897 to move aw_getIconForSplash() out from the path.
(In reply to Ting-Yu Chou [:ting] from comment #32)
> (In reply to Ting-Yu Chou [:ting] from comment #10)
> > earlier in this sequence, for example, TryRemoteBrowser() and LoadURL(),
> > then the notifications.
> 
> Thinker reminds me this, the reorder seems work after disabling the mShown
> checking in TabParent::LoadURL() [1]. But as comment 11 stated, it still
> needs the fd to start loading, which depends on bug 1119692.

Bug 1121905.
blocking-b2g: --- → 2.2?
feature-b2g: 2.2+ → ---
blocking-b2g: 2.2? → 2.2+
Depends on: 1127189
Depends on: 1121905
Summary: Let content process to RecvLoadURL() from the beginning (>40ms) → Let content process to start loading as early as possible (>40ms)
Can we update the target milestone? Thanks.
Flags: needinfo?(janus926)
I don't see any reasons we can not. Bobby?
Flags: needinfo?(janus926) → needinfo?(bchien)
As discussion with Ting-yu, we would like to escalate this bug as meta bug. Tingyu will add others improvement tasks to improve content process loading time.
blocking-b2g: 2.2+ → ---
Flags: needinfo?(bchien)
Summary: Let content process to start loading as early as possible (>40ms) → [Meta] Let content process to start loading as early as possible (>40ms)
Target Milestone: 2.2 S6 (20feb) → ---
Depends on: 1137124
Depends on: 1130993
Depends on: 1121897
Depends on: 1119692
Depends on: 1126681
tracking-b2g: --- → +
Depends on: 1150349
Depends on: 1214133
Priority: P1 → P2
Unassigned myself as I am not working on this.
Assignee: janus926 → nobody
Status: ASSIGNED → NEW
tracking-b2g: + → ---
Flags: needinfo?(21)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: