721352 - Add support for batch operations in LocalDB

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Right now, LocalDB runs each insert/update command in its own transaction. That is likely to have a negative performance impact when migrating tons of data from Places DB. We need to add support for batch operations in LocalDB's ContentProvider so that we can run all commands in one single transaction to minimize the number of times we have to touch the disk.

Comment 1

[Doug Turner (:dougt)]

Lucas - how big of a win is this?  Is this something we want for our first release of native?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Attachment: Test hack: Use transactions for profile migration.

I made this hack to get an idea what we can get from this. It drops Profile Migration on my testcase from 60s to 30s. Given that not all of that is database I/O, it's more than a 2x speedup.

This would benefit profile migration and the initial sync.

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> Created attachment 599933 [details] [diff] [review]
> Test hack: Use transactions for profile migration.
> 
> I made this hack to get an idea what we can get from this. It drops Profile
> Migration on my testcase from 60s to 30s. Given that not all of that is
> database I/O, it's more than a 2x speedup.
> 
> This would benefit profile migration and the initial sync.

Thanks for the data, gcp! I was about to reply dougt with "we need some preliminary data to know if it's worth the effort for Beta" :-)

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Implement transactions in batch operations.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Make Profile Migration use batch ops.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 603689 [details] [diff] [review]
Patch 1. Implement transactions in batch operations.

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

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1556,5 @@
> +        if (numOperations >= 1) {
> +            // We only have 1 database for all Uri's that we can get
> +            db = getWritableDatabase(operations.get(0).getUri());
> +        } else {
> +            return results;

Hmm, shouldn't it return null in that case? You should probably only create results with "new ContentProviderResult[numOperations];" if numOperations is > 0 I guess.

@@ +1559,5 @@
> +        } else {
> +            return results;
> +        }
> +
> +        db.beginTransaction();

How does that beginTransaction() here play with the beginTransaction() called on insert/update/delete methods in the content provider? It seems to me that you should not call beginTransaction/endTransaction on insert/update/delete methods when running inside a batch. Android's ContentProvider has code to handle that btw.

@@ +1571,5 @@
> +                results[i] = new ContentProviderResult(0);
> +                failures = true;
> +                // http://www.sqlite.org/lang_conflict.html
> +                // Note that we need a new transaction, subsequent operations
> +                // on this one will fail (we're in ABORT, which isn't IGNORE).

Ugh...

@@ +1572,5 @@
> +                failures = true;
> +                // http://www.sqlite.org/lang_conflict.html
> +                // Note that we need a new transaction, subsequent operations
> +                // on this one will fail (we're in ABORT, which isn't IGNORE).
> +                db.setTransactionSuccessful();

Hmm, why are you setting transaction as successful when it has failed? Also, why not just throws an exception here straight away instead of running all operations then throwing?

@@ +1590,5 @@
> +        return results;
> +    }
> +
> +    @Override
> +    public int bulkInsert(Uri uri, ContentValues[] values) {

Early return if values is null? Just in case...

@@ +1600,5 @@
> +        db.beginTransaction();
> +
> +        for (int i = 0; i < numValues; i++) {
> +            try {
> +                insert(uri, values[i]);

I guess what you want here is to call insertInTransaction()?

@@ +1605,5 @@
> +                successes++;
> +            } catch (SQLException e) {
> +                Log.w(LOGTAG, "SQLite Exception during bulkInsert: ", e);
> +                // Restart the transaction
> +                db.setTransactionSuccessful();

Isn't endTransaction() enough here?

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 603690 [details] [diff] [review]
Patch 2. Make Profile Migration use batch ops.

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

Nice. This code seriously needs tests. Please file a follow-up for that.

::: mobile/android/base/ProfileMigrator.java
@@ +316,3 @@
>                  values.put(History.DATE_LAST_VISITED, date);
>  
> +                if (cursor != null && cursor.moveToFirst()) {

What are the cases where cursor is null?

@@ +545,5 @@
> +                                       Bookmarks.URL + " = ?",
> +                                       new String[] { url },
> +                                       null);
> +
> +                    if (cursor != null && cursor.moveToFirst()) {

In what cases cursor is null?

Comment 8

[Lucas Rocha (:lucasr)]

I'm in the process of adding test infra for browser provider (see bug 734177). Please file a follow-up bug to write tests for batch operations in browser provider.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>Hmm, shouldn't it return null in that case?

No, not according to the documentation. And not according to the non-overloaded implementation in Android itself.

>How does that beginTransaction() here play with the beginTransaction() called on 
>insert/update/delete methods in the content provider? It seems to me that you 
>should not call beginTransaction/endTransaction on insert/update/delete methods 
>when running inside a batch. Android's ContentProvider has code to handle that btw.

http://developer.android.com/reference/android/database/sqlite/SQLiteDatabase.html#beginTransaction%28%29

Transactions can be nested and they behave as you'd expect. (This is an Android extension btw, normal SQLite itself doesn't do this!)

>Hmm, why are you setting transaction as successful when it has failed? Also, 
>why not just throws an exception here straight away instead of running all 
>operations then throwing?

1) Because everything before it will work.
2) Because it's allowed by the API of applyBatch: "If any of the calls fail, it is up to the implementation how many of the others take effect."
3) Because that's the most sane way to behave for the users of this (Sync, Profile Migration), especially with bug 716729. If that bug is fixed we might consider doing it differently, but I see no such requirement API-wise. The caller can see what succeeded and what not in the result returned, and there'll be an exception thrown if not everything succeeded.

If we don't do this, then you'd have to bisect which operation in your batch is the one throwing, and then take it out before trying again. This makes the entire API useless.

>I guess what you want here is to call insertInTransaction()?

Yes, that looks like an easy win.

>Isn't endTransaction() enough here?

No, that will roll back everything before the failed insert as well, which isn't what we want.

>Ugh...

I agree this sucks completely (I obviously didn't discover it from reading the docs :-P). API-level 8 adds the possibility to set the conflict resolution, but we'd prefer to be on API-level 7, and in any case that new API would require applyBatch to dissect exactly what kind of operation is being passed down. That isn't a simplification. For bulkInsert the situation is slightly better.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 603689 [details] [diff] [review]
Patch 1. Implement transactions in batch operations.

I got enough clarification to give a r+. Gian, maybe add more background info as comments in the code? That will help anyone changing this code in the future.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Implement transactions in batch operations.

Incorporated review comments.

I found a bug: the Android documentation doesn't say anything about bulkInsert being allowed to continue after an error (unlike applyBatch). The return value (just 1 number) also doesn't allow identifying exactly what failed, and its not defined to throw OperationApplicationException (also unlike applyBatch). Conclusion: it should do the simple thing and rollback + pass any exceptions. Simplified the code.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f6f98dc9e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc1574aa91e

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/08f6f98dc9e7
https://hg.mozilla.org/mozilla-central/rev/2bc1574aa91e