Adds a MemoryPressureListener to sql::Connection.

Adds a MemoryPressureListener to sql::Connection.

This will allow SQLite database to reduce their page cache memory on systems
which provide a MemoryPressureListener signal.

BUG=243769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212374

[rmcilroy, 2013-06-14 17:21:30]

As an alternative to CL:17035006 - please let me know if you think this a
sensible idea.

[Scott Hess - ex-Googler, 2013-06-14 18:01:23]

I don't like dropping the backends (your comment from other CL), but I'm
uncomfortable with having the sql/ layer implement policy decisions of this
sort.  Then we'll find some sub-system which really does want to keep the data
because <reason>, and we'll need to add setters for telling the connection about
that.  Beyond that, sql/ is an implementation detail of the sub-system, not a
requirement.

I think it would be better to have the overall sub-system implement the pressure
listener, and have sql/ implement a "trim memory usage" function.

In the trim-memory-usage function, put a TODO(shess): about using
sqlite3_db_release_memory().  Our version of SQLite doesn't have that, but the
latest version does.

[Scott Hess - ex-Googler, 2013-06-14 18:03:33]

Also, thread safety.

[bulach, 2013-06-17 10:23:27]

I think we're all in agreement here :)

summarizing:
1. Higher-up layers would have their own memory listeners (as for thread safety,
the callback is guaranteed to be called on the thread that registered it).

2. here, we could have just the "trim memory" method exposed, called by the
layers above.

3. levels: we'd need to measure how much memory can we purge with "trim memory"
versus dropping the backends.. if the difference is noticeable, then on critical
levels it should probably just go ahead and nuke the backends, whilst on
moderate levels, trim memory would be enough.

all good? :)

[rmcilroy, 2013-06-17 11:48:57]

On 2013/06/17 10:23:27, bulach wrote:
> I think we're all in agreement here :)
> 
> summarizing:
> 1. Higher-up layers would have their own memory listeners (as for thread
safety,
> the callback is guaranteed to be called on the thread that registered it).
> 
> 2. here, we could have just the "trim memory" method exposed, called by the
> layers above.
> 
> 3. levels: we'd need to measure how much memory can we purge with "trim
memory"
> versus dropping the backends.. if the difference is noticeable, then on
critical
> levels it should probably just go ahead and nuke the backends, whilst on
> moderate levels, trim memory would be enough.
> 
> all good? :)

Yes, I'm certainly in agreement with this.  I've just uploaded a new patch-set
which does the trim memory in the sql layer, and the memory listenerhigher up in
the history_backend sub-system.

[Scott Hess - ex-Googler, 2013-06-17 20:13:24]

On 2013/06/17 10:23:27, bulach wrote:
> summarizing:
> 1. Higher-up layers would have their own memory listeners (as for thread
safety,
> the callback is guaranteed to be called on the thread that registered it).

You should double-check these places and make sure that there isn't anything
interesting going on WRT threading.  I have had people ask me scary questions
about SQLite thread-safety, so while I like to think that people always use a
single thread for the DB operations, I'm not entirely convinced that they always
do in practice.

I'm overall comfortable with the direction this is taking, though.

https://codereview.chromium.org/17071007/diff/10001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/17071007/diff/10001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:776: }
This code seems to use no-brace style for single-line if() clauses like this.

https://codereview.chromium.org/17071007/diff/10001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17071007/diff/10001/sql/connection.cc#newcode232
sql/connection.cc:232: }
Style in this code is no braces for this case.  Also, whitespace line between
this and next stanza.

https://codereview.chromium.org/17071007/diff/10001/sql/connection.cc#newcode234
sql/connection.cc:234: int default_cache_size = 2000;  //
SQLITE_DEFAULT_CACHE_SIZE in sqliteLimit.h
I think that "PRAGMA cache_size = 0" gives the SQLITE_DEFAULT_CACHE_SIZE, but if
you run "PRAGMA cache_size" you will get the _current_ cache size, which will be
the value of SQLITE_DEFAULT_CACHE_SIZE if it has never been explicitly set.  I
think that would be a more reasonable way to initialize orig_cache_size.

