Add access count and time-since-accessed histograms to temp storage eviction.

Add access count and time-since-accessed histograms to temp storage eviction.

This CL adds the Quota.EvictedOriginAccessCount and
Quota.EvictedOriginTimeSinceAccess histograms that track how many time an
evicted origin has been used and how long since it has been used respectively.

BUG=542075
Committed: https://crrev.com/e6b51fbf5c9043b120f98b2b3f78d2c86742acf2
Cr-Commit-Position: refs/heads/master@{#358210}

[calamity, 2015-10-29 03:18:01]

calamity@chromium.org changed reviewers:
+ isherman@chromium.org, michaeln@chromium.org

[calamity, 2015-10-29 03:18:02]

+isherman for histograms OWNERS.
+michaeln for storage/ and c/b/quota OWNERS.

Thanks!

[Ilya Sherman, 2015-10-29 22:32:46]

histograms lgtm

[michaeln, 2015-10-30 00:49:02]

Thnx for adding these!

https://codereview.chromium.org/1424653002/diff/20001/content/browser/quota/q...
File content/browser/quota/quota_database_unittest.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:468:
EXPECT_EQ(kTableEntries[0].last_modified_time, entry.last_modified_time);
can you add a test negative, try to get info for an origin that is not in the
table

https://codereview.chromium.org/1424653002/diff/20001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:675: GetOriginInfo(kDbFile);
Any test that touches the disk is incredibly slow.

I see you're following the example of all the other tests in here by conducting
the test with an in memory and on disk sqlitedb. I'd vote to skip the on disk
test. Sqlite has seperate test coverage and the new code for the new quotadb
method doesn't handle disk vs mem any differently.

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:382: return statement.Succeeded();
Hmmm... this will return true when nothing was found.

There's precedent for methods that lookup a value in a particular row to return
false when there is no such row, see GetHostQuota() and FindOriginUsedCount().
It'd be good to keep the getters consistent about how NOTFOUND is handled. Wdyt
about adjusting the semantics of the return value to match those pre-existing
methods.

  if (!statement.Step())
    return false;

  // read the columns
  return true;


I think it'd be good to change recently added GetOriginLastEvictionTime() to
match too. (Sorry i didn't notice the difference when reviewing the CL that
added that new method.)

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:169: base::Time now = base::Time::Now();
maybe swap  lines |now| and |entry| are declared on since entry is only used
within the new block below and now is used in the new block and in existing code
at the end of the method

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:170: if (database->GetOriginInfo(origin,
type, &entry)) {
please add |is_eviction| to this condition too

[calamity, 2015-11-02 06:27:45]

https://codereview.chromium.org/1424653002/diff/20001/content/browser/quota/q...
File content/browser/quota/quota_database_unittest.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:468:
EXPECT_EQ(kTableEntries[0].last_modified_time, entry.last_modified_time);
On 2015/10/30 00:49:02, michaeln wrote:
> can you add a test negative, try to get info for an origin that is not in the
> table

Done.

https://codereview.chromium.org/1424653002/diff/20001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:675: GetOriginInfo(kDbFile);
On 2015/10/30 00:49:02, michaeln wrote:
> Any test that touches the disk is incredibly slow.
> 
> I see you're following the example of all the other tests in here by
conducting
> the test with an in memory and on disk sqlitedb. I'd vote to skip the on disk
> test. Sqlite has seperate test coverage and the new code for the new quotadb
> method doesn't handle disk vs mem any differently.

Done.

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:382: return statement.Succeeded();
On 2015/10/30 00:49:02, michaeln wrote:
> Hmmm... this will return true when nothing was found.
> 
> There's precedent for methods that lookup a value in a particular row to
return
> false when there is no such row, see GetHostQuota() and FindOriginUsedCount().
> It'd be good to keep the getters consistent about how NOTFOUND is handled.
Wdyt
> about adjusting the semantics of the return value to match those pre-existing
> methods.
> 
>   if (!statement.Step())
>     return false;
> 
>   // read the columns
>   return true;
> 
> 
> I think it'd be good to change recently added GetOriginLastEvictionTime() to
> match too. (Sorry i didn't notice the difference when reviewing the CL that
> added that new method.)

The return value indicating success/error is actually used by the QuotaManager
quite heavily to determine if it should disable the database. I'm not sure why
GetHostQuota is different but FindOriginUsedCount is private and used in
contexts where subsequent database failures will return false.

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:169: base::Time now = base::Time::Now();
On 2015/10/30 00:49:02, michaeln wrote:
> maybe swap  lines |now| and |entry| are declared on since entry is only used
> within the new block below and now is used in the new block and in existing
code
> at the end of the method

Done.

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:170: if (database->GetOriginInfo(origin,
type, &entry)) {
On 2015/10/30 00:49:02, michaeln wrote:
> please add |is_eviction| to this condition too

Done. Nice catch.

[michaeln, 2015-11-03 02:42:04]

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:382: return statement.Succeeded();
> > Hmmm... this will return true when nothing was found.
> > 
> > There's precedent for methods that lookup a value in a particular row to
> return
> > false when there is no such row, see GetHostQuota() and
FindOriginUsedCount().
> > It'd be good to keep the getters consistent about how NOTFOUND is handled.
> Wdyt
> > about adjusting the semantics of the return value to match those
pre-existing
> > methods.
> > 
> >   if (!statement.Step())
> >     return false;
> > 
> >   // read the columns
> >   return true;
> > 
> > 
> > I think it'd be good to change recently added GetOriginLastEvictionTime() to
> > match too. (Sorry i didn't notice the difference when reviewing the CL that
> > added that new method.)
> 
> The return value indicating success/error is actually used by the QuotaManager
> quite heavily to determine if it should disable the database. I'm not sure why
> GetHostQuota is different but FindOriginUsedCount is private and used in
> contexts where subsequent database failures will return false.

The return value of the new GetOriginInfo method is not used in that way in the
CL. It appears to be used in a way that expects 'true' to mean that something
was found and then the values it returns are logged. I think consistency trumps
here and it looks like the return value indicating whether something was gotten
is prevalent for getters of specific values.

[calamity, 2015-11-04 00:42:44]

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1424653002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:382: return statement.Succeeded();
On 2015/11/03 02:42:04, michaeln wrote:
> > > Hmmm... this will return true when nothing was found.
> > > 
> > > There's precedent for methods that lookup a value in a particular row to
> > return
> > > false when there is no such row, see GetHostQuota() and
> FindOriginUsedCount().
> > > It'd be good to keep the getters consistent about how NOTFOUND is handled.
> > Wdyt
> > > about adjusting the semantics of the return value to match those
> pre-existing
> > > methods.
> > > 
> > >   if (!statement.Step())
> > >     return false;
> > > 
> > >   // read the columns
> > >   return true;
> > > 
> > > 
> > > I think it'd be good to change recently added GetOriginLastEvictionTime()
to
> > > match too. (Sorry i didn't notice the difference when reviewing the CL
that
> > > added that new method.)
> > 
> > The return value indicating success/error is actually used by the
QuotaManager
> > quite heavily to determine if it should disable the database. I'm not sure
why
> > GetHostQuota is different but FindOriginUsedCount is private and used in
> > contexts where subsequent database failures will return false.
> 
> The return value of the new GetOriginInfo method is not used in that way in
the
> CL. It appears to be used in a way that expects 'true' to mean that something
> was found and then the values it returns are logged. I think consistency
trumps
> here and it looks like the return value indicating whether something was
gotten
> is prevalent for getters of specific values.

Done. Added some comments about return values.

[michaeln, 2015-11-04 23:57:16]

lgtm, thnx!

[michaeln, 2015-11-05 00:01:23]

https://codereview.chromium.org/1424653002/diff/60001/content/browser/quota/q...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1424653002/diff/60001/content/browser/quota/q...
content/browser/quota/quota_manager_unittest.cc:1386:
QuotaManager::kEvictedOriginTimeSinceAccessHistogram, 1);
ooops, wait... this test is red?

[calamity, 2015-11-05 07:10:48]

https://codereview.chromium.org/1424653002/diff/60001/content/browser/quota/q...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1424653002/diff/60001/content/browser/quota/q...
content/browser/quota/quota_manager_unittest.cc:1386:
QuotaManager::kEvictedOriginTimeSinceAccessHistogram, 1);
On 2015/11/05 00:01:23, michaeln wrote:
> ooops, wait... this test is red?

Only ran quota database tests locally after the change. Forgot to update the
QuotaManager after the return value change of GetOriginInfo. Fixed now.

[calamity, 2015-11-06 00:45:10]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-11-06 00:45:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1424653002/#ps80001
(title: "fix test")

[commit-bot: I haz the power, 2015-11-06 00:46:48]

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

[commit-bot: I haz the power, 2015-11-06 00:53:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-06 00:54:57]

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