Purge browser cache for dom storage in a smarter way

Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551, 607449
Committed: https://crrev.com/454f3cefe1b33de5a422778e5cf09d6311bb7fa7
Cr-Commit-Position: refs/heads/master@{#395418}

[ssid, 2016-05-06 03:07:35]

Description was changed from

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs). Also set the cache size lower.
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. To keep thread
   safety the listener (which takes task runner and context references)
   is leaked and a lock is used to destroy the references to database
   and task runner.

BUG=
==========

to

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs). Also set the cache size lower.
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. To keep thread
   safety the listener (which takes task runner and context references)
   is leaked and a lock is used to destroy the references to database
   and task runner.

BUG=
==========

[ssid, 2016-05-06 03:08:18]

Patchset #1 (id:1) has been deleted

[ssid, 2016-05-06 03:28:51]

ssid@chromium.org changed reviewers:
+ michaeln@chromium.org

[ssid, 2016-05-06 03:28:52]

Hi, I have drafted a CL to purge memory better in dom databases from what I
understood from the implementation and all the ideas that I had in mind. It's
probably over engineered and error prone.
Could you please take an initial look at this?

Thanks!

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:187:
inactive_in_memory_area_size_budget = 1024 * 1024;
I chose these values based on what sizes I observed from the UMA metric
LocalStorage.BrowserLocalStorageSizeInKB and Sqlite.SizeKB.DOMStorageDatabase.
But it is still unclear how many databases does a single tab uses, or the total
number of databases in memory.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:197: size_t cache_size =
it->second.area_->map_usage_in_bytes();
Do you think just the cache size is a good estimate of the memory usage?
From what I understand the cache is always bigger than the sql databases, since
the cache is not cleared but the databases get committed to file.

[ssid, 2016-05-06 03:30:47]

Description was changed from

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs). Also set the cache size lower.
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. To keep thread
   safety the listener (which takes task runner and context references)
   is leaked and a lock is used to destroy the references to database
   and task runner.

BUG=
==========

to

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on num of databases and cache sizes based on the
   platform and ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs). Also set the cache size lower.
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. To keep thread
   safety the listener (which takes task runner and context references)
   is leaked and a lock is used to destroy the references to database
   and task runner.

BUG=
==========

[michaeln, 2016-05-06 20:07:44]

I haven't looked closely yet, but thanx for getting the itch to make it better
and doing something about that itch!

[michaeln, 2016-05-09 21:50:31]

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:307: if (backing_)
There are some complications that i think make this problematic?

The db is used on two different 'sequences'. The initial import of data is
performed on the 'primary' sequence. Commits are the performed on the 'commit'
sequence. And the db can be reset on the 'primary' sequence when there are no
commits in flight.

A possible simplification to sidestep complexity stemming from that is to trim
the caches ever time we touch the db (see other comments in this file). 

Or consider leaving this part out for now.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:378:
backing_->ReadAllValues(&initial_values);
so maybe call TrimDatabase() here

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:483:
commit_batch->changed_values);
and maybe call TrimMemory() here

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_impl.cc:61: base::AutoLock
lock(lock_);
Lets avoid leaks and locks for this. Having a separate helper with a
MemoryPressureListener to eves drop on OnMemoryPressure in a well known thread
looks good, but leaking and interlocking on shutdown does not.

What if...

A) We have the DOMStorageContextWrapper be responsible for setting up and
tearing down the MemoryPressureListener. The purpose of the wrapper class is to
help plug into other parts of chrome. The base::Listener could be created in the
ctor (called on the UI thread) and deleted in
DOMStorageContextWrapper::Shutdow() (aldo called on the UI thread). That might
be the most straightforward way of hooking into OnMemoryPressure() which whould
then PostTask on the sequence runner to the context impl.

There's probably other ways to avoid leaks and locks.

B) Another idea, lazily construct this inner Listener class.  PostTask to the
BrowserThread::IO to initialize it. We'll have to construct the
base::MemoryPressureListener on the IO thread. Then in
DOMStorageContextImpl::Shutdown() we call
BrowserThread::DeleteSoon(BrowserThread::IO, mem_listner_.release()). That
sounds more complicated that option A though.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_database.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_database.cc:133: db_->TrimMemory(true /*
aggressively */);
This needs to have a test for IsOpen() or an assertion that it's only called
when IsOpen().

If we do decide to do this unconditionally on each read/write, the ReadAll and
Commit methods could call db_->TrimMem() directly w/o exposing this as a public
method.

