1195496 - Start speculative connection earlier in startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Right now we start speculative connections in the CLH, which runs rather late in the startup process. We can move it up earlier.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Associate queued calls with available states (v1)

Many calls are associated with a Gecko state when they become available.
For example, calls that only depend on XPCOM become available very early
in startup, at the JNI_READY state. However, calls that depend on JS
components may only be available at the end of startup, at the RUNNING
state.

This patch adds an available state to every queued call, so that calls
can be made as soon as they become available, which is important for
calls like making speculative connections.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add PROFILE_READY Gecko state (v1)

Many Gecko operations depend on the profile being available. This
patch adds a PROFILE_READY Gecko state so that we can queue calls
until profile is loaded.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Implement speculative connection method in GeckoThread;

One thing we do in the Fennec CLH is to make a speculative connection
based on the URI that's passed in. However, by the time the CLH runs,
we're far along into startup, and the advantage of a speculative
connection is reduced. This patch implements making speculative
connection as a method in GeckoThread, so that Fennec can make a
speculative connection without relying on the Fennec CLH.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move speculative connection from CLH to GeckoApp;

Moving speculative connection from CLH to GeckoApp allows us to start
the speculative connection very early in the startup process rather
than later in startup.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8648959 [details] [diff] [review]
Move speculative connection from CLH to GeckoApp;


>             GeckoThread.ensureInit(args, action,
>                     TextUtils.isEmpty(uri) ? null : uri,
>                     /* debugging */ ACTION_DEBUG.equals(action));
>+
>+            if (uri != null) {
>+                // Start a speculative connection as soon as Gecko loads.
>+                GeckoThread.speculativeConnect(uri);
>+            }

Is a null check good enough? Above we also check for and empty string:

TextUtils.isEmpty(uri) ? null : uri

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8648958 [details] [diff] [review]
Implement speculative connection method in GeckoThread;

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

Looks good with nits

::: mobile/android/base/GeckoThread.java
@@ +550,5 @@
> +
> +    @WrapForJNI(stubName = "SpeculativeConnect")
> +    private static native void speculativeConnectNative(String uri);
> +
> +    public static void speculativeConnect(final String uri) {

Do we have an annotation that will let us ignore a method (or member variable, etc) in the bindings? We'll never call this from native code, but the generated made a binding anyway.

::: widget/android/nsAppShell.cpp
@@ +160,5 @@
> +    }
> +};
> +
> +already_AddRefed<nsIURI>
> +nsAppShell::ResolveURI(const nsCString& uriStr)

Do you plan to use this other places? I don't think this method makes a lot of sense in nsAppShell. You could just put these handful of lines in the SpeculativeConnect() method instead. Or at least in GeckoThreadNatives.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8648959 [details] [diff] [review]
Move speculative connection from CLH to GeckoApp;

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

Great. Does this result in any measurable perf improvement? In theory it should, right?

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dee6eb23df21
https://hg.mozilla.org/integration/mozilla-inbound/rev/6242c04623f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d89db567a58
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5b38d75c5e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dee6eb23df21
https://hg.mozilla.org/mozilla-central/rev/6242c04623f1
https://hg.mozilla.org/mozilla-central/rev/2d89db567a58
https://hg.mozilla.org/mozilla-central/rev/6d5b38d75c5e