fix compiled statement bugs, synchronization bugs
1. when LRU cache wants to remove the eldest entry, it may be finalizing
a statement that is still in use
2. a couple of synchronization issues.
3. a bit code refactoring in SQLiteCompiledSql
4. remove a bunch of unsed debug code from SQLiteDatabase
Change-Id: I45454dc2dbadd7d8842ba77dd2b0e9ff138ec6f4
diff --git a/core/java/android/database/sqlite/SQLiteCompiledSql.java b/core/java/android/database/sqlite/SQLiteCompiledSql.java
index 0b5a4df..588384b 100644
--- a/core/java/android/database/sqlite/SQLiteCompiledSql.java
+++ b/core/java/android/database/sqlite/SQLiteCompiledSql.java
@@ -31,12 +31,12 @@
private static final String TAG = "SQLiteCompiledSql";
/** The database this program is compiled against. */
- /* package */ SQLiteDatabase mDatabase;
+ /* package */ final SQLiteDatabase mDatabase;
/**
* Native linkage, do not modify. This comes from the database.
*/
- /* package */ int nHandle = 0;
+ /* package */ final int nHandle;
/**
* Native linkage, do not modify. When non-0 this holds a reference to a valid
@@ -54,38 +54,13 @@
private boolean mInUse = false;
/* package */ SQLiteCompiledSql(SQLiteDatabase db, String sql) {
- if (!db.isOpen()) {
- throw new IllegalStateException("database " + db.getPath() + " already closed");
- }
+ db.verifyDbIsOpen();
+ db.verifyLockOwner();
mDatabase = db;
mSqlStmt = sql;
mStackTrace = new DatabaseObjectNotClosedException().fillInStackTrace();
- this.nHandle = db.mNativeHandle;
- compile(sql, true);
- }
-
- /**
- * Compiles the given SQL into a SQLite byte code program using sqlite3_prepare_v2(). If
- * this method has been called previously without a call to close and forCompilation is set
- * to false the previous compilation will be used. Setting forceCompilation to true will
- * always re-compile the program and should be done if you pass differing SQL strings to this
- * method.
- *
- * <P>Note: this method acquires the database lock.</P>
- *
- * @param sql the SQL string to compile
- * @param forceCompilation forces the SQL to be recompiled in the event that there is an
- * existing compiled SQL program already around
- */
- private void compile(String sql, boolean forceCompilation) {
- mDatabase.verifyLockOwner();
- // Only compile if we don't have a valid statement already or the caller has
- // explicitly requested a recompile.
- if (forceCompilation) {
- // Note that the native_compile() takes care of destroying any previously
- // existing programs before it compiles.
- native_compile(sql);
- }
+ nHandle = db.mNativeHandle;
+ native_compile(sql);
}
/* package */ void releaseSqlStatement() {
@@ -102,7 +77,7 @@
*/
/* package */ synchronized boolean acquire() {
if (mInUse) {
- // someone already has acquired it.
+ // it is already in use.
return false;
}
mInUse = true;
@@ -117,6 +92,13 @@
return mInUse;
}
+ /* package */ synchronized void releaseIfNotInUse() {
+ // if it is not in use, release its memory from the database
+ if (!isInUse()) {
+ releaseSqlStatement();
+ }
+ }
+
/**
* Make sure that the native resource is cleaned up.
*/
@@ -125,10 +107,15 @@
try {
if (nStatement == 0) return;
// finalizer should NEVER get called
- int len = mSqlStmt.length();
- Log.w(TAG, "Releasing statement in a finalizer. Please ensure " +
- "that you explicitly call close() on your cursor: " +
- mSqlStmt.substring(0, (len > 100) ? 100 : len), mStackTrace);
+ // but if the database itself is not closed and is GC'ed, then
+ // all sub-objects attached to the database could end up getting GC'ed too.
+ // in that case, don't print any warning.
+ if (!mInUse) {
+ int len = mSqlStmt.length();
+ Log.w(TAG, "Releasing statement in a finalizer. Please ensure " +
+ "that you explicitly call close() on your cursor: " +
+ mSqlStmt.substring(0, (len > 100) ? 100 : len), mStackTrace);
+ }
releaseSqlStatement();
} finally {
super.finalize();
@@ -140,6 +127,8 @@
StringBuilder buff = new StringBuilder();
buff.append(" nStatement=");
buff.append(nStatement);
+ buff.append(", mInUse=");
+ buff.append(mInUse);
buff.append(", db=");
buff.append(mDatabase.getPath());
buff.append(", db_connectionNum=");
diff --git a/core/java/android/database/sqlite/SQLiteDatabase.java b/core/java/android/database/sqlite/SQLiteDatabase.java
index 4200701..3df1790 100644
--- a/core/java/android/database/sqlite/SQLiteDatabase.java
+++ b/core/java/android/database/sqlite/SQLiteDatabase.java
@@ -293,11 +293,7 @@
return false;
}
// cache is full. eldest will be removed.
- SQLiteCompiledSql entry = eldest.getValue();
- if (!entry.isInUse()) {
- // this {@link SQLiteCompiledSql} is not in use. release it.
- entry.releaseSqlStatement();
- }
+ eldest.getValue().releaseIfNotInUse();
// return true, so that this entry is removed automatically by the caller.
return true;
}
@@ -308,8 +304,7 @@
* SQL statement & schema.
*/
public static final int MAX_SQL_CACHE_SIZE = 100;
- private int mCacheFullWarnings;
- private static final int MAX_WARNINGS_ON_CACHESIZE_CONDITION = 1;
+ private boolean mCacheFullWarning;
/** maintain stats about number of cache hits and misses */
private int mNumCacheHits;
@@ -2092,12 +2087,6 @@
}
}
- /*
- * ============================================================================
- *
- * The following methods deal with compiled-sql cache
- * ============================================================================
- */
/**
* Adds the given SQL and its compiled-statement-id-returned-by-sqlite to the
* cache of compiledQueries attached to 'this'.
@@ -2113,26 +2102,21 @@
return;
}
- if (mCompiledQueries.size() == mMaxSqlCacheSize) {
+ if (!isCacheFullWarningLogged() && mCompiledQueries.size() == mMaxSqlCacheSize) {
/*
* cache size of {@link #mMaxSqlCacheSize} is not enough for this app.
* log a warning.
* chances are it is NOT using ? for bindargs - or cachesize is too small.
*/
- if (++mCacheFullWarnings == MAX_WARNINGS_ON_CACHESIZE_CONDITION) {
- Log.w(TAG, "Reached MAX size for compiled-sql statement cache for database " +
- getPath() + ". Use setMaxSqlCacheSize() to increase cachesize. ");
- }
+ Log.w(TAG, "Reached MAX size for compiled-sql statement cache for database " +
+ getPath() + ". Use setMaxSqlCacheSize() to increase cachesize. ");
+ setCacheFullWarningLogged();
}
/* add the given SQLiteCompiledSql compiledStatement to cache.
* no need to worry about the cache size - because {@link #mCompiledQueries}
* self-limits its size to {@link #mMaxSqlCacheSize}.
*/
mCompiledQueries.put(sql, compiledStatement);
- if (SQLiteDebug.DEBUG_SQL_CACHE) {
- Log.v(TAG, "|adding_sql_to_cache|" + getPath() + "|" +
- mCompiledQueries.size() + "|" + sql);
- }
}
}
@@ -2150,24 +2134,13 @@
* From the compiledQueries cache, returns the compiled-statement-id for the given SQL.
* Returns null, if not found in the cache.
*/
- /* package */ SQLiteCompiledSql getCompiledStatementForSql(String sql) {
- SQLiteCompiledSql compiledStatement = null;
- boolean cacheHit;
- synchronized(mCompiledQueries) {
- cacheHit = (compiledStatement = mCompiledQueries.get(sql)) != null;
- }
- if (cacheHit) {
- mNumCacheHits++;
- } else {
+ /* package */ synchronized SQLiteCompiledSql getCompiledStatementForSql(String sql) {
+ SQLiteCompiledSql compiledStatement = mCompiledQueries.get(sql);
+ if (compiledStatement == null) {
mNumCacheMisses++;
+ return null;
}
-
- if (SQLiteDebug.DEBUG_SQL_CACHE) {
- Log.v(TAG, "|cache_stats|" +
- getPath() + "|" + mCompiledQueries.size() +
- "|" + mNumCacheHits + "|" + mNumCacheMisses +
- "|" + cacheHit + "|" + sql);
- }
+ mNumCacheHits++;
return compiledStatement;
}
@@ -2203,6 +2176,26 @@
}
}
+ private synchronized boolean isCacheFullWarningLogged() {
+ return mCacheFullWarning;
+ }
+
+ private synchronized void setCacheFullWarningLogged() {
+ mCacheFullWarning = true;
+ }
+
+ private synchronized int getCacheHitNum() {
+ return mNumCacheHits;
+ }
+
+ private synchronized int getCacheMissNum() {
+ return mNumCacheMisses;
+ }
+
+ private synchronized int getCachesize() {
+ return mCompiledQueries.size();
+ }
+
/* package */ void finalizeStatementLater(int id) {
if (!isOpen()) {
// database already closed. this statement will already have been finalized.
@@ -2484,16 +2477,16 @@
}
if (pageCount > 0) {
dbStatsList.add(new DbStats(dbName, pageCount, db.getPageSize(),
- lookasideUsed, db.mNumCacheHits, db.mNumCacheMisses,
- db.mCompiledQueries.size()));
+ lookasideUsed, db.getCacheHitNum(), db.getCacheMissNum(),
+ db.getCachesize()));
}
}
// if there are pooled connections, return the cache stats for them also.
if (db.mConnectionPool != null) {
for (SQLiteDatabase pDb : db.mConnectionPool.getConnectionList()) {
dbStatsList.add(new DbStats("(pooled # " + pDb.mConnectionNum + ") "
- + lastnode, 0, 0, 0, pDb.mNumCacheHits, pDb.mNumCacheMisses,
- pDb.mCompiledQueries.size()));
+ + lastnode, 0, 0, 0, pDb.getCacheHitNum(),
+ pDb.getCacheMissNum(), pDb.getCachesize()));
}
}
} catch (SQLiteException e) {