[DOMStorage] Do not try purging caches when no database is open

[DOMStorage] Do not try purging caches when no database is open

Looking at the histogram metrics, it looks like there are multiple
memory pressure signals triggered in the same device frequently. We
still record multiple 0s after clearing the caches at the first signal.
This CL tries to reduce this by checking before calling purge and
recording the values.

BUG=610551
Committed: https://crrev.com/d000d7c16219b88a225ef7676ab58f3388691623
Cr-Commit-Position: refs/heads/master@{#402539}

[ssid, 2016-06-24 21:39:41]

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

[ssid, 2016-06-24 21:39:41]

I am sorry I keep coming back with changes in this logic. I had to understand
what was going on based on the UMA, and I had posted some learning from UMA on
the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=610551. Please
see if this change makes sense. Do you have other suggestions?

[michaeln, 2016-06-27 21:51:15]

https://codereview.chromium.org/2092133003/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/2092133003/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:441: 50);
if (!stats.total_area_count)
  return;

I think the change makes sense. Would hoisting this test for no areas up here
may make the significant early return more obvious? This is the significant
change, is that right?

https://codereview.chromium.org/2092133003/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:470: if (!purge_reason)
{
nit: it might be nice to set purge_reason in all cases up above where your
figuring out the options, before calling ->PurgeMemory

[ssid, 2016-06-27 22:00:37]

https://codereview.chromium.org/2092133003/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/2092133003/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:441: 50);
On 2016/06/27 21:51:15, michaeln wrote:
> if (!stats.total_area_count)
>   return;
> 
> I think the change makes sense. Would hoisting this test for no areas up here
> may make the significant early return more obvious? This is the significant
> change, is that right?

Thanks for pointing out, i have moved this condition up and before the uma since
we don't want to keep recording 0s after purging all databases.

https://codereview.chromium.org/2092133003/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:470: if (!purge_reason)
{
On 2016/06/27 21:51:15, michaeln wrote:
> nit: it might be nice to set purge_reason in all cases up above where your
> figuring out the options, before calling ->PurgeMemory

Done.

[michaeln, 2016-06-27 22:59:02]

lgtm

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

https://codereview.chromium.org/2092133003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_impl.cc:470: return;
nit: you might be able to work this early return into the if else above too

if (PURGE_IF_NEEDED) {
 ...
} else if (PURGE_UNOPENED) {
  purge_reason = "Moderate"
  if (!initial_stats.inactive_area_count)
    return;
} else {
  purge_reason = "Aggressive"
}

[ssid, 2016-06-27 23:09:29]

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

https://codereview.chromium.org/2092133003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_impl.cc:470: return;
On 2016/06/27 22:59:02, michaeln wrote:
> nit: you might be able to work this early return into the if else above too
> 
> if (PURGE_IF_NEEDED) {
>  ...
> } else if (PURGE_UNOPENED) {
>   purge_reason = "Moderate"
>   if (!initial_stats.inactive_area_count)
>     return;
> } else {
>   purge_reason = "Aggressive"
> }
> 

Hm there are 2 cases where this helps.
1. When we decide purging is needed and set to PURGE_UNOPENED because size limit
or low end device. But, we do not actually check if there are unopened
databases.
2. When moderate memory signal is passed.
This will do an early return for both these cases. Previously we only used to
return for case (1). Now we do return for both these cases.

Isn't it better to do this check once here rather than having it twice in
different if conditions?

[ssid, 2016-06-28 17:09:50]

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

[commit-bot: I haz the power, 2016-06-28 17:10:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaeln, 2016-06-28 18:33:45]

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

https://codereview.chromium.org/2092133003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_impl.cc:470: return;
On 2016/06/27 23:09:29, ssid wrote:
> On 2016/06/27 22:59:02, michaeln wrote:
> > nit: you might be able to work this early return into the if else above too
> > 
> > if (PURGE_IF_NEEDED) {
> >  ...
> > } else if (PURGE_UNOPENED) {
> >   purge_reason = "Moderate"
> >   if (!initial_stats.inactive_area_count)
> >     return;
> > } else {
> >   purge_reason = "Aggressive"
> > }
> > 
> 
> Hm there are 2 cases where this helps.
> 1. When we decide purging is needed and set to PURGE_UNOPENED because size
limit
> or low end device. But, we do not actually check if there are unopened
> databases.
> 2. When moderate memory signal is passed.
> This will do an early return for both these cases. Previously we only used to
> return for case (1). Now we do return for both these cases.
> 
> Isn't it better to do this check once here rather than having it twice in
> different if conditions?

still lgtm, good point about only having one early return call for the condition

[commit-bot: I haz the power, 2016-06-28 20:12:17]

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

[commit-bot: I haz the power, 2016-06-28 20:12:18]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-06-28 20:33:39]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2016-06-28 20:35:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 20:41:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-28 20:43:55]

Description was changed from

==========
[DOMStorage] Do not try purging caches when no database is open

Looking at the histogram metrics, it looks like there are multiple
memory pressure signals triggered in the same device frequently. We
still record multiple 0s after clearing the caches at the first signal.
This CL tries to reduce this by checking before calling purge and
recording the values.

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

to

==========
[DOMStorage] Do not try purging caches when no database is open

Looking at the histogram metrics, it looks like there are multiple
memory pressure signals triggered in the same device frequently. We
still record multiple 0s after clearing the caches at the first signal.
This CL tries to reduce this by checking before calling purge and
recording the values.

BUG=610551

Committed: https://crrev.com/d000d7c16219b88a225ef7676ab58f3388691623
Cr-Commit-Position: refs/heads/master@{#402539}
==========

[commit-bot: I haz the power, 2016-06-28 20:43:56]

Patchset 2 (id:??) landed as
https://crrev.com/d000d7c16219b88a225ef7676ab58f3388691623
Cr-Commit-Position: refs/heads/master@{#402539}