Closed Bug 1062377 Opened 7 years ago Closed 7 years ago

Load profile.ini in background if possible

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
See also, bug 894887 which was about not parsing this at all. I'd love it if someone could fix that.
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.
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 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.
(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.
Flags: needinfo?(lucasr.at.mozilla)
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.
Flags: needinfo?(lucasr.at.mozilla)
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
Attachment #8483626 - Flags: review?(mark.finkle)
Attachment #8483626 - Flags: review?(mark.finkle) → review+
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.
Attachment #8485073 - Flags: review?(mark.finkle)
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.
Attachment #8485073 - Flags: review?(mark.finkle) → review?(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.
Attachment #8483626 - Flags: review-
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 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.
Attachment #8485073 - Flags: review?(rnewman) → feedback+
(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
Attachment #8483626 - Attachment is obsolete: true
Attachment #8485889 - Flags: review?(rnewman)
(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.
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).
(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.
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.
Attachment #8485073 - Attachment is obsolete: true
Attachment #8486030 - Flags: review?(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!
Attachment #8486030 - Flags: review?(rnewman) → review+
Comment on attachment 8486031 [details] [diff] [review]
Add JUnit test for new GeckoBackgroundThread (v1)

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

Nice, really elegant.
Attachment #8486031 - Flags: review?(rnewman) → review+
Attachment #8485889 - Flags: review?(rnewman) → review+
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).
Attachment #8488807 - Flags: review?(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.
Attachment #8488807 - Flags: review?(rnewman) → review+
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!
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.
Attachment #8488807 - Attachment is obsolete: true
Attachment #8493174 - Flags: review?(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.
(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 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!
Attachment #8493174 - Flags: review?(rnewman) → 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.