Closed Bug 1130810 Opened 9 years ago Closed 3 years ago

Use ContentProviderClient in LocalBrowserDB

Categories

(Firefox for Android Graveyard :: Data Providers, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: rnewman, Unassigned, Mentored)

Details

(Keywords: perf, Whiteboard: [good next bug][lang=java])

Attachments

(1 file)

ContentResolver comes with its own overhead. ContentProviderClient exists to allow callers to do the resolution and lookup once, getting a handle to the CP directly. This only works in the same process, but we're in the same process.

We should consider using CPC in LocalBrowserDB. This ought to buy us some easy perf wins across the board.
I'll start working on this. I've nosed through the LocalBrowserDB class and some documentation on the two classes, and think I understand what needs to be done.
Is it better to change the methods in LocalBrowserDB to accept the ContentProviderClient, or to acquire and release it from within each method independently?
Flags: needinfo?(rnewman)
The main value from using CPC is in having an instance with a longer lifetime than a single method.


I suggest making the client as a member during LocalBrowserDB's constructor as a first stab; this might impact startup time, but we can cross that bridge when we come to it.

The alternative implementation is as a ThreadLocal, populated when needed.


CPCs should be scoped to a thread, so that implies either ignoring that warning, assuming that all of our access is on the background thread, or using ThreadLocal.

It's possible that this bug will expose threading issues that using ContentResolver implicitly worked around.


This leaves the question of when to release the CPC. I have some ideas here, but nothing good enough to note yet.
Flags: needinfo?(rnewman)
Attached patch Bug1130810.patchSplinter Review
Well I've changed the class to use the ContentProviderClient, basically whenever a public method it tries to grab the CPC instance from the ThreadLocal variable, if it doesn't have one it creates it.

I wasn't sure about the best way to release it. I know overriding the finalize method is usually not the best way to go about things, but I'm not sure of a better way without finding every time LocalBrowserDB is instantiated and calling it from outside.
Attachment #8570061 - Flags: review?(michael.l.comella)
Comment on attachment 8570061 [details] [diff] [review]
Bug1130810.patch

Richard is better suited for this one.
Attachment #8570061 - Flags: review?(michael.l.comella) → review?(rnewman)
Assignee: nobody → tylertstonge
Status: NEW → ASSIGNED
Comment on attachment 8570061 [details] [diff] [review]
Bug1130810.patch

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

Sorry for the slightly delayed response!



I'm gonna noodle for a minute about lifecycles; bear with me.

Ideally the scope of a CPC here would be about the same as the main browser activity. When we stop browsing, we release the reference and allow the CP to be closed if necessary, but during ordinary operation we only need to look up the BrowserProvider once.

Alas, that's not the case if we store a reference in LocalBrowserDB: a browser DB is held by a GeckoProfile instance, and the GeckoProfile instance is statically cached. The cache is never cleared until the process dies. A finalizer, then, is only a nicety -- the object reference lasts as long as the process, so our finalizer runs when the process dies, if ever.


So we need a way to make sure that the LBDB grabs a CPC reference when needed, but has a mechanism to safely discard it when given an *external* instruction. We can invoke that cleanup when our chosen activity is finished, bearing in mind the possibility of race conditions, where a background task has either captured the CPC or will result in a new one being obtained after cleanup.

Think about that for a little while.



Otherwise, my repetitive comments:

