Compiled statement cache grows unbounded when not using prepared statements

[GoogleCodeExporter]

Here is a nonsensical code example I just made up:

int getQuantity(int orderId) {
  SQLiteStatement st = db.prepare("SELECT quantity FROM orders WHERE order_id = " + orderId);
    try {
      while (st.step()) {
        return st.columnLong(0);
      }
    } finally {
      st.dispose();
    }
}

for(int orderId = 0; orderId <= 1000000; orderId++) {
  int quantity = getQuantity(orderId);
  ...
}

Every time getQuantity is called, a new statement is prepared and disposed.  
The problem is that when each statement is disposed, since each statement is 
different, a new entry is added to SqliteConnection.myStatementCache.  This 
will grow absolutely unbounded until it causes a Java Heap Space exception or 
until the connection is closed.

Now, I realize this code is bad:
1.  Using the same prepared statement would be much more efficient.
2.  In general, you shouldn't concatenate your own strings to form SQL, you 
should use bindings.

However, I would not expect it to use 100MB of memory and cause a Java Heap 
Space exception.

So here are my proposed fixes:
1.  Get rid of the statement cache.  Do we really need a statement cache?  Is 
this just to help people out that don't want to reuse their prepared 
statements?  I would have just expected that if I call .prepare(...) multiple 
times, that I would actually have to prepare again.  Would people really feel 
the performance hit if the cache was removed?

2.  Set a limit on the statement cache.  What should the limit be?  How would 
you pick one to remove from the cache when it's full?

I'll write a patch if we get some discussion going on what needs to be done.

Thanks,
Brian Vincent

Original issue reported on code.google.com by bra...@gmail.com on 25 Aug 2011 at 11:01

[GoogleCodeExporter]

Whoops.  I was just looking around your issues and noticed issue 32.  I promise 
I searched beforehand!

I actually didn't know about the boolean on prepare that allows me to specify 
that I don't want the statement cached.  That's useful.

Anyway, I'd still maintain that the cache is an unexpected feature, and at 
least needs a limit.

Original comment by bra...@gmail.com on 26 Aug 2011 at 4:27

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Brian, thanks for posting this issue.

I see nothing terrible in OutOfMemoryError and 100MB of used memory - it 
allowed you to discover caching in sqlite4java and the additional option to 
prepare method. :) Anyone can easily write code, using any library or not, that 
would consume all of the memory and terminate. 

I think we could have a bounded LRU cache for the statements, but I doubt its 
usefulness. Imagine it were there, and your application wouldn't terminate 
because out of memory - but because you didn't know about the caching, it would 
still consume, say, 50MB constantly. Not a good thing. Unbounded cache is more 
fail-fast.

What makes sense to me is that probably most users don't need this caching. 
There is benefit in caching the results of prepare(), but it is likely to be 
negligible in the context of most apps. The library was created with really 
strong performance requirements in mind -- the SQLParts class, for example, is 
intended to avoid concatenating Strings when creating generated SQL, to produce 
less garbage.

I think we can add a SQLite.setCacheStatementByDefault() method to govern the 
call to prepare() without explicit cache parameter, and have the corresponding 
flag set to false initially. This would not be backwards-compatible with the 
current version though - current users would have to call 
SQLite.setCacheStatementByDefault(true) to revert to the old version behavior.

Thoughts?

Cheers
Igor

Original comment by ser...@gmail.com on 26 Aug 2011 at 11:02

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

At the moment, we have decided not to go ahead with any caching improvements, 
given the existing method parameter. Should there be sufficient demand for this 
feature, the issue may be reopened.

Thanks,
Igor

Original comment by ser...@almworks.com on 21 Sep 2014 at 6:27

• Changed state: WontFix
• Added labels: ****
• Removed labels: ****