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

NEW
Unassigned

Status

Firefox OS
Performance
P2
normal
3 years ago
2 years ago

People

(Reporter: ting, Unassigned)

Tracking

(Depends on: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.

Comment 1

3 years ago
ting-yu: could you estimate how much time is wasted for waiting?
(Reporter)

Comment 2

3 years ago
Based on bug 1094010 comment 8, SendLoadURL() is 44ms after Webapps.jsm doLaunch(), and RecvLoadURL() is 67ms after SendLoadURL().
(Reporter)

Updated

3 years ago
Summary: Let content process to RecvLoadURL() from the beginning → Let content process to RecvLoadURL() from the beginning (>40ms)

Updated

3 years ago
feature-b2g: --- → 2.2?
(Reporter)

Comment 3

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

Comment 5

3 years ago
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.

Updated

3 years ago
feature-b2g: 2.2? → 2.2+
Priority: -- → P1
(Reporter)

Comment 6

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

Comment 7

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

Updated

3 years ago
Assignee: nobody → tchou
(Reporter)

Comment 8

3 years ago
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)

Comment 9

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

Comment 10

3 years ago
(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?

Updated

3 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

3 years ago
See Also: → bug 786818
(Reporter)

Comment 11

3 years ago
LoadURL() earlier probably can't fix the issue thoroughly, RecvCacheFileDescriptor() is required as well.
(Reporter)

Comment 12

3 years ago
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?
(Reporter)

Comment 14

3 years ago
(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.
Created attachment 8545341 [details] [diff] [review]
package-preload.patch

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

Comment 17

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

Comment 19

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

Comment 20

3 years ago
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?
(Reporter)

Comment 22

3 years ago
It definitely helps, just still need some other means to get better result.
(Reporter)

Comment 23

3 years ago
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.

Updated

3 years ago
Target Milestone: --- → 2.2 S6 (20feb)
(Reporter)

Comment 24

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

Comment 25

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

Comment 26

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

Comment 29

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

Comment 30

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

Comment 31

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

Comment 32

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

Comment 33

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

Comment 34

3 years ago
(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.

Updated

3 years ago
blocking-b2g: --- → 2.2?
feature-b2g: 2.2+ → ---

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+
(Reporter)

Updated

3 years ago
Depends on: 1127189
(Reporter)

Updated

3 years ago
Depends on: 1121905
(Reporter)

Updated

3 years ago
Summary: Let content process to RecvLoadURL() from the beginning (>40ms) → Let content process to start loading as early as possible (>40ms)

Comment 35

3 years ago
Can we update the target milestone? Thanks.
Flags: needinfo?(janus926)
(Reporter)

Comment 36

3 years ago
I don't see any reasons we can not. Bobby?
Flags: needinfo?(janus926) → needinfo?(bchien)

Comment 37

3 years ago
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) → ---
(Reporter)

Updated

3 years ago
Depends on: 1137124
(Reporter)

Updated

3 years ago
Depends on: 1130993
(Reporter)

Updated

3 years ago
Depends on: 1121897
(Reporter)

Updated

3 years ago
Depends on: 1119692
(Reporter)

Updated

3 years ago
Depends on: 1126681

Updated

3 years ago
tracking-b2g: --- → +
(Reporter)

Updated

3 years ago
Depends on: 1150349
(Reporter)

Updated

2 years ago
Depends on: 1214133

Updated

2 years ago
Priority: P1 → P2
(Reporter)

Comment 38

2 years ago
Unassigned myself as I am not working on this.
Assignee: janus926 → nobody
Status: ASSIGNED → NEW

Updated

2 years ago
tracking-b2g: + → ---
Flags: needinfo?(21)
You need to log in before you can comment on or make changes to this bug.