708967 - Move gecko startup into onCreate

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

We can save a ton of startup time (300ms on Ts or so) by moving library loading from onNewIntent to onCreate.  We probably will only see the win on faster devices.


NOTES:

0) I simplified startup a bunch.  For example, now GeckoThread just loads libraries, sends a event that they are loaded, and runs gecko.

1) adding android:largeHeap seems to reduce the number of GC's on my ICS device (from 5 to 4) during startup

2) I broke ACTION_DEBUG.  jimdb will be busted until we fix it up.  Lets see if we can get jim to take a look.

3) I removed mUserDefinedProfile.  It was no longer needed

4) I removed library extraction.  It is no longer needed to debug.  I left the native code the way it was in case there was some need to pass the flag to extract libraries again.

5) Needed to protect IME a bit as it was being called pretty early.  That is what all of those |mInputConnection == null| checks are about.



If this regresses lower-end devices, we can move the startup back to onNewIntent conditionally.  Most of the changes though can remain.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580324 [details] [diff] [review]
patch v.1

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

getProfile will be wrong if -profile is passed on the command line

I can do a more thorough review later or when there's a new patch, whichever comes first

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580324 [details] [diff] [review]
patch v.1

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

getProfile will be wrong if -profile is passed on the command line

I can do a more thorough review later or when there's a new patch, whichever comes first

::: mobile/android/base/GeckoAppShell.java
@@ +422,1 @@
>          GeckoAppShell.nativeRun(combinedArgs);

you're throwing out all arguments to gecko. So this will break testing as well as make us not load a page if launched from another app.

Comment 3

[Doug Turner (:dougt)]

the launching from other apps does work!  it get funneled to onNewIntent (either via onCreate, or by the system), then we post a message to gecko.

I'll take a look at the -profile logic soon!

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580757 [details] [diff] [review]
patch v.2

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

you're still ignoring the passed in arguments, which will break testing. Have you tried running tests locally? or pushing to try?