* Don't muffle exceptions. If we get a RemoteException, we want to die, rather than silently swallow failures on insert or query.
* Don't break indentation.
* Spaces between operators and syntax elements.
* Make locals final if you can.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +120,5 @@
>          mFolderIdMap = new HashMap<String, Long>();
> +        mClient = new ThreadLocal<ContentProviderClient>() {
> +            @Override
> +            protected void finalize() throws Throwable {
> +                if(this.get() != null) {

Nit: space between 'if' and '('.

@@ +176,5 @@
> +    private ContentProviderClient getContentProviderClient(ContentResolver cr) {
> +        if(mClient.get() == null) {
> +            mClient.set(cr.acquireContentProviderClient(BrowserContract.AUTHORITY_URI));
> +        }
> +        return mClient.get();

This version calls .get() once.

ContentProviderClient cpc = mClient.get();
if (cpc != null) {
    return cpc;
}
cpc = cr.acquireContentProviderClient(…);
mClient.set(cpc);
return cpc;

@@ +215,5 @@
>       * Takes an offset; returns a new offset.
>       */
>      @Override
>      public int addDefaultBookmarks(Context context, ContentResolver cr, final int offset) {
> +        ContentProviderClient client = getContentProviderClient(cr);

final.

@@ +314,5 @@
>       * Takes an offset; returns a new offset.
>       */
>      @Override
>      public int addDistributionBookmarks(ContentResolver cr, Distribution distribution, int offset) {
> +        ContentProviderClient client = getContentProviderClient(cr);

final.

@@ +583,5 @@
> +                    projection,
> +                    selection,
> +                    selectionArgs,
> +                    sortOrder);
> +        } catch(Exception e) {

Nit: space between 'catch' and '('.

I think you should only catch RemoteException here, and I think you should rethrow it as an unchecked IllegalStateException:

  } catch (RemoteException e) {
      Log.e(LOGTAG, "RemoteException accessing ContentProviderClient.", e);
      throw new IllegalStateException("RemoteException.", e);
  }

We generally prefer to crash rather than silently stop working in baffling and hard-to-detect ways.

@@ +591,5 @@
>      }
>  
>      @Override
>      public int getCount(ContentResolver cr, String database) {
> +        ContentProviderClient client = getContentProviderClient(cr);

final

@@ +619,5 @@
> +            Cursor cursor = null;
> +            try {
> +                cursor = client.query(uri, columns, constraint, null, null);
> +            } catch(Exception e) {
> +                Log.e(LOGTAG, "Error getting count.", e);

Don't muffle like this; it makes the control flow hard to see. Introduce a `query` method if you need to repeatedly turn RemoteExceptions into runtime exceptions.

(Granted, this method could do with some cleanup anyway; very stateful!)

@@ +623,5 @@
> +                Log.e(LOGTAG, "Error getting count.", e);
> +            }
> +
> +            if(cursor == null) {
> +                Log.e(LOGTAG, "Null cursor in getCount.");

This should never occur. Allow the exception to affect the control flow instead!

@@ +658,5 @@
>          }
>  
> +        try {
> +            return filterAllSites(client,
> +                    new String[]{Combined._ID,

Don't bust the indentation.

@@ +668,5 @@
> +                    limit,
> +                    null,
> +                    selection, selectionArgs);
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error filtering all sites.", e);

Same comment as above.

@@ +677,5 @@
>      @Override
>      public Cursor getTopSites(ContentResolver cr, int limit) {
>          // Filter out unvisited bookmarks and the ones that don't have real
>          // parents (e.g. pinned sites or reading list items).
> +        ContentProviderClient client = getContentProviderClient(cr);

final.

@@ +698,5 @@
> +                    AboutPages.URL_FILTER,
> +                    selection,
> +                    selectionArgs);
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error getting top sites.", e);

Same.

@@ +720,5 @@
> +                    values,
> +                    History.URL + " = ?",
> +                    new String[]{uri});
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error updating viewed history.", e);

Same.

@@ +736,5 @@
> +                    values,
> +                    History.URL + " = ?",
> +                    new String[]{uri});
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error updating history title.", e);

Same.

@@ +751,5 @@
> +                    History.VISITS + " > 0",
> +                    null,
> +                    null);
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error getting all visited history.", e);

Same.

@@ +772,5 @@
> +                    History.DATE_LAST_VISITED + " > 0",
> +                    null,
> +                    History.DATE_LAST_VISITED + " DESC");
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error getting recent history.", e);

Same.

@@ +798,5 @@
> +            client.delete(mHistoryUriWithProfile,
> +                    History.URL + " = ?",
> +                    new String[] { url });
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error removing history entry.", e);

Same.

@@ +808,5 @@
> +        ContentProviderClient client = getContentProviderClient(cr);
> +        try {
> +            client.delete(mHistoryUriWithProfile, null, null);
> +        } catch(Exception e) {
> +            Log.e(LOGTAG, "Error clearing history.", e);

Same.

@@ +816,5 @@
>      @Override
>      @RobocopTarget
>      public Cursor getBookmarksInFolder(ContentResolver cr, long folderId) {
>          final boolean addDesktopFolder;
> +        ContentProviderClient client = getContentProviderClient(cr);

final.

@@ +844,5 @@
> +                                Bookmarks.MENU_FOLDER_GUID,
> +                                Bookmarks.UNFILED_FOLDER_GUID},
> +                        null);
> +            } catch(Exception e) {
> +                Log.e(LOGTAG, "Error getting bookmarks in fake desktop folder.", e);

Same.
Attachment #8570061 - Flags: review?(rnewman) → feedback+
Reset assignee due to inactivity.
Assignee: tylertstonge → nobody
Status: ASSIGNED → NEW
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: