944665 - Call preload slow things after preallocated process is forked from Nuwa

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

PreloadSlowThings is called before Nuwa freeze now. This function produces many asynchronous messages from parent to frozen Nuwa. For a short term solution, we can move the code of invoking PreloadSlowThings to the new process forked from Nuwa.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8340316 [details] [diff] [review]
Proposed Patch

>From 3318c7156999b60f35ec2c6acfedfd10034f279f Mon Sep 17 00:00:00 2001
>From: Patrick Wang <[e-mail]>
>Date: Fri, 29 Nov 2013 17:28:54 +0800
>Subject: [PATCH 5/5] Move common initialization routine into a function and
> invoke PreloadSlowThings after nuwa forked
>
>---
> dom/ipc/ContentChild.cpp  |   27 ++++---
> dom/ipc/ContentParent.cpp |  188 ++++++++++++++++++++++++---------------------
> dom/ipc/ContentParent.h   |    5 ++
> 3 files changed, 122 insertions(+), 98 deletions(-)
>
>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>index fd558e1..b55eb0f 100644
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp
>@@ -1362,17 +1351,31 @@ ContentChild::RecvAppInfo(const nsCString& version, const nsCString& buildID,
>     mAppInfo.UAName.Assign(UAName);
>     // If we're part of the mozbrowser machinery, go ahead and start
>     // preloading things.  We can only do this for mozbrowser because
>     // PreloadSlowThings() may set the docshell of the first TabChild
>     // inactive, and we can only safely restore it to active from
>     // BrowserElementChild.js.
>     if ((mIsForApp || mIsForBrowser) &&
>         Preferences::GetBool("dom.ipc.processPrelaunch.enabled", false)) {
>-        PreloadSlowThings();
>+#ifdef MOZ_NUWA_PROCESS
>+        if (IsNuwaProcess()) {
>+            // Perform GC before freezing the Nuwa process to reduce memory usage.
>+            ContentChild::GetSingleton()->RecvGarbageCollect();
>+
>+            MessageLoop::current()->
>+                PostTask(FROM_HERE,
>+                         NewRunnableFunction(OnFinishNuwaPreparation));
>+        } else {
>+            // This is a forked process, do preloading here.
>+            PreloadSlowThings();
>+        }
>+#else // !MOZ_NUWA_PROCESS
>+    PreloadSlowThings();
>+#endif
>     }
>     return true;
> }
> 

I will prefer

    if ((mIsForApp || mIsForBrowser) &&
        Preferences::GetBool("dom.ipc.processPrelaunch.enabled", false)
#ifdef MOZ_NUWA_PROCESS
      && !IsNuwaProcess()
#endif
        ) {
        PreloadSlowThings();
    }

And also, posting a delayed task should be moved out of the if block.

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

As discussion on bug 941466, this would be a short term solution to make Nuwa enabled on try server.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Move common logic of ContentParent constructors into a single function

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: don't call PreloadSlowThings before nuwa fork.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: don't call PreloadSlowThings before nuwa fork.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Have we measured how much this increases memory consumption (USS specifically) in the forked processes?  Do we have a bug on file on undoing this and fixing it correctly?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I guess those measurements are in bug 941466.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8341603 [details] [diff] [review]
Part 2: don't call PreloadSlowThings before nuwa fork.

Review of attachment 8341603 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +1363,5 @@
>          PreloadSlowThings();
>      }
> +
> +#ifdef MOZ_NUWA_PROCESS
> +    if (Preferences::GetBool("dom.ipc.processPrelaunch.enabled", false) &&

This is nit picky, but I would move this up further and do

if (!Preferences::GetBool(...)) {
    return true;
}

if ((mIsForApp ....
   ...

#ifdef MOZ_NUWA_PROCESS
   ...

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> I guess those measurements are in bug 941466.

Yes, they are. And bug 941466 is tracking the work for making the preloading work properly as well.

Comment 11

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/9275a2efc9e3
https://hg.mozilla.org/integration/b2g-inbound/rev/a21b57c78253

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9275a2efc9e3
https://hg.mozilla.org/mozilla-central/rev/a21b57c78253

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/06b3a7aea2c0 for frequent (somewhere between 20 and 50%) xpcshell shutdown crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=31651189&tree=B2g-Inbound

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/da8c926f7e4d
https://hg.mozilla.org/integration/b2g-inbound/rev/e6b52f9d4138

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da8c926f7e4d
https://hg.mozilla.org/mozilla-central/rev/e6b52f9d4138

Comment 16

[Preeti Raghunath(:Preeti)]

1.3+ for rel eng.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e63dfbf3661
https://hg.mozilla.org/releases/mozilla-aurora/rev/252715dcde18

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

status-b2g-v1.3T: fixed indicated that its in tarako. remove [tarako] whiteboard