I think it would be reasonable to use cache_size_ if available.  Technically,
the cache size is a hint, so the actual cache size could differ from the one
requested.  In that case I'm not sure if you'd be better off to halve the
requested size or halve the actual size, but I don't think it's worth getting
philosophical about it :-).

https://codereview.chromium.org/17071007/diff/10001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/17071007/diff/10001/sql/connection.h#newcode188
sql/connection.h:188: // <cache-size> for this connection.
Are you certain that this is what happens?  Last time I experimented with this,
I found that unused pages were removed by descending page number.

I think you should either call this TrimMemory() and _not_ talk about the page
cache, or call it TrimPageCache().  I think TrimMemory() would be fine, because
it would allow layering in the release call in the TODO(), and/or clearing the
statement cache.

Also, not entirely sure about "trim_fully".  We don't have enough control to
actually do it "fully".  Maybe something more like "trim_aggressively" would
make sense, or even "aggressively" since the operation is implied by the
function name.

[rmcilroy, 2013-06-18 18:22:53]

I'm pretty sure threading is fine here - HistoryBackend::Init() has a comment
saying it must be called from the history thread, so we are creating the memory
pressure listener on the same thread on which all other calls to the
HistoryBackend are made.

https://codereview.chromium.org/17071007/diff/10001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/17071007/diff/10001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:776: }
On 2013/06/17 20:13:25, shess wrote:
> This code seems to use no-brace style for single-line if() clauses like this.

Done.

https://codereview.chromium.org/17071007/diff/10001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17071007/diff/10001/sql/connection.cc#newcode232
sql/connection.cc:232: }
On 2013/06/17 20:13:25, shess wrote:
> Style in this code is no braces for this case.  Also, whitespace line between
> this and next stanza.

Done.

https://codereview.chromium.org/17071007/diff/10001/sql/connection.cc#newcode234
sql/connection.cc:234: int default_cache_size = 2000;  //
SQLITE_DEFAULT_CACHE_SIZE in sqliteLimit.h
On 2013/06/17 20:13:25, shess wrote:
> I think that "PRAGMA cache_size = 0" gives the SQLITE_DEFAULT_CACHE_SIZE, but
if
> you run "PRAGMA cache_size" you will get the _current_ cache size, which will
be
> the value of SQLITE_DEFAULT_CACHE_SIZE if it has never been explicitly set.  I
> think that would be a more reasonable way to initialize orig_cache_size.
> 
> I think it would be reasonable to use cache_size_ if available.  Technically,
> the cache size is a hint, so the actual cache size could differ from the one
> requested.  In that case I'm not sure if you'd be better off to halve the
> requested size or halve the actual size, but I don't think it's worth getting
> philosophical about it :-).

Done.

https://codereview.chromium.org/17071007/diff/10001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/17071007/diff/10001/sql/connection.h#newcode188
sql/connection.h:188: // <cache-size> for this connection.
On 2013/06/17 20:13:25, shess wrote:
> Are you certain that this is what happens?  Last time I experimented with
this,
> I found that unused pages were removed by descending page number.
> 
> I think you should either call this TrimMemory() and _not_ talk about the page
> cache, or call it TrimPageCache().  I think TrimMemory() would be fine,
because
> it would allow layering in the release call in the TODO(), and/or clearing the
> statement cache.
> 
> Also, not entirely sure about "trim_fully".  We don't have enough control to
> actually do it "fully".  Maybe something more like "trim_aggressively" would
> make sense, or even "aggressively" since the operation is implied by the
> function name.

Done.  Re. LRU order - I think this is what it is meant to be doing (it
certainly frees pages off of the pLruTail queue in pCache1.c), though whether
that is actually what it does I am not sure.  Removed mention of page cache from
comment in any case.

[rmcilroy, 2013-06-28 18:43:11]

ping?

[Scott Hess - ex-Googler, 2013-06-28 20:21:57]

LGTM with two nits.

https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc#newcode227
sql/connection.cc:227: TrimMemory(false);
I think that TrimMemory() here is contrary to the goal of Preload(). 
Admittedly, now Preload() works differently than before, but still.

Also, whoever is calling Preload() can easily enough call TrimMemory() if that
is what they want.