[michaeln, 2016-05-09 22:12:17]

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:177: #if
defined(OS_ANDROID)
This might be more complicated then it needs to be? What if we trust the the
base::MemPressureListener will tell us when we need to PURGE_AGGRESSIVE and in
normal operation PURGE_UNOPENED when mem_used > kMemLimit || num_areas >
kAreaLimit.

We're working on a new impl of DomStorage and this logic will eventually want to
be ported over to the new impl too. The simpler it is, the simpler that will be.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:187:
inactive_in_memory_area_size_budget = 1024 * 1024;
On 2016/05/06 03:28:51, ssid wrote:
> I chose these values based on what sizes I observed from the UMA metric
> LocalStorage.BrowserLocalStorageSizeInKB and Sqlite.SizeKB.DOMStorageDatabase.
> But it is still unclear how many databases does a single tab uses, or the
total
> number of databases in memory.

we don't have stats for the number of 'area in use', that might be nice to know,
also how many are inuse by multiple renderers at the same time

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:197: size_t cache_size =
it->second.area_->map_usage_in_bytes();
On 2016/05/06 03:28:52, ssid wrote:
> Do you think just the cache size is a good estimate of the memory usage?
> From what I understand the cache is always bigger than the sql databases,
since
> the cache is not cleared but the databases get committed to file.

Sure, this is what we have to work with.

[ssid, 2016-05-10 01:49:59]

Description was changed from

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on num of databases and cache sizes based on the
   platform and ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs). Also set the cache size lower.
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. To keep thread
   safety the listener (which takes task runner and context references)
   is leaked and a lock is used to destroy the references to database
   and task runner.

BUG=
==========

to

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551
==========

[ssid, 2016-05-10 02:04:16]

Thanks for your patient review. I have updated the CL based on your suggestions.
PTAL.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:307: if (backing_)
On 2016/05/09 21:50:31, michaeln wrote:
> There are some complications that i think make this problematic?
> 
> The db is used on two different 'sequences'. The initial import of data is
> performed on the 'primary' sequence. Commits are the performed on the 'commit'
> sequence. And the db can be reset on the 'primary' sequence when there are no
> commits in flight.
> 
> A possible simplification to sidestep complexity stemming from that is to trim
> the caches ever time we touch the db (see other comments in this file). 
> 
> Or consider leaving this part out for now.
> 
> 

Thanks, I didn't realize this issue.
Yes, post task might be overkill here and we are now clearing at commit, so i
removed this part.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:378:
backing_->ReadAllValues(&initial_values);
On 2016/05/09 21:50:31, michaeln wrote:
> so maybe call TrimDatabase() here
Done. Made the trim non-aggressive in database.cc.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:483:
commit_batch->changed_values);
On 2016/05/09 21:50:31, michaeln wrote:
> and maybe call TrimMemory() here

Done.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_impl.cc:61: base::AutoLock
lock(lock_);
On 2016/05/09 21:50:31, michaeln wrote:
> Lets avoid leaks and locks for this. Having a separate helper with a
> MemoryPressureListener to eves drop on OnMemoryPressure in a well known thread
> looks good, but leaking and interlocking on shutdown does not.
> 
> What if...
> 
> A) We have the DOMStorageContextWrapper be responsible for setting up and
> tearing down the MemoryPressureListener. The purpose of the wrapper class is
to
> help plug into other parts of chrome. The base::Listener could be created in
the
> ctor (called on the UI thread) and deleted in
> DOMStorageContextWrapper::Shutdow() (aldo called on the UI thread). That might
> be the most straightforward way of hooking into OnMemoryPressure() which
whould
> then PostTask on the sequence runner to the context impl.
> 
> There's probably other ways to avoid leaks and locks.
> 
> B) Another idea, lazily construct this inner Listener class.  PostTask to the
> BrowserThread::IO to initialize it. We'll have to construct the
> base::MemoryPressureListener on the IO thread. Then in
> DOMStorageContextImpl::Shutdown() we call
> BrowserThread::DeleteSoon(BrowserThread::IO, mem_listner_.release()). That
> sounds more complicated that option A though.
> 
> 

listener in DOMStorageContextWrapper sounds like a good idea. I should have
looked into the code little more.

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_database.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_database.cc:133: db_->TrimMemory(true /*
aggressively */);
On 2016/05/09 21:50:31, michaeln wrote:
> This needs to have a test for IsOpen() or an assertion that it's only called
> when IsOpen().
> 
Fixed.

> If we do decide to do this unconditionally on each read/write, the ReadAll and
> Commit methods could call db_->TrimMem() directly w/o exposing this as a
public
> method.

Sorry I do not understand how do i get the pointer to sql::Connection from
DOMStorageArea. It only holds an object of DOMStorageDatabaseAdapter.
I would have to write:
reinterpret_cast<DOMStorageDatabase*>(backing_.get())->db_->TrimMemory(false);

