781724 - Pre-launch "app" processes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

XPCOM initialization is pretty hurty on device (400ms and change), and it's on the critical path of app startup currently.  We can and should look for easy wins there.

In parallel, we can get XPCOM init off the critical path by pre-launching and pre-initializing app processes.  We can't really do this with the "zergling" approach because we can't fork() after XPCOM init.

Initial results show that the upcoming patch shaves about ~300ms off user-perceived startup time.

Comment 1

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

Attachment: Try to keep a process pre-launched for app content, if the frontend asks

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 650767 [details] [diff] [review]
Try to keep a process pre-launched for app content, if the frontend asks

Could you clarify somewhere that by "app process" you also include mozbrowser inside an app?  I had all sorts of comments written about why we should be more general, and then I realized that we are in fact that general.  :)

In fact, I might not call it "app process", because that's kind of misleading.

What goes wrong if we preallocate a contentparent that's not associated with an app?  We should just be able to set its manifest to "" and be done, right?

Separately, as a follow-up, we should tweak the priority manager to give
preloaded processes a high oom_adj and niceness.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>@@ -148,19 +149,84 @@ MemoryReportRequestParent::~MemoryReport
> 
> nsDataHashtable<nsStringHashKey, ContentParent*>* ContentParent::gAppContentParents;
> nsTArray<ContentParent*>* ContentParent::gNonAppContentParents;
> nsTArray<ContentParent*>* ContentParent::gPrivateContent;
> 
> // The first content child has ID 1, so the chrome process can have ID 0.
> static PRUint64 gContentChildID = 1;
> 
>+static bool sPreallocatedAppProcessWillClearOnShutdown;
>+static StaticRefPtr<ContentParent> sPreallocatedAppProcess;
>+static CancelableTask* sPreallocateAppProcessTask;
>+// This number is fairly arbitrary ... the intention is to put off
>+// launching another app process until the last one has finished
>+// loading its content, to reduce CPU/memory/IO contention.
>+static const int kPreallocateDelayMs = 1000;

Could you please make this a pref?