https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc#newcode242
sql/connection.cc:242: int shrink_cache_size = aggressively ? 1 :
(original_cache_size / 2);
As a general hygiene thing, pop this into a scope so that the statement gets
torn down.  At this point it is in a still-executing state which can interact
with later statements.  Or Step() it or Reset() it, but putting it in a scope is
the general pattern I think.

[Scott Hess - ex-Googler, 2013-06-28 22:07:24]

On 2013/06/28 20:21:57, shess wrote:
> LGTM with two nits.
> 
> https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc
> File sql/connection.cc (right):
> 
>
https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc#newcode227
> sql/connection.cc:227: TrimMemory(false);
> I think that TrimMemory() here is contrary to the goal of Preload(). 
> Admittedly, now Preload() works differently than before, but still.
> 
> Also, whoever is calling Preload() can easily enough call TrimMemory() if that
> is what they want.
> 
>
https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc#newcode242
> sql/connection.cc:242: int shrink_cache_size = aggressively ? 1 :
> (original_cache_size / 2);
> As a general hygiene thing, pop this into a scope so that the statement gets
> torn down.  At this point it is in a still-executing state which can interact
> with later statements.  Or Step() it or Reset() it, but putting it in a scope
is
> the general pattern I think.

I'm going OOT tomorrow, so if you make these changes, just go ahead and checkin.

If you wish to ignore the statement-scoping suggestion, that's fine, it's mainly
a code-smell type thing (like periodically I'll see something weird and it will
turn out to be a half-executed statement like this).

[commit-bot: I haz the power, 2013-07-01 09:54:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/17071007/22001

[rmcilroy, 2013-07-01 09:56:39]

https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc#newcode227
sql/connection.cc:227: TrimMemory(false);
On 2013/06/28 20:21:57, shess wrote:
> I think that TrimMemory() here is contrary to the goal of Preload(). 
> Admittedly, now Preload() works differently than before, but still.
> 
> Also, whoever is calling Preload() can easily enough call TrimMemory() if that
> is what they want.

Opps, this was there for testing and I thought I had removed it.  Good catch!

https://codereview.chromium.org/17071007/diff/16001/sql/connection.cc#newcode242
sql/connection.cc:242: int shrink_cache_size = aggressively ? 1 :
(original_cache_size / 2);
On 2013/06/28 20:21:57, shess wrote:
> As a general hygiene thing, pop this into a scope so that the statement gets
> torn down.  At this point it is in a still-executing state which can interact
> with later statements.  Or Step() it or Reset() it, but putting it in a scope
is
> the general pattern I think.

Done.

[commit-bot: I haz the power, 2013-07-01 10:09:41]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rmcilroy, 2013-07-01 11:19:44]

brettw: could you LGTM for history owners please.

[brettw, 2013-07-17 17:44:54]

LGTM with style nits.

https://codereview.chromium.org/17071007/diff/22001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/17071007/diff/22001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:774:
base::MemoryPressureListener::MEMORY_PRESSURE_CRITICAL;
Nit: need one more space indent.

https://codereview.chromium.org/17071007/diff/22001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17071007/diff/22001/sql/connection.cc#newcode271
sql/connection.cc:271: DLOG(WARNING) << "Could not shrink cache size: " <<
GetErrorMessage();
Nit: 2 space undent (same below).

[rmcilroy, 2013-07-18 11:15:12]

Thanks Brett.

https://codereview.chromium.org/17071007/diff/22001/chrome/browser/history/hi...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/17071007/diff/22001/chrome/browser/history/hi...
chrome/browser/history/history_backend.cc:774:
base::MemoryPressureListener::MEMORY_PRESSURE_CRITICAL;
On 2013/07/17 17:44:55, brettw wrote:
> Nit: need one more space indent.

Done.

https://codereview.chromium.org/17071007/diff/22001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17071007/diff/22001/sql/connection.cc#newcode271
sql/connection.cc:271: DLOG(WARNING) << "Could not shrink cache size: " <<
GetErrorMessage();
On 2013/07/17 17:44:55, brettw wrote:
> Nit: 2 space undent (same below).

Done.

[commit-bot: I haz the power, 2013-07-18 11:15:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/17071007/36001

[commit-bot: I haz the power, 2013-07-18 15:34:37]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-18 15:53:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/17071007/36001

[commit-bot: I haz the power, 2013-07-18 18:49:16]

Change committed as 212374