::: mobile/android/base/GeckoThread.java
@@ +44,5 @@
>  
>  public class GeckoThread extends Thread {
>      private static final String LOGTAG = "GeckoThread";
>  
> +    GeckoThread () {}

you don't need this

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.3

https://tbpl.mozilla.org/?tree=Try&rev=648dabc3366f

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580827 [details] [diff] [review]
patch v.3

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

These patches seem to do a whole lot more than move gecko start-up into onCreate. It might be better to break it up into smaller changes in separate bugs rather than let some of these riskier changes hold up the stated goal of this bug.

::: mobile/android/base/GeckoAppShell.java
@@ -435,4 @@
>          if (args != null)
>              combinedArgs += " " + args;
> -        if (url != null)
> -            combinedArgs += " -remote " + url;

why are you getting rid of this? As far as I can tell we don't use mLastUri at all now, so if we get OOM killed and comeback with a screenshot we won't load the page the screenshot is from in the background.

@@ +428,2 @@
>  
> +        String uri = intent.getDataString();

You're not checking mLastUri at all now, so if we get OOM killed and comeback with a screenshot we won't load the page the screenshot is from in the background.

@@ -989,5 @@
>  
>      public static void setKeepScreenOn(final boolean on) {
> -        GeckoApp.mAppContext.runOnUiThread(new Runnable() {
> -            public void run() {
> -                // TODO

is there a bug open for having to re-implement this? If not, please file one. Or, while you're here you can just call mLayerManager.getView().setKeepScreenOn(on);

::: mobile/android/base/GeckoThread.java
@@ -99,5 @@
> -        Locale.setDefault(locale);
> -        Resources res = app.getBaseContext().getResources();
> -        Configuration config = res.getConfiguration();
> -        config.locale = locale;
> -        res.updateConfiguration(config, res.getDisplayMetrics());

why is it okay to get rid of this?

@@ -108,5 @@
> -
> -        final String activityTitle = title;
> -        app.mMainHandler.post(new Runnable() {
> -            public void run() {
> -                app.mBrowserToolbar.setTitle(activityTitle);

you need to do this somewhere for the screenshot to look right. Should probably just do it in onCreate since it needs to be on the main UI thread

Comment 8

[Doug Turner (:dougt)]

yes, i broke the last screenshot.  will test.

setKeepScreenOn fixed.

Locale.setDefault changes should have been removed. 701068 will track what we should do wrt locale changes.

patch coming up!

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580841 [details] [diff] [review]
patch v.4

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

r- for the double load problem. Getting close though.

::: mobile/android/base/GeckoApp.java
@@ -389,5 @@
> -
> -                String args = intent.getStringExtra("args");
> -                if (args != null && args.contains("-profile")) {
> -                    // XXX: TO-DO set mProfileDir to the path passed in
> -                    mUserDefinedProfile = true;

since you're dropping mUserDefinedProfile doing this TO-DO is more important now. please check for -profile in the args and set mProfile to the next argument in the list.

the effect is minimal now, just the recommended addons json file, but when local DB bookmarks and history lands this will break any testing of that

@@ +1236,5 @@
>  
> +        mAppContext = this;
> +        mMainHandler = new Handler();
> +
> +        if (sGREDir == null)

in what case with sGREDir not be null? please drop this check if its not needed or comment as to why it is needed

@@ +1319,5 @@
>               * there is no cached screenshot; in that case, we have no choice but to display a
>               * checkerboard.
>               *
>               * TODO: Fall back to a built-in screenshot of the Fennec Start page for a nice first-
>               * run experience, perhaps?

kill this comment, we use a live about:home now

@@ +1322,5 @@
>               * TODO: Fall back to a built-in screenshot of the Fennec Start page for a nice first-
>               * run experience, perhaps?
>               */
>              mLayerController = new LayerController(this);
> +            mPlaceholderLayerClient = PlaceholderLayerClient.createInstance(this);

unrelated to this bug, but I think we only need to instantiate the place holder layer if we have a screenshot to display. Please file a follow up bug to look at that.

@@ +1327,4 @@
>              if (mPlaceholderLayerClient != null) {
>                  mLayerController.setLayerClient(mPlaceholderLayerClient);
> +                if (mLastUri != null && mLastTitle != null) { 
> +                    GeckoAppShell.sendEventToGecko(new GeckoEvent(mLastUri));

this is wrong, if you get OOM killed and then launch from another app you'll wind up loading that page twice. 

Please just pass mLastUri through GeckoThread to runGecko()

::: mobile/android/base/GeckoAppShell.java
@@ -401,5 @@
> -                    if (libFile.getName().endsWith(".so"))
> -                        libFile.delete();
> -                }
> -            }
> -        }

this needs to be moved into our xul -> native migration code rather than just being dropped completely

@@ +428,4 @@
>  
> +        String uri = intent.getDataString();
> +        if ((uri != null && uri.length() != 0)) {
> +            combinedArgs += " -remote " + uri;        

this is the other side of the "loading the page twice" problem. mLastUri get's set to intent.getDataString() back in onCreate if it is non-null.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 583080 [details] [diff] [review]
patch v.5

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

::: mobile/android/base/GeckoApp.java
@@ +749,5 @@
> +            Intent intent = getIntent();
> +            String args = intent.getStringExtra("args");
> +            StringTokenizer st = new StringTokenizer(args);
> +
> +            while (st.hasMoreTokens())

I'm worried about the time we'll spend in this loop when there is no -profile flag. Can you add:

if (!args.contains("-profile"))
    return;

@@ +766,2 @@
>      public File getProfileDir() {
> +        checkForUserDefinedProfile();

you're going to call this again on the first line of getProfileDir(String), so this isn't required

@@ +1492,3 @@
>               */
>              mLayerController = new LayerController(this);
> +            mPlaceholderLayerClient = PlaceholderLayerClient.createInstance(this);

should we even bother creating this if there is no screenshot?

@@ +1630,5 @@
>              System.exit(0);
>              return;
>          }
> +
> +        if (checkAndSetLaunchState(LaunchState.Launching, LaunchState.Launched)) {

add a comment that this only executes on slow devices

@@ +1644,1 @@
>              prefetchDNS(intent.getData());

you probably only want to do this if gecko isn't already running, right?

::: mobile/android/base/GeckoAppShell.java
@@ +418,5 @@
>          Log.i(LOGTAG, "setSoftwareLayerClient called");
>  
> +        Intent intent = GeckoApp.mAppContext.getIntent();
> +        String args = intent.getStringExtra("args");
> +        String url = intent.getDataString();

so how does mLastUri get loaded if its in the bundle?

Comment 13

[Doug Turner (:dougt)]

> I'm worried about the time we'll spend in this loop when there is no -profile flag. Can you add:

It isn't 1ms in my timing.

> you're going to call this again on the first line of getProfileDir(String), so this isn't required

Removed.

>should we even bother creating this if there is no screenshot?

followup.  the screenshot code doesn't even work atm.

>add a comment that this only executes on slow devices

Sure.  done.

> you probably only want to do this if gecko isn't already running, right?

Discussed with you weeks ago.  we decided to run this for the case where the radio when down because you switched to another application.

> so how does mLastUri get loaded if its in the bundle?

The bundle and loading the splash screen is completely broken right now and has been for at least a week.  I will address this in a followup.

Do you want to see another patch, or are we good here?

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Doug Turner (:dougt) from comment #13)
> > I'm worried about the time we'll spend in this loop when there is no -profile flag. Can you add:
> 
> It isn't 1ms in my timing.
Please still return early. I doubt your testing with args anyway.

 
> > so how does mLastUri get loaded if its in the bundle?
> 
> The bundle and loading the splash screen is completely broken right now and
> has been for at least a week.  I will address this in a followup.
Knowingly breaking it further doesn't help anyone

> Do you want to see another patch, or are we good here?

Yes, don't further break the screenshots

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey] from comment #14)
> (In reply to Doug Turner (:dougt) from comment #13)
> > > so how does mLastUri get loaded if its in the bundle?
> > 
> > The bundle and loading the splash screen is completely broken right now and
> > has been for at least a week.  I will address this in a followup.
> Knowingly breaking it further doesn't help anyone
Also, I just tested and the screenshot works fine for me

Comment 16

[Doug Turner (:dougt)]

lets move that discussion to bug 712543 because screenshots do not work for me on any of the 3 devices I have.

Comment 17

[Doug Turner (:dougt)]

sounds like blassey landed a similar patch which basically does the same thing minus the clean up.  wontfixing...