Status Update
Comments
ml...@google.com <ml...@google.com> #3
Thanks. Let us know when you have some API design (not looking for a doc, just commenting here is also fine, whichever is easier for you).
One tricky bit about this one is the "transactionality" of it.
e.g. we need to handle the case where we called onPopuplated but application crashed before onPopulated
could succeed. We need to re-run it in the next run.
We might do this by keeping a record in room_master_table
(there might be a better option, this is the first thing that comes to my mind)
ra...@google.com <ra...@google.com> #4
My idea is to:
1. Add RoomDatabase.Callback#onPopulated method
2. SQLiteCopyOpenHelper already has access to a list of callbacks (List<RoomDatabase.Callback>) through mDatabaseConfiguration.callbacks
3. Modify getWritableDatabase() and getReadableDatabase() so that after the file is copied I can iterate over those callbacks and call onPopulated. I would pass mDelegate.getWritableDatabase() and mDelegate.getReadableDatabase() as a parameter to onPopulated.
4. Add tests in DatabaseCallbackTest
I asked Danny about it and he said that I might need to temporarily open the DB with a vanilla open helper just to invoke onPopulated close it and then let Room re-open it through the normal path. Not sure how would I do that yet so I'm still reading and trying to get better understanding of the code.
This probably also doesn't take this "transactionality" thing into account so I need to look into it. I think I'll try to create some tests that will help me verify if it works or not.
I'm also wondering if there is a way to build the lib locally so I can test it on a sample project. Can I somehow publish it to local maven or something?
ml...@google.com <ml...@google.com> #5
For transactionality, we do actually have a table called `room_master_table` that we can use. I made a mistake on the design of that table by not having proper keys :facepalm: instead it only has 1 id column. It is possible to create a migration for it within room but for this task, we can just add another magic number (we use 42 for identity hash) to check if we did successfully call onPopuplated (and also, we need to call on populated as we propbably don't want to call it if it is not created from assets).
For testing, TestApp is probably sufficient.
If you want to build a maven repo, you can execute:
```
cd room && DIST_DIR=$(pwd)/../repo ./gradlew createDiffArchiveForAndroidxRoom
```
This is a special task that checks the versions in
ra...@google.com <ra...@google.com> #6
When it comes to naming, I was considering onPopulatedFromAssets but there is also createFromFile method in Room database builder so I think we should use something more generic. What do you think?
ml...@google.com <ml...@google.com> #7
yea makes sense. Lets go with your first suggestion for now, maybe Dany will have a better idea.
cc...@google.com <cc...@google.com> #8
I think onPopulated
is a good starting point. Maybe a more specific name could be onOpenPrepackagedDatabase
, we mention 'pre-package databases' a few in the documentation so maybe we can use that same term.
Meanwhile, let me clarify what I meant by 'opening the DB with a vanilla helper'. In SQLiteCopyOpenHelper
once delegate.getWritableDatabase()
is invoked it will actually open the DB and go through all the onCreate
, migrations and onOpen
callbacks which means if you add logic to call onPopulated
it would have already been too late. My feeling is onPopulated
should be called around the same time as onCreate
, before migrations and before onOpen
so that users get a chance to modify the pre-package DB, possibly making schemas changes to comply with Room's validation. Therefore, I think we need to temporarily open the DB just to pass it so onPopulated
, we can do this with a new FrameworkSQLiteOpenHelper
, then close the DB and let the delegate open it, which would go through the rest of Room's flow. Hope this makes sense!
ra...@google.com <ra...@google.com> #9
do you mean doing it in the copying helper? Maybe even we can let createFromAsset
builder methods to receive a callback which includes onPopulated?
ml...@google.com <ml...@google.com> #10
Yeah, doing it on copying helper, it's the most accurate place where we know a pre-packaged DB was used / copied. The callback itself can also be receive in createFromAsset
/ createFromFile
, or just a new callback in RoomDatabase.Callback
, I still think the tricky part is giving a DB in the callback itself (onPopulated(SuppoertSqliteDatabase db)
) which requires opening the DB in the middle of the 'opening sequence'.
cc...@google.com <cc...@google.com> #11
thats why i thought about scoping it to createFromFile(file, callback)
so that it would be responsibility of the createFromFile
to open the database and call the callback and then close it.
now it makes me wonder if that callback needs to be full fledged open helper callback instead of a special on populated? What if we just let them pass the regular callback there and if that callback exists, they can do whatever.
Another problem is though, we don't know the version so how are we going to open it in the first place :/.
ml...@google.com <ml...@google.com> #12
Seems reasonable to scope it and not be a full fledge callback. However, I still think it needs to be lazy called (when the DB is opened for the first time, usually on the first query done) and not when building the DB (via the builder). The callback is really only useful is it receives the DB as a param so the user gets a chance to do something, so still think we need to open the DB and close it.
By the way, we can read the DB version without opening the DB:
ra...@google.com <ra...@google.com>
ap...@google.com <ap...@google.com> #13
But that means that we would be adding 2 new methods to the builder - createFromAsset/createFromFile but with additional parameter, right? So that we don't break backwards compatibility. I'll play with this ideas tomorrow.
But what about "transactionality"? Does putting this in copying helper make any difference? Do you think we still should use Room master table to detect if this new onPopulated callback should be called or not?
Description
When you enable method tracing for benchmarks, it's currently started with
startActivityAndWait
and it's disabled after the benchmark finishes.Method tracing collects huge amount of data which might result in big trace file and might take long due to its nature to slow down the app's performance.
This causes problems for scenarios navigating to a certain screen and starting collection from there. For example start the app, log in, navigate to a screen and scroll some content. The interested part is only scrolling the content.
There might be several options how this could work:
Enable method traces for
measureBlock
. This would be intuitive with when benchmarks actually record data.Have API to dynamically enable/disable method tracing. This might be helpful to record only part of the measure block. This could result in more polluted API though. What if you don't pass method androidx.benchmark.profiling.mode=MethodTracing`, but call the API?