Closed Bug 1339160 Opened 4 years ago Closed 4 years ago

Extract libraries on Gecko thread

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(4 files)

Extract libraries to cache on Gecko thread to avoid blocking the UI thread, and as a way to preload the libraries.
When GeckoThread is launched without being initialized, it will load all
Gecko libs and then wait until it is initialized, before calling the
Gecko entry point. This allows us to preload Gecko libs without actually
running Gecko.
Attachment #8836820 - Flags: review?(snorp)
Add two actions to GeckoService to load libs only, and to load libs plus
Gecko, respectively.
Attachment #8836821 - Flags: review?(snorp)
Use the GeckoService load-gecko action to warm up Gecko instead of using
a separate method.
Attachment #8836823 - Flags: review?(snorp)
Use the GeckoService load-libs action to load and extract new libraries
when we receive the update broadcast. This makes us not block the UI
thread to extract libs, and lets Fennec run normally if the user
launches Fennec right after updating.
Attachment #8836824 - Flags: review?(snorp)
Attachment #8836823 - Attachment description: 3. Use GeckoService to warm up Gecko from custom tabs service (v3) → 3. Use GeckoService to warm up Gecko from custom tabs service (v1)
Comment on attachment 8836820 [details] [diff] [review]
1. Allow GeckoThread to launch without being initialized (v1)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +148,5 @@
> +        setName("Gecko");
> +    }
> +
> +    private boolean isChildProcess() {
> +        return mIPCFileDescriptor != -1;

There should probably be a better way of figuring this out...
Attachment #8836820 - Flags: review?(snorp) → review+
Comment on attachment 8836823 [details] [diff] [review]
3. Use GeckoService to warm up Gecko from custom tabs service (v1)

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

This is a perf regression, because right now we fully start Gecko. I think we want to continue doing that, because the warmup() call is saying "we're very likely to open a custom tab soon".
Attachment #8836823 - Flags: review?(snorp) → review-
Comment on attachment 8836824 [details] [diff] [review]
4. Use GeckoService to extract libs on update (v1)

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

I think blocking the UI thread was done on purpose. How can the user launch fennec "normally" if the libs haven't been extracted yet? Eugen should probably review this anyway.
Attachment #8836824 - Flags: review?(snorp) → review?(esawin)
Comment on attachment 8836821 [details] [diff] [review]
2. Add GeckoService actions for loading libs and Gecko (v1)

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

This is fine, but not sure we need it since I don't really agree with the things that need it.
Attachment #8836821 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #6)
> Comment on attachment 8836823 [details] [diff] [review]
> 3. Use GeckoService to warm up Gecko from custom tabs service (v1)
> 
> Review of attachment 8836823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a perf regression, because right now we fully start Gecko. I think
> we want to continue doing that, because the warmup() call is saying "we're
> very likely to open a custom tab soon".

The new patch also fully starts Gecko, just using existing GeckoService code instead of some code that's specific to custom tabs.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #7)
> Comment on attachment 8836824 [details] [diff] [review]
> 4. Use GeckoService to extract libs on update (v1)
> 
> Review of attachment 8836824 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think blocking the UI thread was done on purpose. How can the user launch
> fennec "normally" if the libs haven't been extracted yet? Eugen should
> probably review this anyway.

We don't need extracted libs to show the UI. Blocking the UI thread makes you unable to launch Fennec at all right after an update, whereas when you extract libs on the Gecko thread, you can still launch Fennec and have the UI display immediately, while the libs extract/load in the background, just like what happens in a "normal" Fennec launch.
(In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #6)
> > Comment on attachment 8836823 [details] [diff] [review]
> > 3. Use GeckoService to warm up Gecko from custom tabs service (v1)
> > 
> > Review of attachment 8836823 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is a perf regression, because right now we fully start Gecko. I think
> > we want to continue doing that, because the warmup() call is saying "we're
> > very likely to open a custom tab soon".
> 
> The new patch also fully starts Gecko, just using existing GeckoService code
> instead of some code that's specific to custom tabs.

Ah, I see. I thought that was the new libs-only action. Maybe rename this one to getIntentToStartGecko() since that's what we're really doing.

> 
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #7)
> > Comment on attachment 8836824 [details] [diff] [review]
> > 4. Use GeckoService to extract libs on update (v1)
> > 
> > Review of attachment 8836824 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think blocking the UI thread was done on purpose. How can the user launch
> > fennec "normally" if the libs haven't been extracted yet? Eugen should
> > probably review this anyway.
> 
> We don't need extracted libs to show the UI. Blocking the UI thread makes
> you unable to launch Fennec at all right after an update, whereas when you
> extract libs on the Gecko thread, you can still launch Fennec and have the
> UI display immediately, while the libs extract/load in the background, just
> like what happens in a "normal" Fennec launch.

Alright, that makes sense.
Comment on attachment 8836824 [details] [diff] [review]
4. Use GeckoService to extract libs on update (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/PackageReplacedReceiver.java
@@ +25,5 @@
>              return;
>          }
>  
> +        // Load new Gecko libs to extracted them to cache.
> +        context.startService(GeckoService.getIntentToLoadLibs(context));

extract
Attachment #8836824 - Flags: review?(esawin) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80d17b6ea48
1. Allow GeckoThread to launch without being initialized; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/26359cfd0d24
2. Add GeckoService actions for loading libs and starting Gecko; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/8620e18b5878
3. Use GeckoService to warm up Gecko from custom tabs service; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/427fd5767291
4. Use GeckoService to extract libs on update; r=esawin
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.