>+/*static*/ already_AddRefed<ContentParent>
>+ContentParent::MaybeTakePreallocatedAppProcess()
>+{
>+    nsRefPtr<ContentParent> process = sPreallocatedAppProcess.get();

Sorry about that API cludge.  :(

> /*static*/ ContentParent*
>-ContentParent::GetNewOrUsed()
>+ContentParent::GetNewOrUsed(ProcessUsage aUsage)
> {
>+    if (aUsage == USE_FOR_APP) {
>+        if (!sPreallocatedAppProcess) {
>+            PreallocateAppProcess();
>+        }
>+        // Callers can't use this process; it's preallocated for
>+        // internal use here.
>+        return nullptr;

Calling this method GetNewOrUsed() when it doesn't get anything if you pass USE_FOR_APP is a footgun.  Can you split this off into an explicit PreallocateForApp() method, or something?

>@@ -289,16 +361,25 @@ ContentParent::Init()
>     // process.
>     if (nsIPresShell::IsAccessibilityActive()) {
>         unused << SendActivateA11y();
>     }
> #endif
> }
> 
> void
>+ContentParent::SetManifestFromPreallocated(const nsAString& aAppManifestURL)
>+{
>+    MOZ_ASSERT(mAppManifestURL == MAGIC_PREALLOCATED_APP_MANIFEST_URL);
>+    // Clients should think of mAppManifestURL as const ... we're
>+    // bending the rules here just for the preallocation hack.
>+    const_cast<nsString&>(mAppManifestURL) = aAppManifestURL;
>+}

If this is all we have to do to transform ourselves into an app process, I don't see why we need the distinction between preloading an app process and preloading a non-app process.

>@@ -107,26 +108,35 @@ protected:
>     void OnChannelConnected(int32 pid);
>     virtual void ActorDestroy(ActorDestroyReason why);
> 
> private:
>     static nsDataHashtable<nsStringHashKey, ContentParent*> *gAppContentParents;
>     static nsTArray<ContentParent*>* gNonAppContentParents;
>     static nsTArray<ContentParent*>* gPrivateContent;
> 
>+    static void PreallocateAppProcess();
>+    static void DelayedPreallocateAppProcess();
>+    static already_AddRefed<ContentParent>
>+    MaybeTakePreallocatedAppProcess();

Nit: Newline before the already_AddRefed line, please.

>diff --git a/xpcom/system/nsIXULRuntime.idl b/xpcom/system/nsIXULRuntime.idl
>@@ -72,23 +72,29 @@ interface nsIXULRuntime : nsISupports
>    * This will cause components to be autoregistered and all
>    * fastload data to be re-created.
>    */
>   void invalidateCachesOnRestart();
> 
>   /**
>    * Starts a child process. This method is intented to pre-start a
>    * content child process so that when it is actually needed, it is
>-   * ready to go.
>+   * ready to go.  The process is allocated for "browser tab" content.
>    *
>    * @throw NS_ERROR_NOT_AVAILABLE if not available.
>    */
>   void ensureContentProcess();
> 
>   /**
>+   * Like ensureContentProcess(), but pre-allocates a process for
>+   * "app" content.
>+   */
>+  void ensureAppProcess();

In particular, these comments are confusing because a process created by
ensureAppProcess() can in fact be used for browser tab content, afaict.

I'm not sure if this should be r+ or r-, because I might be wrong about this "app process" business.  But if I'm wrong, I want to re-read the patch, so I guess that's r-.  :)

Comment 3

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

(In reply to Justin Lebar [:jlebar] from comment #2)
> Comment on attachment 650767 [details] [diff] [review]
> Try to keep a process pre-launched for app content, if the frontend asks
> 
> Could you clarify somewhere that by "app process" you also include
> mozbrowser inside an app?  I had all sorts of comments written about why we
> should be more general, and then I realized that we are in fact that
> general.  :)
> 

It doesn't, though.  Does that change the rest of your review comments?

Comment 4

[Justin Lebar (not reading bugmail)]

> It doesn't, though.

Hm, I see now in the nsFrameLoader code that calls CreateBrowser, you're right.

But I wonder if that's correct.  That does not seem correct to me.

Comment 5

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

This code needs some love.

Comment 6

[Justin Lebar (not reading bugmail)]

Looking at this again with comment 4 in mind, it's mostly fine.

"CreateBrowser" is a pretty confusing name now, because there's a low-level
distinction between "browser-tab" and "non-browser-tab" processes, but they
both are created via "CreateBrowser".  You could have two sepratate methods.

At the very least, would you mind adding an assertion to CreateBrowser that
(!aApp || !aIsBrowserElement), so the definition of those parameters in this
context is enforced?  (We really need to clean this up in general, once we
figure out what's the right way to redo it.)

>+// This number is fairly arbitrary ... the intention is to put off
>+// launching another app process until the last one has finished
>+// loading its content, to reduce CPU/memory/IO contention.
>+static const int kPreallocateDelayMs = 1000;

I'd still prefer if this were a pref.  (For example, we might want different
values on different hw devices, and that's much easier to configure via a
pref.)

> /*static*/ ContentParent*
>-ContentParent::GetNewOrUsed()
>+ContentParent::GetNewOrUsed(ProcessUsage aUsage)
> {
>+    if (aUsage == USE_FOR_APP) {
>+        if (!sPreallocatedAppProcess) {
>+            PreallocateAppProcess();
>+        }
>+        // Callers can't use this process; it's preallocated for
>+        // internal use here.
>+        return nullptr;
>+    }

I don't think we should have a public GetNewOrUsed method which, for
USE_DEFAULT, returns something, and for USE_FOR_APP, always returns null.

It's a little silly, because as written, we will always pre-allocate app
processes, except for the first one, which we might or might not pre-allocate.

So maybe the right thing to do is to schedule your first pre-allocation N
seconds after startup (if a pref is set), and not modify this method at all.

Alternatively, you could expose PreallocateAppProcess() as a public method, but
that's kind of confusing if you only need to call it for the first
pre-allocation.

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>+    static void PreallocateAppProcess();
>+    static void DelayedPreallocateAppProcess();
>+    static already_AddRefed<ContentParent>
>+    MaybeTakePreallocatedAppProcess();

Nit: Newline before already_AddRefed, please.

I'd like to take this through another round, for the API's sake.  If you need to land now and follow up (or let someone else follow up) in a separate bug, that's fine with me.

Comment 7

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

Ended up putting together a megapatch for the upcoming demo, so there's no particular urgency for landing this upstream.  I'd be happy to do the app/browser-element cleanup before landing this, but we need a plan first :).

(In reply to Justin Lebar [:jlebar] from comment #6)
> >+// This number is fairly arbitrary ... the intention is to put off
> >+// launching another app process until the last one has finished
> >+// loading its content, to reduce CPU/memory/IO contention.
> >+static const int kPreallocateDelayMs = 1000;
> 
> I'd still prefer if this were a pref.  (For example, we might want different
> values on different hw devices, and that's much easier to configure via a
> pref.)
> 

What I really want here is some kind of idle timer, but I suspect it would be very hard to get that "right".

> > /*static*/ ContentParent*
> >-ContentParent::GetNewOrUsed()
> >+ContentParent::GetNewOrUsed(ProcessUsage aUsage)
> > {
> >+    if (aUsage == USE_FOR_APP) {
> >+        if (!sPreallocatedAppProcess) {
> >+            PreallocateAppProcess();
> >+        }
> >+        // Callers can't use this process; it's preallocated for
> >+        // internal use here.
> >+        return nullptr;
> >+    }
> 
> I don't think we should have a public GetNewOrUsed method which, for
> USE_DEFAULT, returns something, and for USE_FOR_APP, always returns null.
> 

Yeah, on second thought it's probably better to control this with a pref too.

Comment 8

[Justin Lebar (not reading bugmail)]

> I'd be happy to do the app/browser-element cleanup before landing this, but we need a plan first :).

I don't think we should block this on cleanup work.  The cleanup will have to touch so many other places that the marginal cost of adding this one extra place is low.

> What I really want here is some kind of idle timer, but I suspect it would be very hard to get that 
> "right".

What if you pre-launched the process after a short delay, but ran it with very low CPU priority?  Might that accomplish the same thing?

Comment 9

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

(In reply to Justin Lebar [:jlebar] from comment #6)
> Looking at this again with comment 4 in mind, it's mostly fine.
> 
> At the very least, would you mind adding an assertion to CreateBrowser that
> (!aApp || !aIsBrowserElement), so the definition of those parameters in this
> context is enforced?  (We really need to clean this up in general, once we
> figure out what's the right way to redo it.)
> 

I added this for the hinting bug.

> >+// This number is fairly arbitrary ... the intention is to put off
> >+// launching another app process until the last one has finished
> >+// loading its content, to reduce CPU/memory/IO contention.
> >+static const int kPreallocateDelayMs = 1000;
> 
> I'd still prefer if this were a pref.  (For example, we might want different
> values on different hw devices, and that's much easier to configure via a
> pref.)
> 

Done.

> > /*static*/ ContentParent*
> >-ContentParent::GetNewOrUsed()
> >+ContentParent::GetNewOrUsed(ProcessUsage aUsage)
> > {
> >+    if (aUsage == USE_FOR_APP) {
> >+        if (!sPreallocatedAppProcess) {
> >+            PreallocateAppProcess();
> >+        }
> >+        // Callers can't use this process; it's preallocated for
> >+        // internal use here.
> >+        return nullptr;
> >+    }
> 
> I don't think we should have a public GetNewOrUsed method which, for
> USE_DEFAULT, returns something, and for USE_FOR_APP, always returns null.
> 

Removed.

> Alternatively, you could expose PreallocateAppProcess() as a public method,
> but
> that's kind of confusing if you only need to call it for the first
> pre-allocation.
> 

ContentParent now participates in nsLayoutStatics startup, and decides whether to prelaunch when called from there depending on a pref.

> >diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
> >+    static void PreallocateAppProcess();
> >+    static void DelayedPreallocateAppProcess();
> >+    static already_AddRefed<ContentParent>
> >+    MaybeTakePreallocatedAppProcess();
> 
> Nit: Newline before already_AddRefed, please.
> 

Done.

Comment 10

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

Attachment: Try to keep a process pre-launched for app content, if the pref says to, v2

Comment 11

[Justin Lebar (not reading bugmail)]

>+/*static*/ void
>+ContentParent::PreallocateAppProcess()
>+{
>+    MOZ_ASSERT(!sPreallocatedAppProcess);
>+    if (sPreallocateAppProcessTask) {
>+        // We were called directly while a delayed task was scheduled.
>+        sPreallocateAppProcessTask->Cancel();
>+        sPreallocateAppProcessTask = nullptr;
>+    }
>+
>+    if (!sPreallocatedAppProcessWillClearOnShutdown) {
>+        sPreallocatedAppProcessWillClearOnShutdown = true;
>+        ClearOnShutdown(&sPreallocatedAppProcess);
>+    }

You could do this on StartUp now.

>+/*static*/ void
>+ContentParent::StartUp()
>+{
>+    if (XRE_GetProcessType() != GeckoProcessType_Default) {
>+        return;
>+    }
>+
>+    if (Preferences::GetBool("dom.ipc.processPrelauch.enabled", false)) {
>+        sPreallocateDelayMs = Preferences::GetInt(
>+            "dom.ipc.processPrelauch.delayMs", 1000);
>+        if (sPreallocateDelayMs < 0) {
>+            sPreallocateDelayMs = 1000;
>+        }

Any reason you're not using GetUint?

>@@ -225,20 +316,26 @@ ContentParent::CreateBrowser(mozIApplica
>     // Send the local app ID to the new TabChild so it knows what app
>     // it is.
>     PRUint32 appId;
>     if (NS_FAILED(appsService->GetAppLocalIdByManifestURL(manifestURL, &appId))) {
>         NS_ERROR("Failed to get local app ID");
>         return nullptr;
>     }
> 
>-    ContentParent* p = gAppContentParents->Get(manifestURL);
>+    nsRefPtr<ContentParent> p = gAppContentParents->Get(manifestURL);
>     if (!p) {
>-        p = new ContentParent(manifestURL, aIsBrowserElement);
>-        p->Init();
>+        p = MaybeTakePreallocatedAppProcess();
>+        if (p) {
>+            p->SetManifestFromPreallocated(manifestURL);
>+        } else {
>+            NS_WARNING("Unable to use pre-allocated app process");
>+            p = new ContentParent(manifestURL, aIsBrowserElement);
>+            p->Init();
>+        }

As written, dom.ipc.processPrelaunch.enabled controls only whether we
pre-launch the /first/ process.  All other processes will be pre-launched,
right?

I don't want to waste too much time writing code nobody is going to use, but so
long as we have a pref to disable pre-allocation, perhaps
MaybeTakePreallocatedAppProcess should return null if prelaunch is not enabled,
and then we can fall back to the |else| above.  I don't care if you fix the
warning, myself.

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>+    /** Shut down the content-process machinery. */
>+    static void ShutDown();

It would be nice if ContentParent *p; p->ShutDown(); was an error, but I'm not
sure we have machinery to do that.  :(

>@@ -107,39 +115,50 @@ protected:
>     void OnChannelConnected(int32 pid);
>     virtual void ActorDestroy(ActorDestroyReason why);
> 
> private:
>     static nsDataHashtable<nsStringHashKey, ContentParent*> *gAppContentParents;
>     static nsTArray<ContentParent*>* gNonAppContentParents;
>     static nsTArray<ContentParent*>* gPrivateContent;
> 
>+    static void PreallocateAppProcess();
>+    static void DelayedPreallocateAppProcess();
>+    static void ScheduleDelayedPreallocateAppProcess();
>+    static already_AddRefed<ContentParent> MaybeTakePreallocatedAppProcess();

Sorry!  It was difficult to separate out the two-line declaration from
the two one-line declarations above, so I was hoping for

  static void Foo();
  static void Bar();

  static alread_AddRefed<T>
  FooBar();

Sorry again for not being clear about this.  (FWIW, the whole signature seems to fit within 80 chars, so maybe we don't need to wrap at all!  :)

Comment 12

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

(In reply to Justin Lebar [:jlebar] from comment #11)
> >+/*static*/ void
> >+ContentParent::PreallocateAppProcess()
> >+{
> >+    MOZ_ASSERT(!sPreallocatedAppProcess);
> >+    if (sPreallocateAppProcessTask) {
> >+        // We were called directly while a delayed task was scheduled.
> >+        sPreallocateAppProcessTask->Cancel();
> >+        sPreallocateAppProcessTask = nullptr;
> >+    }
> >+
> >+    if (!sPreallocatedAppProcessWillClearOnShutdown) {
> >+        sPreallocatedAppProcessWillClearOnShutdown = true;
> >+        ClearOnShutdown(&sPreallocatedAppProcess);
> >+    }
> 
> You could do this on StartUp now.
> 

Good call, much cleaner.

> >+/*static*/ void
> >+ContentParent::StartUp()
> >+{
> >+    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> >+        return;
> >+    }
> >+
> >+    if (Preferences::GetBool("dom.ipc.processPrelauch.enabled", false)) {
> >+        sPreallocateDelayMs = Preferences::GetInt(
> >+            "dom.ipc.processPrelauch.delayMs", 1000);
> >+        if (sPreallocateDelayMs < 0) {
> >+            sPreallocateDelayMs = 1000;
> >+        }
> 
> Any reason you're not using GetUint?
> 

Negative numbers would be a bug, and GetUint() would cause this code to freak out.  But it doesn't matter too much; changed.

> >@@ -225,20 +316,26 @@ ContentParent::CreateBrowser(mozIApplica
> >     // Send the local app ID to the new TabChild so it knows what app
> >     // it is.
> >     PRUint32 appId;
> >     if (NS_FAILED(appsService->GetAppLocalIdByManifestURL(manifestURL, &appId))) {
> >         NS_ERROR("Failed to get local app ID");
> >         return nullptr;
> >     }
> > 
> >-    ContentParent* p = gAppContentParents->Get(manifestURL);
> >+    nsRefPtr<ContentParent> p = gAppContentParents->Get(manifestURL);
> >     if (!p) {
> >-        p = new ContentParent(manifestURL, aIsBrowserElement);
> >-        p->Init();
> >+        p = MaybeTakePreallocatedAppProcess();
> >+        if (p) {
> >+            p->SetManifestFromPreallocated(manifestURL);
> >+        } else {
> >+            NS_WARNING("Unable to use pre-allocated app process");
> >+            p = new ContentParent(manifestURL, aIsBrowserElement);
> >+            p->Init();
> >+        }
> 
> As written, dom.ipc.processPrelaunch.enabled controls only whether we
> pre-launch the /first/ process.  All other processes will be pre-launched,
> right?
> 

Yeah, that's just a bug.  I fixed it by saving the "enabled" pref and never scheduling a prelaunch task if false.

> I don't want to waste too much time writing code nobody is going to use, but
> so
> long as we have a pref to disable pre-allocation, perhaps
> MaybeTakePreallocatedAppProcess should return null if prelaunch is not
> enabled,
> and then we can fall back to the |else| above.  I don't care if you fix the
> warning, myself.
> 

I left the warning in, since there's no reason anyone would want to flip the pref except for debugging.

> >diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
> >+    /** Shut down the content-process machinery. */
> >+    static void ShutDown();
> 
> It would be nice if ContentParent *p; p->ShutDown(); was an error, but I'm
> not
> sure we have machinery to do that.  :(
> 

I don't believe we do.

> >@@ -107,39 +115,50 @@ protected:
> >     void OnChannelConnected(int32 pid);
> >     virtual void ActorDestroy(ActorDestroyReason why);
> > 
> > private:
> >     static nsDataHashtable<nsStringHashKey, ContentParent*> *gAppContentParents;
> >     static nsTArray<ContentParent*>* gNonAppContentParents;
> >     static nsTArray<ContentParent*>* gPrivateContent;
> > 
> >+    static void PreallocateAppProcess();
> >+    static void DelayedPreallocateAppProcess();
> >+    static void ScheduleDelayedPreallocateAppProcess();
> >+    static already_AddRefed<ContentParent> MaybeTakePreallocatedAppProcess();
> 
> Sorry!  It was difficult to separate out the two-line declaration from
> the two one-line declarations above, so I was hoping for
> 
>   static void Foo();
>   static void Bar();
> 
>   static alread_AddRefed<T>
>   FooBar();
> 
> Sorry again for not being clear about this.  (FWIW, the whole signature
> seems to fit within 80 chars, so maybe we don't need to wrap at all!  :)

So it does.  Unwrapped.

Comment 13

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

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

Comment 14

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

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