Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Overhead of insert notifications #15

Closed
johnjohndoe opened this issue Feb 4, 2014 · 12 comments
Closed

Overhead of insert notifications #15

johnjohndoe opened this issue Feb 4, 2014 · 12 comments

Comments

@johnjohndoe
Copy link
Contributor

I noticed that multiple notifications are sent when bulkInsert is used. I figured out this because you iterating each ContentValue and run an individual transaction insert on the item.

@Override
public int bulkInsert(Uri uri, ContentValues[] values) {
    int numValues = values.length;
    SQLiteDatabase mDb = mOpenHelper.getWritableDatabase();
    mDb.beginTransactionWithListener(this);
    try {
        for (int i = 0; i < numValues; i++) {
            Uri result = insertInTransaction(uri, values[i]);
            if (result != null) {
                mNotifyChange = true;
            }
            mDb.yieldIfContendedSafely();
        }
        mDb.setTransactionSuccessful();
    } finally {
        mDb.endTransaction();
    }

    onEndTransaction();
    return numValues;
}

I cannot see the reason why you are using insert in favor of bulkInsert. Could you please clarify?

@charroch
Copy link
Member

charroch commented Feb 4, 2014

It is a rather old piece of code written as I remember with transactions in mind. Could probably be updated for the better

@charroch
Copy link
Member

charroch commented Feb 4, 2014

We should probably assign performance testing on the different methods of insertion.

@johnjohndoe
Copy link
Contributor Author

I definitely recommend using native bulkInsert. It is a huge performance improvement. I learnt this myself in an older project.

@devisnik
Copy link
Contributor

devisnik commented Feb 5, 2014

Definitely something we have to look into. 👍

@mr-archano
Copy link
Contributor

I think that moving to native bulkInsert can have its drawbacks: delegating the simple insert we do update or insert under the hood, while using the bulkInsert we could not provide the same feature (in my understanding at least).

@charroch
Copy link
Member

charroch commented Feb 5, 2014

yes but upsert is also slow. We should give abilities to insert in different ways. Even to an extreme of getting the DB from contentproviderclient and using the fastest bind method.

@charroch
Copy link
Member

charroch commented Feb 5, 2014

but for that I rather be pragmatic and have the setup to test the performances.

@johnjohndoe
Copy link
Contributor Author

Another workaround would be to queue and squash the notifications which are sent because of the iteration.

@orangeman
Copy link
Contributor

regardless of any performance improvements bulkInsert should only notify content observers once.
That is semantically what it is supposed to do (and a relatively small change). Do we agree on that?

@charroch
Copy link
Member

This should definitely be looked into but I can see some reasoning to have it notified in a rather 'frequent' way. Data set change should not have knowledge about implementation of the insert/update. For instance, if we have a 10min write, you could be interested in updating the UI while the insert happens. Now it s either the activity or the content provider that would be responsible for this.

Lets start by doing just one notify URI per bulk insert and then potentially adapt. We could do so by appending the params to the URI. i.e. content://test.com/table?withNotifyEvery=10

@orangeman
Copy link
Contributor

like that idea ..table?notifyEvery=42
gives fine grained control to the user 👍

@juankysoriano
Copy link
Contributor

Closing as this repository is no longer under maintenance and it's about to be archived

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants