Closed
Bug 1195496
Opened 9 years ago
Closed 9 years ago
Start speculative connection earlier in startup
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
Details
Attachments
(4 files)
8.81 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
5.82 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
9.89 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
Right now we start speculative connections in the CLH, which runs rather late in the startup process. We can move it up earlier.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
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•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•