Closed Bug 1195496 Opened 9 years ago Closed 9 years ago

Start speculative connection earlier in startup

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(4 files)

Right now we start speculative connections in the CLH, which runs rather late in the startup process. We can move it up earlier.
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.
Attachment #8648950 - Flags: review?(snorp)
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.
Attachment #8648953 - Flags: review?(snorp)
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.
Attachment #8648958 - Flags: review?(snorp)
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.
Attachment #8648959 - Flags: review?(snorp)
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
Attachment #8648950 - Flags: review?(snorp) → review+
Attachment #8648953 - Flags: review?(snorp) → review+
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.
Attachment #8648958 - Flags: review?(snorp) → review+
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?
Attachment #8648959 - Flags: review?(snorp) → review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: