From 9504c70f8862f5ffc55b07bc374e0b18b78a2dc6 Mon Sep 17 00:00:00 2001 From: Vasu Nori Date: Fri, 23 Apr 2010 01:04:31 -0700 Subject: [PATCH] fix a bug introduced when prepared-statement cache was made LRU-based 1. when an entry is pushed out of cache, it should be released if it is not in use by any thread. This didn't have to be done when cache was NOT LRU because the object either got into the cache while the caller had a reference to it or it didn't. if it didn't get into cache (because cache is full), the caller's close() released the object. But in LRU cache, an object could get pushed out of cache due to LRU policy and if it is NOT in use by any thread at the time it was pushed out of cache, then it got GC'ed - which caused warnings to be pronted in the log. 2. also delete some unused methods in SQLiteDatabase. Change-Id: I7831952647d3a057342bcc8ac186a6a26eb58f33 --- .../database/sqlite/SQLiteCompiledSql.java | 4 ++ .../database/sqlite/SQLiteDatabase.java | 61 ++++++------------- .../database/sqlite/SQLiteProgram.java | 2 +- .../android/database/DatabaseGeneralTest.java | 46 +++++++++++++- 4 files changed, 67 insertions(+), 46 deletions(-) diff --git a/core/java/android/database/sqlite/SQLiteCompiledSql.java b/core/java/android/database/sqlite/SQLiteCompiledSql.java index 25aa9b378554a..d5c6ad15882cd 100644 --- a/core/java/android/database/sqlite/SQLiteCompiledSql.java +++ b/core/java/android/database/sqlite/SQLiteCompiledSql.java @@ -134,6 +134,10 @@ import android.util.Log; mInUse = false; } + /* package */ synchronized boolean isInUse() { + return mInUse; + } + /** * Make sure that the native resource is cleaned up. */ diff --git a/core/java/android/database/sqlite/SQLiteDatabase.java b/core/java/android/database/sqlite/SQLiteDatabase.java index 0b7e01da95ab1..0a7eac2d95802 100644 --- a/core/java/android/database/sqlite/SQLiteDatabase.java +++ b/core/java/android/database/sqlite/SQLiteDatabase.java @@ -263,7 +263,7 @@ public class SQLiteDatabase extends SQLiteClosable { * invoked. * * this cache has an upper limit of mMaxSqlCacheSize (settable by calling the method - * (@link setMaxCacheSize(int)}). + * (@link setMaxSqlCacheSize(int)}). */ // default statement-cache size per database connection ( = instance of this class) private int mMaxSqlCacheSize = 25; @@ -271,7 +271,24 @@ public class SQLiteDatabase extends SQLiteClosable { new LinkedHashMap(mMaxSqlCacheSize + 1, 0.75f, true) { @Override public boolean removeEldestEntry(Map.Entry eldest) { - return this.size() > mMaxSqlCacheSize; + // eldest = least-recently used entry + // if it needs to be removed to accommodate a new entry, + // close {@link SQLiteCompiledSql} represented by this entry, if not in use + // and then let it be removed from the Map. + synchronized(mCompiledQueries) { // probably not necessary, but can't hurt + if (this.size() <= mMaxSqlCacheSize) { + // cache is not full. nothing needs to be removed + 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(); + } + // return true, so that this entry is removed automatically by the caller. + return true; + } } }; /** @@ -2006,10 +2023,8 @@ public class SQLiteDatabase extends SQLiteClosable { mCompiledQueries.size() + "|" + sql); } } - return; } - private void deallocCachedSqlStatements() { synchronized (mCompiledQueries) { for (SQLiteCompiledSql compiledSql : mCompiledQueries.values()) { @@ -2044,44 +2059,6 @@ public class SQLiteDatabase extends SQLiteClosable { return compiledStatement; } - /** - * returns true if the given sql is cached in compiled-sql cache. - * @hide - */ - public boolean isInCompiledSqlCache(String sql) { - synchronized(mCompiledQueries) { - return mCompiledQueries.containsKey(sql); - } - } - - /** - * purges the given sql from the compiled-sql cache. - * @hide - */ - public void purgeFromCompiledSqlCache(String sql) { - synchronized(mCompiledQueries) { - mCompiledQueries.remove(sql); - } - } - - /** - * remove everything from the compiled sql cache - * @hide - */ - public void resetCompiledSqlCache() { - synchronized(mCompiledQueries) { - mCompiledQueries.clear(); - } - } - - /** - * return the current maxCacheSqlCacheSize - * @hide - */ - public synchronized int getMaxSqlCacheSize() { - return mMaxSqlCacheSize; - } - /** * set the max size of the prepared-statement cache for this database. * (size of the cache = number of compiled-sql-statements stored in the cache). diff --git a/core/java/android/database/sqlite/SQLiteProgram.java b/core/java/android/database/sqlite/SQLiteProgram.java index 89a5f0d1e9837..a9c1ac6310a26 100644 --- a/core/java/android/database/sqlite/SQLiteProgram.java +++ b/core/java/android/database/sqlite/SQLiteProgram.java @@ -147,7 +147,7 @@ public abstract class SQLiteProgram extends SQLiteClosable { * @return a unique identifier for this program */ public final int getUniqueId() { - return nStatement; + return (mCompiledSql != null) ? mCompiledSql.nStatement : 0; } /* package */ String getSqlString() { diff --git a/core/tests/coretests/src/android/database/DatabaseGeneralTest.java b/core/tests/coretests/src/android/database/DatabaseGeneralTest.java index 49bd4e8ad5356..34434d2434aff 100644 --- a/core/tests/coretests/src/android/database/DatabaseGeneralTest.java +++ b/core/tests/coretests/src/android/database/DatabaseGeneralTest.java @@ -33,6 +33,7 @@ import android.util.Log; import junit.framework.Assert; import java.io.File; +import java.util.ArrayList; import java.util.Arrays; import java.util.Locale; @@ -519,13 +520,13 @@ public class DatabaseGeneralTest extends AndroidTestCase implements PerformanceT assertEquals(1, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("don't forget to handled 's", c.getString(1)); - c.close(); + c.deactivate(); // make sure code should checking null string properly so that // it won't crash try { mDatabase.query("test", new String[]{"_id"}, - "_id=?", null, null, null, null); + "_id=?", new String[]{null}, null, null, null); fail("expected exception not thrown"); } catch (IllegalArgumentException e) { // expected @@ -1136,5 +1137,44 @@ public class DatabaseGeneralTest extends AndroidTestCase implements PerformanceT } } } - } + } + + @SmallTest + public void testLruCachingOfSqliteCompiledSqlObjs() { + mDatabase.execSQL("CREATE TABLE test (i int, j int);"); + mDatabase.execSQL("insert into test values(1,1);"); + // set cache size + int N = SQLiteDatabase.MAX_SQL_CACHE_SIZE; + mDatabase.setMaxSqlCacheSize(N); + + // do N+1 queries - and when the 0th entry is removed from LRU cache due to the + // insertion of (N+1)th entry, make sure 0th entry is closed + ArrayList stmtObjs = new ArrayList(); + for (int i = 0; i < N+1; i++) { + SQLiteStatement c = mDatabase.compileStatement("select * from test where i = " + i); + c.close(); + stmtObjs.add(i, c); + } + + assertEquals(0, stmtObjs.get(0).getUniqueId()); + for (int i = 1; i < N+1; i++) { + assertTrue(stmtObjs.get(i).getUniqueId() > 0); + } + } + + @SmallTest + public void testSetMaxCahesize() { + mDatabase.execSQL("CREATE TABLE test (i int, j int);"); + mDatabase.execSQL("insert into test values(1,1);"); + // set cache size + int N = SQLiteDatabase.MAX_SQL_CACHE_SIZE; + mDatabase.setMaxSqlCacheSize(N); + + // try reduce cachesize + try { + mDatabase.setMaxSqlCacheSize(1); + } catch (IllegalStateException e) { + assertTrue(e.getMessage().contains("cannot set cacheSize to a value less than")); + } + } }