And also add DOMStorageArea as friend of DOMStorageDatabase.

Am I missing something here?

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1953703004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:177: #if
defined(OS_ANDROID)
On 2016/05/09 22:12:17, michaeln wrote:
> This might be more complicated then it needs to be? What if we trust the the
> base::MemPressureListener will tell us when we need to PURGE_AGGRESSIVE and in
> normal operation PURGE_UNOPENED when mem_used > kMemLimit || num_areas >
> kAreaLimit.
> 
> We're working on a new impl of DomStorage and this logic will eventually want
to
> be ported over to the new impl too. The simpler it is, the simpler that will
be.

I agree this is very complicated. I was expecting better suggestions on what is
useful.
I have added just 2 cases now. One more than what you suggested, just for low
end device. wdyt?

https://codereview.chromium.org/1953703004/diff/60001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1953703004/diff/60001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:26: #endif
Not sure if I should move these to dom_storage_types.h. Since these are just
used in this file.

[ssid, 2016-05-10 17:27:44]

Patchset #4 (id:80001) has been deleted

[ssid, 2016-05-10 17:34:18]

Patchset #4 (id:100001) has been deleted

[ssid, 2016-05-11 15:38:02]

Patchset #5 (id:140001) has been deleted

[ssid, 2016-05-11 15:39:58]

Patchset #6 (id:180001) has been deleted

[ssid, 2016-05-11 15:45:33]

Patchset #6 (id:200001) has been deleted

[ssid, 2016-05-11 22:50:10]

I changed the logic a bit to include all namespaces and purge all if needed. I
also added 2 UMA metrics to track how much memory is used and is being purged.
WDYT?

[ssid, 2016-05-12 22:22:30]

On 2016/05/11 22:50:10, ssid wrote:
> I changed the logic a bit to include all namespaces and purge all if needed. I
> also added 2 UMA metrics to track how much memory is used and is being purged.
> WDYT?

friendly ping.

[michaeln, 2016-05-14 00:42:51]

michaeln@chromium.org changed reviewers:
+ jsbell@chromium.org

[michaeln, 2016-05-14 00:42:52]

This looks good to me, thanx for doing this!

I'm wondering if we should wait till after the branch point to land so we have
more time in canary to ferret out any surprises? Unless theres a reason to rush?
 It might be good to have unittest.

+jsbell to the reviewers list to see what he thinks about ^^^

https://codereview.chromium.org/1953703004/diff/220001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1953703004/diff/220001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_area.cc:480: backing_->TrimMemory();
What i meant by not exposing this in public api was that
DOMStorageDatabase::CommitChanges and ReadAllValues methods could internally
perform this function. So no public api or virtual adapter plumbing would be
needed.

[jsbell, 2016-05-16 16:20:45]

On 2016/05/14 00:42:52, michaeln wrote:
> I'm wondering if we should wait till after the branch point to land so we have
> more time in canary to ferret out any surprises? Unless theres a reason to
rush?

sgtm - less than a week until branch cut now.

[michaeln, 2016-05-16 20:27:35]

Description was changed from

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551
==========

to

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551,607449
==========

[ssid, 2016-05-16 20:45:37]

Patchset #8 (id:260001) has been deleted

[ssid, 2016-05-16 20:48:24]

Patchset #8 (id:280001) has been deleted

[ssid, 2016-05-16 20:51:57]

Thanks for review. I added unittest. ptal.

Makes sense to wait till branch point. I did not think about the release dates.
I will land it after 19th.

https://codereview.chromium.org/1953703004/diff/220001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1953703004/diff/220001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_area.cc:480: backing_->TrimMemory();
On 2016/05/14 00:42:52, michaeln wrote:
> What i meant by not exposing this in public api was that
> DOMStorageDatabase::CommitChanges and ReadAllValues methods could internally
> perform this function. So no public api or virtual adapter plumbing would be
> needed.

Sorry, I was blind. Fixed this.

