797239 - Prelaunch app loads too soon

Status: RESOLVED FIXED

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

Description

[Dave Hylands [:dhylands]]

Currently the Prelaunch app launches 1000 msec after launching an app.

If the app being launched takes longer than 1 second to load, then the prelaunch app slows it down.

I think that the ideal time to launch would be when the launched app goes idle for the first time.

With the prelaunch delayMs set to 1000, the settings app takes about 3.3 seconds from the Launching as OOP message until the end of the apps/js/settings.js showBody method (measured on otoro).

With the prelaunch delayMs set to 5000, the settings app only takes about 2.8 seconds.

Comment 1

[Dave Hylands [:dhylands]]

Adding parts 1 and 3 from bug 780692 drops this to around 2.6 seconds (3 runs), although I had run which was only 2.4 seconds.

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Fix message loop so that idle PostIdleTask works in content children

Comment 3

[Dave Hylands [:dhylands]]

Attachment: Defer prelaunch

Defers the very first prelaunch in the parent until the parent goes idle.
Defers subsequent prelaunches until the child process goes idle.

Comment 4

[Dave Hylands [:dhylands]]

Comment on attachment 668688 [details] [diff] [review]
Fix message loop so that idle PostIdleTask works in content children

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

::: ipc/chromium/src/base/message_loop.cc
@@ +31,5 @@
>  #include "base/message_pump_android.h"
>  #endif
>  
>  #include "MessagePump.h"
> +#include "nsThreadUtils.h"

Bah - I missed this earlier. Will remove.

Comment 5

[Dave Hylands [:dhylands]]

I also left some very low frequency prints (one per process launch), as they will be useful for determining when the idle actually happens (it isn't always obvious).

I have also seen a single case where the idle happened earlier than we wanted, but this was with a debug build. The idle events should also happen earlier on an SMP machine, but we're not as concerned about the startup times in that case.

Comment 6

[Dave Hylands [:dhylands]]

I also submitted to try since the message loop stuff could impact other platforms.
https://tbpl.mozilla.org/?tree=Try&rev=683b57aaa6b6

Comment 7

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

Comment on attachment 668688 [details] [diff] [review]
Fix message loop so that idle PostIdleTask works in content children


>diff --git a/ipc/chromium/src/base/message_loop.cc b/ipc/chromium/src/base/message_loop.cc

> #include "MessagePump.h"
>+#include "nsThreadUtils.h"

Looks unused.

Gross but no worse than any of the rest of this code ;).

Comment 8

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

Comment on attachment 668689 [details] [diff] [review]
Defer prelaunch


>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp

>+static void ChildFirstIdle(void)
>+{
>+    printf_stderr("***** ChildFirstIdle *****\n");
>+    ContentChild *child = ContentChild::GetSingleton();
>+    if (!child) {
>+        return;
>+    }

This can never fail, no need to null check.

> PBrowserChild*
> ContentChild::AllocPBrowser(const uint32_t& aChromeFlags,
>                             const bool& aIsBrowserElement, const AppId& aApp)
> {
>+    MessageLoop::current()->PostIdleTask(FROM_HERE, NewRunnableFunction(ChildFirstIdle));
>+

AllocPBrowser() is called for every "tab" we create in the process.  So this code will run for each tab, and it's not really a "first-idle" notification.  That's mostly OK and we'll get away with it in the current setup, but we should maintain semantics.  Just adding a |static bool sentFirstIdle = false;| variable to here or something is fine by me.

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

>+    // Tell the parent that the child has gone idle for the first time
>+    async ChildFirstIdle();
>+

The "Child" is implicit from the direction of the message, child -> parent, so we can drop that.

Otherwise looks great!  The logging is a little gross, so I'd prefer we removed that.  Would like to see the next version.

Really excited to get this big win in :).

Comment 9

[Dave Hylands [:dhylands]]

Attachment: Fix message loop fo that PostIdleTask works in content children v2

Comment 10

[Dave Hylands [:dhylands]]

Attachment: Defer prelaunch v2

Removed log statements, other minor cleanup

Comment 11

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

Comment on attachment 668934 [details] [diff] [review]
Defer prelaunch v2

\o/

Comment 12

[Dave Hylands [:dhylands]]

Pushed to inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2c831793f1a6

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/858a75189c01
https://hg.mozilla.org/mozilla-central/rev/2c831793f1a6

Comment 14

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

Let's let these patches bake for a bit and request aurora approval.

Comment 15

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

These are very safe patches that are required to meet app startup targets for v1.

There's zero risk for other products.

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 668933 [details] [diff] [review]
Fix message loop fo that PostIdleTask works in content children v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 797239
User impact if declined: 0.5 sec slower launch time for apps
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): very safe
String or UUID changes made by this patch: None

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 668934 [details] [diff] [review]
Defer prelaunch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 797239
User impact if declined: 0.5 sec slower launch time for apps
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): very safe
String or UUID changes made by this patch: None

Comment 18

[Dave Hylands [:dhylands]]

Sorry, I see that if the bug is marked blocking-basecamp then I don't need to request approval.

Comment 19

[Dave Hylands [:dhylands]]

Comment on attachment 668933 [details] [diff] [review]
Fix message loop fo that PostIdleTask works in content children v2

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

::: ipc/chromium/src/base/message_loop.cc
@@ +336,5 @@
>    nestable_tasks_allowed_ = true;
>  }
>  
>  bool MessageLoop::DeferOrRunPendingTask(const PendingTask& pending_task) {
> +  if (pending_task.nestable || state_->run_depth >= run_depth_base_) {

I'm looking at this again, and I think that the >= should be <=

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/85aa1352aba7
https://hg.mozilla.org/releases/mozilla-aurora/rev/90a5685dc1be