[WIP] histroy: Adjust cache size OnMemoryStateChange()

[WIP] histroy: Adjust cache size OnMemoryStateChange()

OnMemoryStateChange() is a stateful API so it would be
better to adjust cache size rather than trying to trim
the cache memory every time OnMemoryStateChange() is
called. This CL adds sql::Connection::UpdateCacheSize()
to provide a way to adjust cache size based on the
memory state.

BUG=639700

[bashi, 2016-09-01 10:19:01]

bashi@chromium.org changed reviewers:
+ chrisha@chromium.org, haraken@chromium.org, shess@chromium.org,
sky@chromium.org

[bashi, 2016-09-01 10:19:02]

sky@, shess@: What do you think about this change?

This is a follow-up CL of [1]. Chris suggested that it would be better to have a
way to adjust cache size[2], and I think it is a good idea.

The design doc of memory coordinator is at [3].

If this looks good I'll update the CL to do the same thing for
ThumbnailDatabase.

[1] https://codereview.chromium.org/2261073002/
[2] https://codereview.chromium.org/2261073002/#msg47
[3]
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...

[Scott Hess - ex-Googler, 2016-09-01 17:55:15]

As of https://codereview.chromium.org/1382283003 , the SQLite caches should
mostly be empty outside of active transactions.  I don't see anything in the
linked bug or design doc that indicates specific metrics around SQLite memory
usage - do you have evidence that the cache is actually taking up a lot of
memory?

