Prelaunch app loads too soon

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dhylands, Assigned: dhylands)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [fast:500ms])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

5 years ago
Created attachment 668688 [details] [diff] [review]
Fix message loop so that idle PostIdleTask works in content children
Attachment #668688 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

5 years ago
Created attachment 668689 [details] [diff] [review]
Defer prelaunch

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

Updated

5 years ago
Attachment #668689 - Flags: review?(jones.chris.g)
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

5 years ago
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)
(Assignee)

Comment 9

5 years ago
Created attachment 668933 [details] [diff] [review]
Fix message loop fo that PostIdleTask works in content children v2
Attachment #668688 - Attachment is obsolete: true
Attachment #668933 - Flags: review?(jones.chris.g)
(Assignee)

Comment 10

5 years ago
Created attachment 668934 [details] [diff] [review]
Defer prelaunch v2

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
Last Resolved: 5 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]
(Assignee)

Comment 16

5 years ago
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?
(Assignee)

Comment 17

5 years ago
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?
(Assignee)

Comment 18

5 years ago
Sorry, I see that if the bug is marked blocking-basecamp then I don't need to request approval.
(Assignee)

Comment 19

5 years ago
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
status-firefox18: --- → fixed
status-firefox19: --- → fixed
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?
You need to log in before you can comment on or make changes to this bug.