1062377 - Load profile.ini in background if possible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Right now we always get a StrictMode warning about reading profile.ini, but I believe we can delay profile-accessing code, so that we have enough time to prefetch profile.ini on a background thread, before any UI thread code accesses the profile.

Comment 1

[Wesley Johnston (:wesj)]

See also, bug 894887 which was about not parsing this at all. I'd love it if someone could fix that.

Comment 2

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

I also just filed bug 1062074 for something related.

We definitely need to access the profile.ini during startup. How early, I'm not sure. I recently heard about something called a FutureTask<> that might be useful here. I'm assuming a FutureTask is executed early, does it's work in the Background. If completed before the first access, no blocking the thread. If not, the thread blocks, but it's unavoidable and no worse than the current state.

Comment 3

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

Attachment: Try loading profile.ini in background (v1)

We can move our profile accessing code to later in the startup process. The benefit of that is in early startup, we can prefetch the profile.ini file in the background and not block the UI thread

This patch moves BrowserDB initialization to right before when it's needed. SuggestedSites also used the profile on startup but doesn't have to, and this patch changes that behavior.

Comment 4

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

Comment on attachment 8483626 [details] [diff] [review]
Try loading profile.ini in background (v1)

>diff --git a/mobile/android/base/db/SuggestedSites.java b/mobile/android/base/db/SuggestedSites.java

>     public SuggestedSites(Context appContext, Distribution distribution, File file) {
>         this.context = appContext;
>         this.distribution = distribution;
>         this.file = file;
>     }

Check with Lucas. Maybe we don't even need this constructor anymore.

Comment 5

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

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 8483626 [details] [diff] [review]
> Try loading profile.ini in background (v1)
> 
> >diff --git a/mobile/android/base/db/SuggestedSites.java b/mobile/android/base/db/SuggestedSites.java
> 
> >     public SuggestedSites(Context appContext, Distribution distribution, File file) {
> >         this.context = appContext;
> >         this.distribution = distribution;
> >         this.file = file;
> >     }
> 
> Check with Lucas. Maybe we don't even need this constructor anymore.

I saw this constructor being used during startup, but Lucas would know more.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8483626 [details] [diff] [review]
Try loading profile.ini in background (v1)

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

::: mobile/android/base/db/SuggestedSites.java
@@ +300,5 @@
>                  // Update cached list of sites.
>                  setCachedSites(sites);
>  
>                  // Save the result to disk.
> +                final File file = getFile();

Kinda dangerous to use synchronized(file) in a non-final member. But I can live with this.

Comment 7

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

Comment on attachment 8483626 [details] [diff] [review]
Try loading profile.ini in background (v1)

Try looks good, https://tbpl.mozilla.org/?tree=Try&rev=6a0c27209be8

Comment 8

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

Attachment: Rewrite GeckoBackgroundThread to not wait for thread start if possible (v1)

I found that when GeckoBackgroundThread is first used, it starts a new thread and waits for a Handler on the UI thread. This can delay startup somewhat. Instead, we can just save the Runnable, let the UI thread continue, and run the saved Runnable when the background thread is up and running.

Comment 9

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

Comment on attachment 8485073 [details] [diff] [review]
Rewrite GeckoBackgroundThread to not wait for thread start if possible (v1)

I don't think I am tall enough to review this. Passing to Richard.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8483626 [details] [diff] [review]
Try loading profile.ini in background (v1)

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

I have some concerns about this; see inline. See what happens to overall startup time when you remove the speculative prefetch altogether.

::: mobile/android/base/GeckoApp.java
@@ +1167,5 @@
>                  }
>              }
>          }
>  
> +        // Fetch profile in the background if possible.

I'd change this to "Speculatively pre-fetch the profile in the background.".

@@ +1445,5 @@
>          Tabs.registerOnTabsChangedListener(this);
>  
>          initializeChrome();
>  
> +        BrowserDB.initialize(getProfile().getName());

This call is in initialize, which is called from onWindowFocusChanged.

My guess is that this will be invoked *after* the background init work on line 1259, where BrowserHealthRecorder will already access the profile.

That is: your speculative fetch in the previous hunk is probably unnecessary, because we'll already do it for real in the very next background runnable, and you're only delaying that by queuing another runnable.

If onWindowFocusChanged is called *first*, then again, the speculative fetch is probably unnecessary -- if oWFC is gonna beat the current background runnable, it'll beat the new one, too.

