Compiled statement cache grows unbounded when not using prepared statements

[Brainn]

Originally reported on Google Code with ID 35

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

Reported by brainn on 2011-08-25 23:01:23

[Brainn]

``` 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. ```

Reported by `brainn` on 2011-08-26 04:27:51

[Igor Sereda]

``` 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 ```

Reported by `sereda` on 2011-08-26 11:02:24

[Former user]

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

Reported by [e-mail] on 2014-09-21 18:27:10 - Status changed: WontFix