[The above CL keeps the cache mostly-clear by using mmap.  I _believe_ that at
this point most platforms support mmap.  iOS didn't for awhile, but I think
current versions do.  If this is untrue, I have a suspicion that making the
cache-clearing unconditional wouldn't actually affect performance much.]

[haraken, 2016-09-01 18:03:17]

On 2016/09/01 17:55:15, Scott Hess wrote:
> As of https://codereview.chromium.org/1382283003 , the SQLite caches should
> mostly be empty outside of active transactions.  I don't see anything in the
> linked bug or design doc that indicates specific metrics around SQLite memory
> usage - do you have evidence that the cache is actually taking up a lot of
> memory?
> 
> [The above CL keeps the cache mostly-clear by using mmap.  I _believe_ that at
> this point most platforms support mmap.  iOS didn't for awhile, but I think
> current versions do.  If this is untrue, I have a suspicion that making the
> cache-clearing unconditional wouldn't actually affect performance much.]

Basically what we're doing here is to migrate existing MemoryPressureListener's
implementation to the new MemoryCoordinator's API :)

HistoryBackend::OnMemoryPressure is purging the cache, so we're trying to do the
same thing in MemoryCoordinator as well.

[chrisha, 2016-09-01 18:17:30]

On 2016/09/01 18:03:17, haraken wrote:
> On 2016/09/01 17:55:15, Scott Hess wrote:
> > As of https://codereview.chromium.org/1382283003 , the SQLite caches should
> > mostly be empty outside of active transactions.  I don't see anything in the
> > linked bug or design doc that indicates specific metrics around SQLite
memory
> > usage - do you have evidence that the cache is actually taking up a lot of
> > memory?
> > 
> > [The above CL keeps the cache mostly-clear by using mmap.  I _believe_ that
at
> > this point most platforms support mmap.  iOS didn't for awhile, but I think
> > current versions do.  If this is untrue, I have a suspicion that making the
> > cache-clearing unconditional wouldn't actually affect performance much.]
> 
> Basically what we're doing here is to migrate existing
MemoryPressureListener's
> implementation to the new MemoryCoordinator's API :)
> 
> HistoryBackend::OnMemoryPressure is purging the cache, so we're trying to do
the
> same thing in MemoryCoordinator as well.

We don't have any up to date data for the value of individual MPLs right now,
and are simply migrating them one at a time.

A problem in general with MPL is "jank", in that things are asked to trim and
then they immediately start growing back to using as much memory as previously.
MC fixes this by turning notifications into states, and expecting clients to
make persistent changes. This CL is an attempt to do that for the History DB.

If the change to using a memory mapped DB obviates this, then maybe we no longer
need an MPL client here either?

Regardless, it should be relatively easy to instrument this to see how much if
any memory the cache is typically using.

[Scott Hess - ex-Googler, 2016-09-01 18:29:04]

On 2016/09/01 18:17:30, chrisha (slow) wrote:
> On 2016/09/01 18:03:17, haraken wrote:
> > On 2016/09/01 17:55:15, Scott Hess wrote:
> > > As of https://codereview.chromium.org/1382283003 , the SQLite caches
should
> > > mostly be empty outside of active transactions.  I don't see anything in
the
> > > linked bug or design doc that indicates specific metrics around SQLite
> memory
> > > usage - do you have evidence that the cache is actually taking up a lot of
> > > memory?
> > > 
> > > [The above CL keeps the cache mostly-clear by using mmap.  I _believe_
that
> at
> > > this point most platforms support mmap.  iOS didn't for awhile, but I
think
> > > current versions do.  If this is untrue, I have a suspicion that making
the
> > > cache-clearing unconditional wouldn't actually affect performance much.]
> > 
> > Basically what we're doing here is to migrate existing
> MemoryPressureListener's
> > implementation to the new MemoryCoordinator's API :)
> > 
> > HistoryBackend::OnMemoryPressure is purging the cache, so we're trying to do
> the
> > same thing in MemoryCoordinator as well.
> 
> We don't have any up to date data for the value of individual MPLs right now,
> and are simply migrating them one at a time.
> 
> A problem in general with MPL is "jank", in that things are asked to trim and
> then they immediately start growing back to using as much memory as
previously.
> MC fixes this by turning notifications into states, and expecting clients to
> make persistent changes. This CL is an attempt to do that for the History DB.
> 
> If the change to using a memory mapped DB obviates this, then maybe we no
longer
> need an MPL client here either?
> 
> Regardless, it should be relatively easy to instrument this to see how much if
> any memory the cache is typically using.

OK - mostly I'm just nervous that we're tweaking a setting which mostly will
never matter.  The only time it might matter is when doing large transactions,
because a too-small cache will force SQLite to page to the journal file
manually, which is likely to be terrible for performance.  I'd be somewhat
surprised if the /2 or /4 cases would cause this, though.

[Scott Hess - ex-Googler, 2016-09-01 18:29:10]

https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc#newcode1001
sql/connection.cc:1001: return true;
I think this is fine, but might not even be worth checking.  You could just put:

  cache_size_ = new_cache_size;
  const std::string sql = ...;
  return Execute(sql.c_str());

Yes, that might allow for cases where cache_size_ doesn't equal the _actual_
cache size SQLite is using, but AFAICT that is already the case, and I don't
think any code depends on cache_size_ anyhow.  AFAICT, the pragma doesn't have
any provision for errors to be propagated, and the docs explicitly indicate that
the cache_size is in some sense advisory.

https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc#newcode1006
sql/connection.cc:1006: DLOG(WARNING) << "Could not update cache size: " <<
GetErrorMessage();
Execute() will already emit messages in case of errors.  See OnSqliteError(). 
This message would, of course, be ever-so-slightly more on-topic, but, honestly,
nobody is ever going to see it so I'd not bother.

https://codereview.chromium.org/2297243004/diff/1/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/2297243004/diff/1/sql/connection.h#newcode294
sql/connection.h:294: bool UpdateCacheSize(int new_cache_size);
I want to complain that this should be called Set rather than Update, but, sigh,
set_cache_size() would be confusing.

Maybe it should be SetCacheSize(), and if the database was not open yet it would
set cache_size_ and exit, but if the database was open it would also issue the
pragma.

[Maybe the pre-init configuration should really be pushed to a
Connection::Config object passed to the constructor.  I'll think about that.]

[chrisha, 2016-09-01 18:31:52]

On 2016/09/01 18:29:10, Scott Hess wrote:
> https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc
> File sql/connection.cc (right):
> 
>
https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc#newcode1001
> sql/connection.cc:1001: return true;
> I think this is fine, but might not even be worth checking.  You could just
put:
> 
>   cache_size_ = new_cache_size;
>   const std::string sql = ...;
>   return Execute(sql.c_str());
> 
> Yes, that might allow for cases where cache_size_ doesn't equal the _actual_
> cache size SQLite is using, but AFAICT that is already the case, and I don't
> think any code depends on cache_size_ anyhow.  AFAICT, the pragma doesn't have
> any provision for errors to be propagated, and the docs explicitly indicate
that
> the cache_size is in some sense advisory.
> 
>
https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc#newcode1006
> sql/connection.cc:1006: DLOG(WARNING) << "Could not update cache size: " <<
> GetErrorMessage();
> Execute() will already emit messages in case of errors.  See OnSqliteError(). 
> This message would, of course, be ever-so-slightly more on-topic, but,
honestly,
> nobody is ever going to see it so I'd not bother.
> 
> https://codereview.chromium.org/2297243004/diff/1/sql/connection.h
> File sql/connection.h (right):
> 
> https://codereview.chromium.org/2297243004/diff/1/sql/connection.h#newcode294
> sql/connection.h:294: bool UpdateCacheSize(int new_cache_size);
> I want to complain that this should be called Set rather than Update, but,
sigh,
> set_cache_size() would be confusing.
> 
> Maybe it should be SetCacheSize(), and if the database was not open yet it
would
> set cache_size_ and exit, but if the database was open it would also issue the
> pragma.
> 
> [Maybe the pre-init configuration should really be pushed to a
> Connection::Config object passed to the constructor.  I'll think about that.]

Thanks for the review. However, before we land this I think it would be prudent
to see if it actually has any realistic impact in mmap'd world. It may be that
the existing MPL simply no longer has relevance, in which case there's no need
to create a MCC port of it.

[haraken, 2016-09-01 18:42:19]

On 2016/09/01 18:31:52, chrisha (slow) wrote:
> On 2016/09/01 18:29:10, Scott Hess wrote:
> > https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc
> > File sql/connection.cc (right):
> > 
> >
>
https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc#newcode1001
> > sql/connection.cc:1001: return true;
> > I think this is fine, but might not even be worth checking.  You could just
> put:
> > 
> >   cache_size_ = new_cache_size;
> >   const std::string sql = ...;
> >   return Execute(sql.c_str());
> > 
> > Yes, that might allow for cases where cache_size_ doesn't equal the _actual_
> > cache size SQLite is using, but AFAICT that is already the case, and I don't
> > think any code depends on cache_size_ anyhow.  AFAICT, the pragma doesn't
have
> > any provision for errors to be propagated, and the docs explicitly indicate
> that
> > the cache_size is in some sense advisory.
> > 
> >
>
https://codereview.chromium.org/2297243004/diff/1/sql/connection.cc#newcode1006
> > sql/connection.cc:1006: DLOG(WARNING) << "Could not update cache size: " <<
> > GetErrorMessage();
> > Execute() will already emit messages in case of errors.  See
OnSqliteError(). 
> > This message would, of course, be ever-so-slightly more on-topic, but,
> honestly,
> > nobody is ever going to see it so I'd not bother.
> > 
> > https://codereview.chromium.org/2297243004/diff/1/sql/connection.h
> > File sql/connection.h (right):
> > 
> >
https://codereview.chromium.org/2297243004/diff/1/sql/connection.h#newcode294
> > sql/connection.h:294: bool UpdateCacheSize(int new_cache_size);
> > I want to complain that this should be called Set rather than Update, but,
> sigh,
> > set_cache_size() would be confusing.
> > 
> > Maybe it should be SetCacheSize(), and if the database was not open yet it
> would
> > set cache_size_ and exit, but if the database was open it would also issue
the
> > pragma.
> > 
> > [Maybe the pre-init configuration should really be pushed to a
> > Connection::Config object passed to the constructor.  I'll think about
that.]
> 
> Thanks for the review. However, before we land this I think it would be
prudent
> to see if it actually has any realistic impact in mmap'd world. It may be that
> the existing MPL simply no longer has relevance, in which case there's no need
> to create a MCC port of it.

That sounds better.

Scott: Is there any test we can use to evaluate the cache impact?

[sky, 2016-09-01 19:35:05]

Description was changed from

==========
[WIP] histroy: Adjust cache size OnMemoryStateChange()

OnMemoryStateChange() is a stateful API so it would be
better to adjust cache size rather than trying to trim
the cache memory every time OnMemoryStateChange() is
called. This CL adds sql::Connection::UpdateCacheSize()
to provide a way to adjust cache size based on the
memory state.

BUG=639700
==========

to

==========
[WIP] histroy: Adjust cache size OnMemoryStateChange()

OnMemoryStateChange() is a stateful API so it would be
better to adjust cache size rather than trying to trim
the cache memory every time OnMemoryStateChange() is
called. This CL adds sql::Connection::UpdateCacheSize()
to provide a way to adjust cache size based on the
memory state.

BUG=639700
==========

[sky, 2016-09-01 19:35:05]

sky@chromium.org changed reviewers:
+ brettw@chromium.org
- sky@chromium.org

[sky, 2016-09-01 19:35:11]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-09-01 19:35:11]

sky->brettw

[Scott Hess - ex-Googler, 2016-09-01 22:20:44]

On 2016/09/01 18:42:19, haraken wrote:
> Scott: Is there any test we can use to evaluate the cache impact?

Not that I'm aware of.  The basis of the mmap change was that there's a copy of
each page in the filesystem buffers _and_ in the SQLite cache, so we should let
the OS determine things.  So on the read side, I think it's pretty subtle and
hard to get an angle on.  When writing, the SQLite cache is used to stage
changes before committing the transaction.  So changes there are likely to
impact large transactions which have poor locality, which is determined by a
combination of update strategy and schema.

Put another way, we don't have any generally-agreed canonical profiles, so it's
hard to say what operations are important or not.  But I think you'd probably be
fine to drop the code then layer it back if someone complains in a year or two. 
Just my opinion.

[bashi, 2016-09-01 23:29:50]

On 2016/09/01 22:20:44, Scott Hess wrote:
> On 2016/09/01 18:42:19, haraken wrote:
> > Scott: Is there any test we can use to evaluate the cache impact?
> 
> Not that I'm aware of.  The basis of the mmap change was that there's a copy
of
> each page in the filesystem buffers _and_ in the SQLite cache, so we should
let
> the OS determine things.  So on the read side, I think it's pretty subtle and
> hard to get an angle on.  When writing, the SQLite cache is used to stage
> changes before committing the transaction.  So changes there are likely to
> impact large transactions which have poor locality, which is determined by a
> combination of update strategy and schema.
> 
> Put another way, we don't have any generally-agreed canonical profiles, so
it's
> hard to say what operations are important or not.  But I think you'd probably
be
> fine to drop the code then layer it back if someone complains in a year or
two. 
> Just my opinion.

Thanks so much Scott for review and feedback. I'll close this CL for now.

MPL removal: Maybe just remove as there is no easy way to measure impact of
cache?