Add histograms for deletion of session cookies on startup.

Add histograms for deletion of session cookies on startup.

The new histograms measure the performance of session cookie deletion.

BUG=485694
Committed: https://crrev.com/14d67837d5343978f5191f3a2d10208349d118b5
Cr-Commit-Position: refs/heads/master@{#329529}

[erikchen, 2015-05-11 21:51:12]

erikchen@chromium.org changed reviewers:
+ erikwright@chromium.org

[erikchen, 2015-05-11 21:51:13]

erikwright: Please review

[erikwright (departed), 2015-05-12 14:30:43]

https://codereview.chromium.org/1131413004/diff/40001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1131413004/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:704: int
deleted_cookies_count = 0;
Assuming you agree with my comment below, move count declaration to line 714 and
inline the calculation of elapsed time on line 712.

https://codereview.chromium.org/1131413004/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:719: if
(deleted_cookies_count > 0) {
I'm not convinced that this is useful. In any case, it can be generated via
dremel (query for UMA uploads containg both the TimeSpent.. and
NumberOfCookies.. and do the division there).

What would be more interesting would be to do a scatterplot of those two
numbers, to see if there is a clear relationship between them.

[erikchen, 2015-05-12 18:50:35]

erikwright: PTAL

https://codereview.chromium.org/1131413004/diff/40001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1131413004/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:704: int
deleted_cookies_count = 0;
On 2015/05/12 14:30:43, erikwright wrote:
> Assuming you agree with my comment below, move count declaration to line 714
and
> inline the calculation of elapsed time on line 712.

I removed the variables elapsed_time and deleted_cookies_count.

https://codereview.chromium.org/1131413004/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:719: if
(deleted_cookies_count > 0) {
On 2015/05/12 14:30:43, erikwright wrote:
> I'm not convinced that this is useful. In any case, it can be generated via
> dremel (query for UMA uploads containg both the TimeSpent.. and
> NumberOfCookies.. and do the division there).
> 
> What would be more interesting would be to do a scatterplot of those two
> numbers, to see if there is a clear relationship between them.

I wasn't aware that this was easy to do with dremel. I'm guessing that the
scatterplot you're referring to is also done via dremel? 

I've removed this histogram.

[erikwright (departed), 2015-05-12 19:39:50]

https://codereview.chromium.org/1131413004/diff/60001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1131413004/diff/60001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:711:
db_->GetLastChangeCount());
Either:

(1) Have DeleteSessionCookiesOnStartup return this count
(2) Put the metric reporting inside DeleteSessionCookiesOnStartup
(3) Inline the contents of DeleteSessionCookiesOnStartup here

Reason: It's not obvious from what we see here that
DeleteSessionCookiesOnStartup only executes a single statement. But this code
assumes that. If DeleteSessionCookiesOnStartup were later changed to (for
example) delete on a domain-by-domain basis, the assumption would be broken.

[erikchen, 2015-05-12 20:18:54]

Patchset #5 (id:80001) has been deleted

[erikchen, 2015-05-12 20:33:49]

erikwright: PTAL

https://codereview.chromium.org/1131413004/diff/60001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1131413004/diff/60001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:711:
db_->GetLastChangeCount());
On 2015/05/12 19:39:50, erikwright wrote:
> Either:
> 
> (1) Have DeleteSessionCookiesOnStartup return this count
> (2) Put the metric reporting inside DeleteSessionCookiesOnStartup
> (3) Inline the contents of DeleteSessionCookiesOnStartup here
> 
> Reason: It's not obvious from what we see here that
> DeleteSessionCookiesOnStartup only executes a single statement. But this code
> assumes that. If DeleteSessionCookiesOnStartup were later changed to (for
> example) delete on a domain-by-domain basis, the assumption would be broken.

Oh yes, good point. This logic originally started in
DeleteSessionCookiesOnStartup(), but got moved around while I was adding
different metrics. Thanks!

[erikwright (departed), 2015-05-12 21:29:32]

LGTM.

[erikchen, 2015-05-12 21:39:14]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-05-12 21:40:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131413004/100001

[commit-bot: I haz the power, 2015-05-12 21:50:50]

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

[commit-bot: I haz the power, 2015-05-12 21:50:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2015-05-12 22:03:54]

erikchen@chromium.org changed reviewers:
+ isherman@chromium.org

[erikchen, 2015-05-12 22:03:55]

isherman: Please review.

[Ilya Sherman, 2015-05-12 23:07:36]

LGTM

[Ilya Sherman, 2015-05-12 23:07:42]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-05-12 23:08:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131413004/100001

[commit-bot: I haz the power, 2015-05-12 23:12:07]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-05-12 23:13:02]

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