Closed Bug 797239 Opened 7 years ago Closed 7 years ago

Prelaunch app loads too soon

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

(Whiteboard: [fast:500ms])

Attachments

(2 files, 2 obsolete files)

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.
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.
Attached patch Defer prelaunch (obsolete) — Splinter Review
Defers the very first prelaunch in the parent until the parent goes idle.
Defers subsequent prelaunches until the child process goes idle.
Attachment #668689 - Flags: review?(jones.chris.g)
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.
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.
I also submitted to try since the message loop stuff could impact other platforms.
https://tbpl.mozilla.org/?tree=Try&rev=683b57aaa6b6
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 ;).
Attachment #668688 - Flags: review?(jones.chris.g) → review+
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 :).
Attachment #668689 - Flags: review?(jones.chris.g)
Attachment #668688 - Attachment is obsolete: true
Attachment #668933 - Flags: review?(jones.chris.g)
Removed log statements, other minor cleanup
Attachment #668689 - Attachment is obsolete: true
Attachment #668934 - Flags: review?(jones.chris.g)
Attachment #668933 - Flags: review?(jones.chris.g) → review+
Comment on attachment 668934 [details] [diff] [review]
Defer prelaunch v2

\o/
Attachment #668934 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/mozilla-central/rev/858a75189c01
https://hg.mozilla.org/mozilla-central/rev/2c831793f1a6
Assignee: nobody → dhylands
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: General → DOM
Product: Boot2Gecko → Core
Let's let these patches bake for a bit and request aurora approval.
Whiteboard: [needs-checkin-aurora]
These are very safe patches that are required to meet app startup targets for v1.

There's zero risk for other products.
blocking-basecamp: --- → +
Whiteboard: [needs-checkin-aurora] → [needs-checkin-aurora][fast:500ms]
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
Attachment #668933 - Flags: approval-mozilla-aurora?
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
Attachment #668934 - Flags: approval-mozilla-aurora?
Sorry, I see that if the bug is marked blocking-basecamp then I don't need to request approval.
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 <=
https://hg.mozilla.org/releases/mozilla-aurora/rev/85aa1352aba7
https://hg.mozilla.org/releases/mozilla-aurora/rev/90a5685dc1be
Flags: in-testsuite-
Whiteboard: [needs-checkin-aurora][fast:500ms] → [fast:500ms]
Target Milestone: --- → mozilla19
Attachment #668933 - Flags: approval-mozilla-aurora?
Attachment #668934 - Flags: approval-mozilla-aurora?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.