Closed sbosley closed 8 years ago
I think we've resolved the first two open questions satisfactorily. We won't do any fancy locking on prepared statements or cleanup when the DB closes, as 1) 99% of the time best practice is probably to use prepared statements inside of transactions, and 2) users would need to implement code to both safely acquire and invalidate any long-lived references to prepared statements that they had kept around when the database is about to close anyways -- as we do in the fast insert case. A new onClose()
hook has been added to SquidDatabase to address use cases like this, but most users (including users of the prepareStatement()
API) will probably never need it.
Final question is whether or not to limit the number of prepared statements cached for fast insert. I realized that keeping these caches in a ThreadLocal is probably a bad idea because it could lead to memory leaks if the threads never exited, e.g. if they belonged to a thread pool. Therefore I think it'll be best to implement a different synchronization mechanism for these fast insert prepared statements so that the total number of such statements is bounded and not tied to the number of calling threads in any way.
After further thought and experimenting I think we should defer refactoring out the ThreadLocal pieces of the fast insert code to a future PR. Every non-ThreadLocal approach I tried was deadlock prone because of the way the low-level statements acquire connections to the SQLiteDatabase when being executed -- we need to bind arguments to such statements and then execute them atomically, but acquiring any lock before binding+executing the statement could lead to a deadlock condition if another thread began a transaction and then attempted to use a prepared statement in use by another thread. Keeping prepared statements thread local is the safest and easiest way for now, and we can implement a more sophisticated solution in the future.
LGTM
TL;DR: The changes in this PR give SquiDB a 50-70% performance improvement when inserting rows.
This PR contains two related enhancements: 1) A new API that allows preparing a low-level SQLite statement, which can have its arguments rebound and be executed multiple times. Such statements are represented as instances of the new
ISQLitePreparedStatement
interface, which is based on the interface of Android's SQLiteStatement class. Cross-platform support for these statements are implemented using simple wrapper classes around the platform-specific counterpart to SQLiteStatement. 2) SquidDatabase can now optionally use this API to get a massive speed increase when performing insert operations with TableModels. This is accomplished via caching a prepared insert statement for the given table and reusing it for all inserts performed on instances of that model class. Performance gains are particularly noticeable when inserting a large number of rows e.g. in a transactionThe speed boosts we get from using these prepared statements are pretty significant. I used the excellent AndroidDatabaseLibraryComparison project from Raizlabs to get benchmarks. Before this change, our insert performance was quite slow; after the change it's in the same class as the fastest libraries in that comparison (within 5-10%). We take a very slight performance hit compared to other libraries because of our various levels of indirection for the cross-platform stuff, as well as our "key-value pairs" approach to model objects, but in the benchmarks I ran the difference was something like less than 1/100th of a millisecond overhead per row inserted, which I think is probably an acceptable tradeoff :) When all's said and done, this change gives SquiDB a 50-70% speed increase for large insert transactions.
Note that right now this "fast insert" feature is hidden behind a feature flag so we can test it out a little bit more. It seems pretty solid and our test suite is all green, but I figured it was safer not to make such a big change all at once. Users can enable/experiment with the enhancements by calling
setFastInsertEnabled(true)
in their SquidDatabaseonConfigure()
.There are some questions that are still open for discussion before merging this PR. @jdkoren your feedback on these would be most helpful:
prepareStatement()
, or is this detail better left to the user? Maybe anonClosed()
hook in SquidDatabase would offer more flexibility for that.prepareStatement()
acquire the non-exclusive lock for the DB that created them when running any of theirexecute
methods? Right now it's just mentioned in documentation that locking may be necessary for long-lived prepared statements, e.g. for those created and used outside a transaction.