[michaeln, 2016-05-16 21:32:42]

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl_unittest.cc (right):

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl_unittest.cc:271:
TEST_F(DOMStorageContextImplTest, PurgeMemory) {
Thanx for adding the tests!

[michaeln, 2016-05-16 21:34:39]

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_area.h:148: struct CommitBatch {
needs a CONTENT_EXPORT here now

[ssid, 2016-05-16 23:02:26]

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_area.h:148: struct CommitBatch {
On 2016/05/16 21:34:39, michaeln wrote:
> needs a CONTENT_EXPORT here now

Done.

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl_unittest.cc (right):

https://codereview.chromium.org/1953703004/diff/300001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl_unittest.cc:271:
TEST_F(DOMStorageContextImplTest, PurgeMemory) {
On 2016/05/16 21:32:42, michaeln wrote:
> Thanx for adding the tests!

Acknowledged.

[ssid, 2016-05-20 08:24:47]

Friendly ping.

[michaeln, 2016-05-20 19:41:57]

LGTM!

[ssid, 2016-05-20 22:55:24]

ssid@chromium.org changed reviewers:
+ rkaplow@chromium.org

[ssid, 2016-05-20 22:55:25]

+ rkaplow for uma histogram change. ptal, thanks

[ssid, 2016-05-23 18:22:38]

ssid@chromium.org changed reviewers:
+ mpearson@chromium.org
- rkaplow@chromium.org

[ssid, 2016-05-23 18:22:39]

+mpearson since Robert is on holiday

[Mark P, 2016-05-23 18:27:15]

https://codereview.chromium.org/1953703004/diff/320001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1953703004/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:438:
initial_stats.total_cache_size / 1024, 0, 100000,
There is already an "underflow" bucket; please use a min_value of 1, not 0.
ditto below

https://codereview.chromium.org/1953703004/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1953703004/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19564: +    Total size of the HTML5
LocalStorage browser-side cache purged in KB.
Please briefly mention when this tends to happen.

https://codereview.chromium.org/1953703004/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19571: +    Total size of the HTML5
LocalStorage DB(s) in KB in the browser-side cache.
Please explicitly describe when this is emitted.

Also, please change "DB(s)" to "DBs" or "databases".  Writing it like "DB(s)"
make me think this function is emitted once for each database, which isn't
right.

[ssid, 2016-05-23 18:42:17]

Thanks, made changes suggested. ptal.

https://codereview.chromium.org/1953703004/diff/320001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1953703004/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:438:
initial_stats.total_cache_size / 1024, 0, 100000,
On 2016/05/23 18:27:14, Mark P wrote:
> There is already an "underflow" bucket; please use a min_value of 1, not 0.
> ditto below

Done.

https://codereview.chromium.org/1953703004/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1953703004/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19564: +    Total size of the HTML5
LocalStorage browser-side cache purged in KB.
On 2016/05/23 18:27:15, Mark P wrote:
> Please briefly mention when this tends to happen. 

Sorry I was thinking it should be a single line description. Fixed them, wdyt?

https://codereview.chromium.org/1953703004/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19571: +    Total size of the HTML5
LocalStorage DB(s) in KB in the browser-side cache.
On 2016/05/23 18:27:14, Mark P wrote:
> Please explicitly describe when this is emitted.
> 
> Also, please change "DB(s)" to "DBs" or "databases".  Writing it like "DB(s)"
> make me think this function is emitted once for each database, which isn't
> right.

Done.

[Mark P, 2016-05-23 19:02:56]

histograms.xml lgtm

[ssid, 2016-05-23 19:03:33]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 19:03:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953703004/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953703004/340001

[commit-bot: I haz the power, 2016-05-23 20:25:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 20:25:03]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-05-23 21:27:26]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-05-23 21:27:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1953703004/#ps340001
(title: "Fix limit and description for UMA.")

[commit-bot: I haz the power, 2016-05-23 21:27:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953703004/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953703004/340001

[commit-bot: I haz the power, 2016-05-23 21:34:03]

Description was changed from

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551,607449
==========

to

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551,607449
==========

[commit-bot: I haz the power, 2016-05-23 21:34:04]

Committed patchset #10 (id:340001)

[commit-bot: I haz the power, 2016-05-23 21:35:37]

Description was changed from

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551,607449
==========

to

==========
Purge browser cache for dom storage in a smarter way

This CL does:
1. Use the total cache size to decide when to purge memory from in
   memory caches.
2. Use different limits on databases and cache sizes based on the
   platform and the ram available.
3. For low memory devices do not keep any cache around for closed
   databases (closed tabs).
4. Add a memory pressure listener to dom storage. This listener is
   supposed to construct and destruct in same thread. So, it is
   constructed and destructed on UI thread by DOMStorageContextWrapper
   and post tasks purge to the dom task runner.

BUG=610551,607449

Committed: https://crrev.com/454f3cefe1b33de5a422778e5cf09d6311bb7fa7
Cr-Commit-Position: refs/heads/master@{#395418}
==========

[commit-bot: I haz the power, 2016-05-23 21:35:39]

Patchset 10 (id:??) landed as
https://crrev.com/454f3cefe1b33de5a422778e5cf09d6311bb7fa7
Cr-Commit-Position: refs/heads/master@{#395418}