::: mobile/android/base/db/SuggestedSites.java
@@ +145,5 @@
>      }
>  
>      /* inner-access */ final Context context;
>      /* inner-access */ final Distribution distribution;
> +    /* inner-access */ File file;

This can now be private, if all access has been changed to be via `getFile`.

And please comment that this is lazily computed.

@@ +163,5 @@
>          this.distribution = distribution;
>          this.file = file;
>      }
>  
> +    private synchronized File getFile() {

Make this package-scoped.

@@ +300,5 @@
>                  // Update cached list of sites.
>                  setCachedSites(sites);
>  
>                  // Save the result to disk.
> +                final File file = getFile();

Careful: you're shadowing the member here, so the `synchronized` below is now misleading. I'd suggest using a different name.

Comment 11

[Richard Newman [:rnewman]]

An overall comment, too, before I tackle the next patch: main-thread IO (the cause of the StrictMode warning) isn't necessarily bad, and work scheduling has knock-on effects.

We're definitely going to need to parse profiles.ini before our data sources are available. That means it'll be done before we can finish showing UI -- suggested sites, your chosen home panels, etc.

It might yield the best overall user experience -- that is, time-to-working-browser -- to just muffle the StrictMode warning (as we always do in release builds) and roll on.

I'd like positive numbers (on both single-core and multi-core devices) to back this up before we go ahead. Maybe autophone is needed for that?

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8485073 [details] [diff] [review]
Rewrite GeckoBackgroundThread to not wait for thread start if possible (v1)

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

This is actually a lot like how Thread is supposed to work:

  run()
  If this thread was constructed using a separate Runnable run object,
  then that Runnable object's run method is called; otherwise, this 
  method does nothing and returns.

so props for cleaning up a discrepancy here.

This looks good to me, but I'd like a minimal test case, and I have a couple of questions inline.

::: mobile/android/base/util/GeckoBackgroundThread.java
@@ -13,5 @@
>      private static final String LOOPER_NAME = "GeckoBackgroundThread";
>  
> -    // Guarded by 'this'.
> -    private static Handler sHandler;
> -    private SynchronousQueue<Handler> mHandlerQueue = new SynchronousQueue<Handler>();

I'm glad to see mHandlerQueue die!

@@ +15,5 @@
> +    // Guarded by 'GeckoBackgroundThread.class'.
> +    private static Handler handler;
> +    private static Thread thread;
> +
> +    // The initial Runnable to run on a new thread. Its purpose

on *the* new thread.

@@ +35,5 @@
> +            GeckoBackgroundThread.class.notify();
> +        }
> +
> +        if (initialRunnable != null) {
> +            initialRunnable.run();

I'd like a test that ensures that the first runnable is not invoked on the calling thread. This seems easy to regress.

@@ +36,5 @@
> +        }
> +
> +        if (initialRunnable != null) {
> +            initialRunnable.run();
> +            initialRunnable = null;

Clear the runnable before you run it. If it throws, we don't want to try again.

What happens if we throw here?

@@ +47,5 @@
> +        final Thread newThread = new GeckoBackgroundThread(initialRunnable);
> +        newThread.setDaemon(true);
> +        newThread.start();
> +
> +        ThreadUtils.setBackgroundThread(newThread);

Is there a reason why you do this *after* starting the thread? We used to do it before.

Comment 13

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

Attachment: Try loading profile.ini in background (v2)

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 8483626 [details] [diff] [review]
> Try loading profile.ini in background (v1)
> 
> Review of attachment 8483626 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the comments! I addressed most of them.

> @@ +1445,5 @@
> >          Tabs.registerOnTabsChangedListener(this);
> >  
> >          initializeChrome();
> >  
> > +        BrowserDB.initialize(getProfile().getName());
> 
> This call is in initialize, which is called from onWindowFocusChanged.
> 
> My guess is that this will be invoked *after* the background init work on
> line 1259, where BrowserHealthRecorder will already access the profile.
> 
> That is: your speculative fetch in the previous hunk is probably
> unnecessary, because we'll already do it for real in the very next
> background runnable, and you're only delaying that by queuing another
> runnable.
> 
> If onWindowFocusChanged is called *first*, then again, the speculative fetch
> is probably unnecessary -- if oWFC is gonna beat the current background
> runnable, it'll beat the new one, too.

A good chunk of our Java startup time is spent in setContentView [1] (around 300ms out of 1000ms on my N4), which happens before we do background init. That 300ms is a good time to do blocking I/O on the background thread. We might be able to move background init to before setContentView; in any case, we have a better chance of avoiding blocking the main thread if we fetch it sooner rather than later.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1237

Comment 14

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

(In reply to Richard Newman [:rnewman] from comment #11)
> An overall comment, too, before I tackle the next patch: main-thread IO (the
> cause of the StrictMode warning) isn't necessarily bad, and work scheduling
> has knock-on effects.

I agree. Unfortunately, profile.ini is read near the very beginning of startup, which is one critical area where we want to minimize UI thread I/O as best as we can IMO.

> We're definitely going to need to parse profiles.ini before our data sources
> are available. That means it'll be done before we can finish showing UI --
> suggested sites, your chosen home panels, etc.
> 
> It might yield the best overall user experience -- that is,
> time-to-working-browser -- to just muffle the StrictMode warning (as we
> always do in release builds) and roll on.

As explained above, I think the time spent in setContentView is a good opportunity to read profile.ini on the background thread. Instead of waiting on I/O, we'd be performing actual work.

> I'd like positive numbers (on both single-core and multi-core devices) to
> back this up before we go ahead. Maybe autophone is needed for that?

I agree with having numbers, but it's hard to measure the impact of this as it depends on many factors. I think Autophone may be the best bet.

Comment 15

[Wesley Johnston (:wesj)]

My 2cents, we really don't need to parse profiles.ini at all. I'm sorry I made us do it :(

We should be able to assume default (as long as we always pass a profile to Gecko when we start it).

Comment 16

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

(In reply to Wesley Johnston (:wesj) from comment #15)
> My 2cents, we really don't need to parse profiles.ini at all. I'm sorry I
> made us do it :(
> 
> We should be able to assume default (as long as we always pass a profile to
> Gecko when we start it).

Not parsing profiles.ini might break working with Gecko in other ways. I have an add-on that allows switching between profiles because it uses nsIToolkitProfileService, which uses profiles.ini internally. Let's not get into a bad extensibility state.

Comment 17

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

Attachment: Rewrite GeckoBackgroundThread to not wait for thread start if possible (v2)

Addressed most comments. Test will be in another patch.

(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 8485073 [details] [diff] [review]
> Rewrite GeckoBackgroundThread to not wait for thread start if possible (v1)
> 
> Review of attachment 8485073 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +36,5 @@
> > +        }
> > +
> > +        if (initialRunnable != null) {
> > +            initialRunnable.run();
> > +            initialRunnable = null;
> 
> Clear the runnable before you run it. If it throws, we don't want to try
> again.
> 
> What happens if we throw here?

A Thread instance can only be run once. If we throw here, the thread is killed and we crash.

> @@ +47,5 @@
> > +        final Thread newThread = new GeckoBackgroundThread(initialRunnable);
> > +        newThread.setDaemon(true);
> > +        newThread.start();
> > +
> > +        ThreadUtils.setBackgroundThread(newThread);
> 
> Is there a reason why you do this *after* starting the thread? We used to do
> it before.

Moved it back to before. Looking at ThreadUtils, the correct placement is before the thread starts.

Comment 18

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

Attachment: Add JUnit test for new GeckoBackgroundThread (v1)

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 8486030 [details] [diff] [review]
Rewrite GeckoBackgroundThread to not wait for thread start if possible (v2)

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

::: mobile/android/base/util/GeckoBackgroundThread.java
@@ +36,5 @@
> +        }
> +
> +        if (initialRunnable != null) {
> +            initialRunnable.run();
> +            initialRunnable = null;

OK, so no need to clear this first, because if we don't reach this line, we've crashed hard :)

Fair enough!

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8486031 [details] [diff] [review]
Add JUnit test for new GeckoBackgroundThread (v1)

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

Nice, really elegant.

Comment 21

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

https://hg.mozilla.org/integration/fx-team/rev/ec9b51158d34
https://hg.mozilla.org/integration/fx-team/rev/8f323dfa4f0f
https://hg.mozilla.org/integration/fx-team/rev/e34198954c2c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for troboprovider perma-fail.
https://hg.mozilla.org/integration/fx-team/rev/83f04fa0fa20

https://tbpl.mozilla.org/php/getParsedLog.php?id=47711500&tree=Fx-Team

Comment 23

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

Attachment: Use separate LocalBrowserDB instance for initializing new profile (v1)

Because we're moving back BrowserDB initialization, it's possible that BrowserDB is not yet initialized when we add default bookmarks during profile creation. I think we can either initialize BrowserDB here or use a separate LocalBrowserDB instance (what this patch does).

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8488807 [details] [diff] [review]
Use separate LocalBrowserDB instance for initializing new profile (v1)

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

Neat. This is almost certainly the simplest and least coupled approach, even though it does guarantee that we'll have two LocalBrowserDB instantiations during first run.

Comment 25

[Richard Newman [:rnewman]]

Amusingly, the actual test itself isn't failing in the log in comment 22.

TestBrowserProviderPerf is an odd duck. It's not really a robocop test, but it starts the activity. It's the initialization of the profile for the launched browser that's failing, not the test!

Comment 26

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

Attachment: Lock profile and use separate LocalBrowserDB instance for initializing new profile (v1)

This finally fixes the test failures; let me know if you think it's the right approach.

Basically because we're doing delayed initialization in the background, there can be a race if the main thread tries do things like removing the profile before initialization completes. This race was causing testGeckoProfile to fail. This patch makes us synchronize with the main thread when doing delayed initialization.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 8493174 [details] [diff] [review]
Lock profile and use separate LocalBrowserDB instance for initializing new profile (v1)

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

I feel slightly conflicted about this.

I believe this change is safe; the combination of synchronization and checking whether the directory exists is valid. And it's true that the init code you touched assumes that the profile will continue to exist during that block of code, which is a slightly flawed assumption.

Here are some of my concerns.

1. Access to the profile directory that doesn't go through the same GeckoProfile instance will not be affected by the synchronization. For example, deleting the profile directory by getting its path and using File. Perhaps we do that in other tests? This shouldn't be a concern, so long as we always use GeckoProfile correctly.

2. It's quite possible that we have other code that makes profile lifecycle assumptions, so this might be a bit whack-a-mole.

3. It locks that profile instance while we perform bookmark insertion, which can take an arbitrary amount of time. This shouldn't reduce throughput, assuming that all use of GeckoProfile is isolated to a single thread. But that you needed to make this change to avoid test failures implies that this is perhaps not the case!


(3) I think is the main area of worry. What are we doing to our first-run throughput here by synchronizing?

And do we care?

I'd like to hear your thoughts on this before signing off.

Comment 28

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

(In reply to Richard Newman [:rnewman] from comment #27)
> Comment on attachment 8493174 [details] [diff] [review]
> Lock profile and use separate LocalBrowserDB instance for initializing new
> profile (v1)
> 
> (3) I think is the main area of worry. What are we doing to our first-run
> throughput here by synchronizing?
> 
> And do we care?
> 
> I'd like to hear your thoughts on this before signing off.

Yeah I agree with all the concerns.

Looking at GeckoProfile, we only synchronize on GeckoProfile instance when potentially doing I/O (calling getDir(), remove(), etc.). Furthermore, let's assume all profile I/O after creation happens on the background thread. Assuming that, we'd be waiting on delayed initialization in any case, with or without this patch. So if that assumption is valid, I don't think the patch would have much of an impact. I don't have any real data to back that up though.

On the other hand, the test was failing because we try to remove the profile right after creating it, ignoring queued initialization. However, in actual Fennec code, it seems we only ever delete webapp profiles, which don't do delayed initialization. So from a quick look, it appears we will never run into that race condition in actual usage. Therefore, you could argue that we should fix the test, because it's testing something that should never happen. Personally though, I think I prefer that we fix the race condition whether or not it actually happens.

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8493174 [details] [diff] [review]
Lock profile and use separate LocalBrowserDB instance for initializing new profile (v1)

Let's try it, but let's be prepared for this to regress startup or cause other problems, and for it to be backed out!

Comment 30

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b37f591b400e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c23bb480ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2cf626b77e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9473c599af8f

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b37f591b400e
https://hg.mozilla.org/mozilla-central/rev/c8c23bb480ec
https://hg.mozilla.org/mozilla-central/rev/aa2cf626b77e
https://hg.mozilla.org/mozilla-central/rev/9473c599af8f