Initial commit of CookieMonster statistics.

Initial commit of CookieMonster statistics.

Specifically:
* Number of cookies, recorded every ten minutes of active browsing (i.e. cookies being requested from CookieMonster)
* Last access time, recorded when cookie accessed.
* Last access time for evicted cookies, recorded on eviction
* Time until cookie expires, recorded on insertion.
* Reason for a cookie being removed from store, recorded when removed.
* Size of batch update to persistent cookie store and whether it succeeded or failed, recorded at success/failure.

BUG=4005
TEST=net_unittests CookieMonster.* on Linux

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50477

[Randy Smith (Not in Mondays), 2010-06-11 18:01:08]

Andy, Eric: Would you take a look at this first pass for cookie monster
statistics?  I'm specifically interested in:

* How I really should be doing the task dispatching around recording statistics
every six hours of uptime; what I've makes me think that there has to be a
better way.  
* (Eric specifically) I'm choosing only to gather number of cookie stats on the
original CookieMonster created in FactoryForOriginal, but all the other stats
are gathered for all CookieMonsters.  I think the distinction isn't that
important for the other stats (the main cookie monster will have much more
activity in any reasonable universe I can think of), and I don't want the small
stores for the other CookieMonsters skewing the average size down.  Does this
logic make sense to you?

Thanks much in advance.

[ahendrickson, 2010-06-11 18:58:33]

Bearing in mind that I don't know the cookie monster or histogram systems, it
looks good to me.

A couple of minor nits follow.

http://codereview.chromium.org/2718011/diff/1/4
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/1/4#newcode1191
net/base/cookie_monster.cc:1191: kRecordStatisticsInterval - uptime_seconds %
kRecordStatisticsInterval;
Nit: Can you add parentheses around the mod operation?

http://codereview.chromium.org/2718011/diff/1/5
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/1/5#newcode271
net/base/cookie_monster.h:271: static const int kRecordStatisticsInterval = 6 *
60 * 60;
Minor nit: I'd like to see kRecordStatisticsInterval as
kRecordStatisticsIntervalSeconds (or something like that) to make it clear it's
in seconds when reading the code.

http://codereview.chromium.org/2718011/diff/1/5#newcode273
net/base/cookie_monster.h:273: enum DeletionCause { kExplicit, kOverwrite,
kExpired, kEvicted,
Minor nit:  It would be easier to read if the enums were something like
kDeleteCookieExplicit, kDeleteCookieOverwrite, etc.

[eroman, 2010-06-11 20:15:00]

Some high level comments before I dive into the nits.

http://codereview.chromium.org/2718011/diff/1/2
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/2718011/diff/1/2#newcode237
chrome/browser/net/chrome_url_request_context.cc:237: delete stats_info_;
Generally a safer pattern is to make stats_info_ be a scoped_ptr<>.

Since it is also possible that a Task will not be Run(), but just deleted. (In
which case this will result in a leak).

http://codereview.chromium.org/2718011/diff/1/2#newcode254
chrome/browser/net/chrome_url_request_context.cc:254:
pref->GetInt64(prefs::kUninstallMetricsUptimeSec);
Note that rather than doing this pingpong from the IO thread back over to the UI
thread, you can extract the UninstallMetricsUptimeSec as part of
FactoryOriginal's constructor (which runs on UI thread).

That said, I have comments in the cookie monster about not doing a strictly
time-based metrics tracking.

http://codereview.chromium.org/2718011/diff/1/4
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/1/4#newcode686
net/base/cookie_monster.cc:686:
UMA_HISTOGRAM_CUSTOM_TIMES("net.CookieExpirationTimes",
I recommend naming these something more like "Duration" as opposed to "Times".

http://codereview.chromium.org/2718011/diff/1/4#newcode825
net/base/cookie_monster.cc:825:
UMA_HISTOGRAM_ENUMERATION("net.CookieDeletionCause", kEvicted,
Note that this isn't going to work how you expect.

The UMA_HISTOGRAM_* macros just create a function-local static variable that
contains the data.

Hence if you instantiate the histogram from multiple functions, they will
actually be separate histograms.

To fix this, create a helper function so the instantiation only happens from a
single location.

http://codereview.chromium.org/2718011/diff/1/4#newcode1193
net/base/cookie_monster.cc:1193: MessageLoop::current()->PostDelayedTask(
In general I don't like these sort of heartbeat strategies, as they can lead to
problems. Here are some of my dislikes:

* They can lead to spurious work, even in periods of inactivity (i.e. you aren't
using chrome but it is still spinning up every now and then).

* Weird things happen after waking up from sleep (intervals can be missed, and
timing edges will get clipped).

* The complexity doesn't scale well when lots of code does this. (Can get
unpredictable latency spikes when unrelated timed tasks end up overlapping
because of their periodicity).

Here are some suggestions for alternative approaches that don't require a
recurring task (the value being measured is a bit different, but should still be
useful):

(a) Each time a cookie is added/removed, record the new num cookies  in the
histogram. This histogram will give a sense of how many cookies users have,
although it will be more heavily weighted towards users that update their
cookies a lot. You can easily reduce the bias by introducing a maximum update
perio -- only update the histogram if at least XXX seconds have elapsed since
the last update.

(b) Each time cookies are *accessed*, record the current num cookies in the
histogram. (accesses happen very frequently, and only during periods of
activity. This approach is biased towards users that browse the web the most.
Same thing as in (a), you can reduce the bias by only updating if XXX seconds
have elapsed since the last update.

[Randy Smith (Not in Mondays), 2010-06-16 19:49:35]

I believe I've responded to all comments; could folks take another look?

Jim: Could you take a look at the histogram.cc change (you can look at the rest
if you want, but you certainly don't have to)?  I apologize for dumping a code
review on you while you're on vacation, but if I'm modifying histogram.cc I
figure I really should get your eyes on it.  I'm just making sure not to
overflow the sum squared accumulator; I got a crash in the later DCHECK during
testing, I believe because I'm using a histogram designed for tracking
timescales of the order of milliseconds and using it for timescales at the order
of days :-{.

http://codereview.chromium.org/2718011/diff/1/2
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/2718011/diff/1/2#newcode237
chrome/browser/net/chrome_url_request_context.cc:237: delete stats_info_;
On 2010/06/11 20:15:00, eroman wrote:
> Generally a safer pattern is to make stats_info_ be a scoped_ptr<>.
> 
> Since it is also possible that a Task will not be Run(), but just deleted. (In
> which case this will result in a leak).

Done.

http://codereview.chromium.org/2718011/diff/1/2#newcode254
chrome/browser/net/chrome_url_request_context.cc:254:
pref->GetInt64(prefs::kUninstallMetricsUptimeSec);
On 2010/06/11 20:15:00, eroman wrote:
> Note that rather than doing this pingpong from the IO thread back over to the
UI
> thread, you can extract the UninstallMetricsUptimeSec as part of
> FactoryOriginal's constructor (which runs on UI thread).
> 
> That said, I have comments in the cookie monster about not doing a strictly
> time-based metrics tracking.

Good point.  Not implementing, since I expect the rewrite to gather the stats in
a less questionable way will take care of it.

http://codereview.chromium.org/2718011/diff/1/4
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/1/4#newcode686
net/base/cookie_monster.cc:686:
UMA_HISTOGRAM_CUSTOM_TIMES("net.CookieExpirationTimes",
On 2010/06/11 20:15:00, eroman wrote:
> I recommend naming these something more like "Duration" as opposed to "Times".

net.CookieExpirationTimes -> net.CookieExpirationDuration
net.CookiesLastAccessTimes -> net.CookieBetweenAccessInterval (Duration didn't
feel right here; let me know if you disagree.)

I'll also go through and unify the names properly as to plural/singular.

http://codereview.chromium.org/2718011/diff/1/4#newcode825
net/base/cookie_monster.cc:825:
UMA_HISTOGRAM_ENUMERATION("net.CookieDeletionCause", kEvicted,
On 2010/06/11 20:15:00, eroman wrote:
> Note that this isn't going to work how you expect.
> 
> The UMA_HISTOGRAM_* macros just create a function-local static variable that
> contains the data.
> 
> Hence if you instantiate the histogram from multiple functions, they will
> actually be separate histograms.
> 
> To fix this, create a helper function so the instantiation only happens from a
> single location.

Done.

http://codereview.chromium.org/2718011/diff/1/4#newcode1191
net/base/cookie_monster.cc:1191: kRecordStatisticsInterval - uptime_seconds %
kRecordStatisticsInterval;
On 2010/06/11 18:58:33, ahendrickson wrote:
> Nit: Can you add parentheses around the mod operation?

Done.

http://codereview.chromium.org/2718011/diff/1/4#newcode1193
net/base/cookie_monster.cc:1193: MessageLoop::current()->PostDelayedTask(
On 2010/06/11 20:15:00, eroman wrote:
> In general I don't like these sort of heartbeat strategies, as they can lead
to
> problems. Here are some of my dislikes:
> 
> * They can lead to spurious work, even in periods of inactivity (i.e. you
aren't
> using chrome but it is still spinning up every now and then).
> 
> * Weird things happen after waking up from sleep (intervals can be missed, and
> timing edges will get clipped).
> 
> * The complexity doesn't scale well when lots of code does this. (Can get
> unpredictable latency spikes when unrelated timed tasks end up overlapping
> because of their periodicity).
> 
> Here are some suggestions for alternative approaches that don't require a
> recurring task (the value being measured is a bit different, but should still
be
> useful):
> 
> (a) Each time a cookie is added/removed, record the new num cookies  in the
> histogram. This histogram will give a sense of how many cookies users have,
> although it will be more heavily weighted towards users that update their
> cookies a lot. You can easily reduce the bias by introducing a maximum update
> perio -- only update the histogram if at least XXX seconds have elapsed since
> the last update.
> 
> (b) Each time cookies are *accessed*, record the current num cookies in the
> histogram. (accesses happen very frequently, and only during periods of
> activity. This approach is biased towards users that browse the web the most.
> Same thing as in (a), you can reduce the bias by only updating if XXX seconds
> have elapsed since the last update.

Compromised; I'm no longer doing heartbeats, but I'm still trying to average
over "active browser" time.  But it was a fairly large rewrite, so I won't try
to describe it in a response comment.

http://codereview.chromium.org/2718011/diff/1/5
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/1/5#newcode271
net/base/cookie_monster.h:271: static const int kRecordStatisticsInterval = 6 *
60 * 60;
On 2010/06/11 18:58:33, ahendrickson wrote:
> Minor nit: I'd like to see kRecordStatisticsInterval as
> kRecordStatisticsIntervalSeconds (or something like that) to make it clear
it's
> in seconds when reading the code.
> 

Done.

http://codereview.chromium.org/2718011/diff/1/5#newcode273
net/base/cookie_monster.h:273: enum DeletionCause { kExplicit, kOverwrite,
kExpired, kEvicted,
On 2010/06/11 18:58:33, ahendrickson wrote:
> Minor nit:  It would be easier to read if the enums were something like
> kDeleteCookieExplicit, kDeleteCookieOverwrite, etc.
> 

Done.

[jar (doing other things), 2010-06-17 02:32:21]

http://codereview.chromium.org/2718011/diff/8001/9001
File base/histogram.cc (right):

http://codereview.chromium.org/2718011/diff/8001/9001#newcode505
base/histogram.cc:505: }
Summary: I'd rather not see the above code landed.

The DCHECK at line 509 that the above code obsoletes was most helpful for
finding situations where garbage (large) data was fed to the histogram.  It
turns out that standard deviation of very large data samples is not stable, so
the actual statistic (sum-of-squares) seems to have little value to anyone....
and we may eventually discard it (make it optional in the protobuffer... and
stop sending it via UMA).  

In your situation, the time data you're feeding in (cookie expiration
interval??) is unusually large, when expressed in counts of milliseconds.  I'd
suggest that you use a different scaling factor and then document what the
counts are.  For instance, I'd suggest using UMA_HISTOGRAM_CUSTOM_COUNTS(), and
passing in the your sampled delta as delta.InMinutes() or such.  
If your delta is still managing to be "too large" for getting sum of squares,
I'd suggest clipping the data (in only your application) to be small enough. 
For example, I personally doubt that you can to distinguish 10 year vs 50 year
expiration times for cookies (assuming I'm correct in guessing what was really
causing the problem with the DCHECK in line 509)

http://codereview.chromium.org/2718011/diff/8001/9002
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/2718011/diff/8001/9002#newcode236
chrome/browser/net/sqlite_persistent_cookie_store.cc:236: succeeded ? 1 : 2, 2);
I'd suggest:
...succeeded ? 1 : 2, 3)

The value supplied as the last argument of this HISTOGRAM macro is supposed to
be larger than any samples you provide.  Alternatively, you could use:

...succeeded ? 0 : 1, 2);

There is a subtle reason why following this convention is often helpful when/if
you change or augment the sample set for this histogram.... but you can just
look as it as a convention.

http://codereview.chromium.org/2718011/diff/8001/9004
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/8001/9004#newcode269
net/base/cookie_monster.h:269: enum DeletionCause { kDeleteCookieExplicit,
kDeleteCookieOverwrite,
style nit: I *think* you should declare only one enumerated value per line.

[eroman, 2010-06-17 19:13:15]

Thanks for making those changes from earlier revision.
Can you also update the CL description accordingly?

http://codereview.chromium.org/2718011/diff/8001/9003
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/8001/9003#newcode685
net/base/cookie_monster.cc:685: cc->ExpiryDate() - current,
What happens to negative time deltas?

http://codereview.chromium.org/2718011/diff/8001/9003#newcode970
net/base/cookie_monster.cc:970: CheckRecordPeriodicStats();
wording nit: In my mind "Check" means "Assert". I would either simply call this
RecordPeriodicStats(), or use "Maybe" in place of "Check".

http://codereview.chromium.org/2718011/diff/8001/9003#newcode1024
net/base/cookie_monster.cc:1024: InternalDeleteCookie(curit, true);
I suggest changing the signature of InternalDeleteCookie(), so it takes a
DeletionCause parameter:

void InternalDeleteCookie(CookieMap::iterator it, bool sync_to_store,
DeletionCause deletion_cause);

And now do the histogram logging as part of InternalDeleteCookie.

This way we can guarantee that the statistic won't be broken with future changes
(as consumers HAVE to consider what parameter to pass in); it is easier to prove
then that we are logging for each of the various entry points to deletion.

http://codereview.chromium.org/2718011/diff/8001/9003#newcode1187
net/base/cookie_monster.cc:1187: void CookieMonster::RecordCookieDeleted(enum
DeletionCause cause) {
nit: omit "enum" (we don't do this in our code style).

http://codereview.chromium.org/2718011/diff/8001/9004
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/8001/9004#newcode278
net/base/cookie_monster.h:278: // Inline because this is probed a lot (every
request for cookies.)
Can you move this code to the .cc file?

We try to avoid inlining code in the header.

From a performance perspective, we rely on whole program optimization on release
builds finding the most suitable functions to be inlined.

http://codereview.chromium.org/2718011/diff/8001/9004#newcode280
net/base/cookie_monster.h:280: static const base::TimeDelta
kRecordStatisticsIntervalTime(
I wouldn't make this static.

http://codereview.chromium.org/2718011/diff/8001/9004#newcode286
net/base/cookie_monster.h:286: UMA_HISTOGRAM_CUSTOM_COUNTS("net.CookieCount",
cookies_.size(),
Not sure what our standard is, but it might be helpful to mention the time
period in the name. For example, "CookieCountPer10Minutes".

[Randy Smith (Not in Mondays), 2010-06-17 20:59:48]

Another round, having incorporated comments and updated the CL description.  Let
me know what you think.

http://codereview.chromium.org/2718011/diff/8001/9001
File base/histogram.cc (right):

http://codereview.chromium.org/2718011/diff/8001/9001#newcode505
base/histogram.cc:505: }
On 2010/06/17 02:32:21, jar wrote:
> Summary: I'd rather not see the above code landed.
> 
> The DCHECK at line 509 that the above code obsoletes was most helpful for
> finding situations where garbage (large) data was fed to the histogram.  It
> turns out that standard deviation of very large data samples is not stable, so
> the actual statistic (sum-of-squares) seems to have little value to anyone....
> and we may eventually discard it (make it optional in the protobuffer... and
> stop sending it via UMA).  
> 
> In your situation, the time data you're feeding in (cookie expiration
> interval??) is unusually large, when expressed in counts of milliseconds.  I'd
> suggest that you use a different scaling factor and then document what the
> counts are.  For instance, I'd suggest using UMA_HISTOGRAM_CUSTOM_COUNTS(),
and
> passing in the your sampled delta as delta.InMinutes() or such.  
> If your delta is still managing to be "too large" for getting sum of squares,
> I'd suggest clipping the data (in only your application) to be small enough. 
> For example, I personally doubt that you can to distinguish 10 year vs 50 year
> expiration times for cookies (assuming I'm correct in guessing what was really
> causing the problem with the DCHECK in line 509)

Done.

http://codereview.chromium.org/2718011/diff/8001/9002
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/2718011/diff/8001/9002#newcode236
chrome/browser/net/sqlite_persistent_cookie_store.cc:236: succeeded ? 1 : 2, 2);
On 2010/06/17 02:32:21, jar wrote:
> I'd suggest:
> ...succeeded ? 1 : 2, 3)
> 
> The value supplied as the last argument of this HISTOGRAM macro is supposed to
> be larger than any samples you provide.  Alternatively, you could use:
> 
> ...succeeded ? 0 : 1, 2);
> 
> There is a subtle reason why following this convention is often helpful
when/if
> you change or augment the sample set for this histogram.... but you can just
> look as it as a convention.

Done.  

When you have a chance (low priority), could you explain the subtle reason?  I'm
happy to follow convention, but I'll have an easier time remembering if I also
know the reason.

http://codereview.chromium.org/2718011/diff/8001/9003
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/8001/9003#newcode685
net/base/cookie_monster.cc:685: cc->ExpiryDate() - current,
On 2010/06/17 19:13:15, eroman wrote:
> What happens to negative time deltas?

They get taken as zero.  All of this stuff converges on Histogram::Add, which
takes an int, and checks if the value is < 0, and if it is, sets it to zero.

Note also that in the non-from-backing-store-init path into this function, we
bail if the cookie is expired, which is the only way that this code would see a
negative time delta.  We might still see negative time deltas from
initialization from the store, but now that my attention's called to that, I
don't actually want to record expiry times when we're initializing from the
store, as that'll be double-recorded.  I'll fix that too.

http://codereview.chromium.org/2718011/diff/8001/9003#newcode970
net/base/cookie_monster.cc:970: CheckRecordPeriodicStats();
On 2010/06/17 19:13:15, eroman wrote:
> wording nit: In my mind "Check" means "Assert". I would either simply call
this
> RecordPeriodicStats(), or use "Maybe" in place of "Check".

Done.

http://codereview.chromium.org/2718011/diff/8001/9003#newcode1024
net/base/cookie_monster.cc:1024: InternalDeleteCookie(curit, true);
On 2010/06/17 19:13:15, eroman wrote:
> I suggest changing the signature of InternalDeleteCookie(), so it takes a
> DeletionCause parameter:
> 
> void InternalDeleteCookie(CookieMap::iterator it, bool sync_to_store,
> DeletionCause deletion_cause);
> 
> And now do the histogram logging as part of InternalDeleteCookie.
> 
> This way we can guarantee that the statistic won't be broken with future
changes
> (as consumers HAVE to consider what parameter to pass in); it is easier to
prove
> then that we are logging for each of the various entry points to deletion.

Good catch; thanks.  Done.

http://codereview.chromium.org/2718011/diff/8001/9003#newcode1187
net/base/cookie_monster.cc:1187: void CookieMonster::RecordCookieDeleted(enum
DeletionCause cause) {
On 2010/06/17 19:13:15, eroman wrote:
> nit: omit "enum" (we don't do this in our code style).

Done.

http://codereview.chromium.org/2718011/diff/8001/9004
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/8001/9004#newcode269
net/base/cookie_monster.h:269: enum DeletionCause { kDeleteCookieExplicit,
kDeleteCookieOverwrite,
On 2010/06/17 02:32:21, jar wrote:
> style nit: I *think* you should declare only one enumerated value per line.

Done.

(I couldn't find it in the style guide, but I may not have looked in the right
place.)

http://codereview.chromium.org/2718011/diff/8001/9004#newcode278
net/base/cookie_monster.h:278: // Inline because this is probed a lot (every
request for cookies.)
On 2010/06/17 19:13:15, eroman wrote:
> Can you move this code to the .cc file?
> 
> We try to avoid inlining code in the header.
> 
> From a performance perspective, we rely on whole program optimization on
release
> builds finding the most suitable functions to be inlined.

Done.

http://codereview.chromium.org/2718011/diff/8001/9004#newcode280
net/base/cookie_monster.h:280: static const base::TimeDelta
kRecordStatisticsIntervalTime(
On 2010/06/17 19:13:15, eroman wrote:
> I wouldn't make this static.

Done, but can you say a little bit more as to why?  I think if it's not static,
even though it's const, it'll be recomputed every time we hit the function,
which seems wasteful.

http://codereview.chromium.org/2718011/diff/8001/9004#newcode286
net/base/cookie_monster.h:286: UMA_HISTOGRAM_CUSTOM_COUNTS("net.CookieCount",
cookies_.size(),
On 2010/06/17 19:13:15, eroman wrote:
> Not sure what our standard is, but it might be helpful to mention the time
> period in the name. For example, "CookieCountPer10Minutes".

Unless you feel really strongly about this, I'd rather not.  Our standard, as I
understand it, is that the units that are to be stored should be included in the
name, and they are; we're storing number of cookies.  The fact that we're
storing them every ten minutes isn't directly useful in interpreting the data. 
It's indirectly useful (it controls the universe of browsers we're collecting
from in the time domain), but mostly we're asking "What's the distribution of
cookies across some a representative set of browsers?" and the details of that
set are (IMO) substantially less interesting.

[eroman, 2010-06-18 01:13:56]

lgtm with following comments.

http://codereview.chromium.org/2718011/diff/18001/19002
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/18001/19002#newcode74
net/base/cookie_monster.cc:74: #define UMA_HISTOGRAM_MINUTES(name, sample) \
I discourage this style of naming, unless you are going to move it into
histogram.h.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode75
net/base/cookie_monster.cc:75: UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 10 *
365 * 24 * 60, 50)
Can you mention in the comment this measures up to 10 years?

http://codereview.chromium.org/2718011/diff/18001/19002#newcode120
net/base/cookie_monster.cc:120: last_statistic_record_time_(Time::Now()) {
I don't think we need to be calling Now() here, as the default 'null' time
should work fine for our purposes.

(The default Time constructor will put it at 0, so we will definitely be over
the interval on the very first call).

Or was it intentional to not record the stats for the first 10 minutes?

http://codereview.chromium.org/2718011/diff/18001/19002#newcode696
net/base/cookie_monster.cc:696: const Time current = Time::Now();
This appears to be unused. Can you remove?

http://codereview.chromium.org/2718011/diff/18001/19002#newcode717
net/base/cookie_monster.cc:717: (current - cc->LastAccessDate()).InMinutes());
I wander if the .InMinutes() can't be done by the macro? (so just need to pass
in a TimeDelta).

http://codereview.chromium.org/2718011/diff/18001/19002#newcode871
net/base/cookie_monster.cc:871: kDeleteCookieDontRecord /* Destruction. */);
indent another 4.

(that second case is a bit tricky).

http://codereview.chromium.org/2718011/diff/18001/19002#newcode974
net/base/cookie_monster.cc:974: // Probe to save statistics relatively
frequently.  Here rather than in
Can you start the second phrase with: "We do it here" ...

http://codereview.chromium.org/2718011/diff/18001/19002#newcode976
net/base/cookie_monster.cc:976: // statistics whenver the browser's being used.
typo: whenever.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1194
net/base/cookie_monster.cc:1194: // kRecordStatisticsIntervalSeconds
nit: end with period.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1195
net/base/cookie_monster.cc:1195: // Inline because this is probed a lot (every
request for cookies.)
Remove this comment, no longer applicable.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1196
net/base/cookie_monster.cc:1196: void CookieMonster::RecordPeriodicStats(void) {
nit: we generally omit "void" in parameter list.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1199
net/base/cookie_monster.cc:1199: base::Time current(base::Time::Now());
very minor nit: can you make this "const" ?

http://codereview.chromium.org/2718011/diff/18001/19003
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/18001/19003#newcode74
net/base/cookie_monster.h:74: last_statistic_record_time_(base::Time::Now()) {
same comment as before -- can you remove the initialization to Now() ?

http://codereview.chromium.org/2718011/diff/18001/19003#newcode242
net/base/cookie_monster.h:242: // deletion_cause argument is for collecting
statistics
nit:  |deletion_cause| (to be consistent with rest of this file). Also please
end the comment with a period.

[jar (doing other things), 2010-06-18 18:48:12]

Small changes listed, relating heavily to ERoman's comments.

http://codereview.chromium.org/2718011/diff/18001/19002
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/18001/19002#newcode74
net/base/cookie_monster.cc:74: #define UMA_HISTOGRAM_MINUTES(name, sample) \
+1.  I'd rather see this inline expanded, than to define another macro (which
most readers would assume to be part of the histogram.h file).

On 2010/06/18 01:13:57, eroman wrote:
> I discourage this style of naming, unless you are going to move it into
> histogram.h.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode75
net/base/cookie_monster.cc:75: UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 10 *
365 * 24 * 60, 50)
I'd suggest you define a constant like:
kMinutesInTenYears

That constant will make it much clearer when you write out the macro inline.

http://codereview.chromium.org/2718011/diff/18001/19003
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/18001/19003#newcode280
net/base/cookie_monster.h:280: // See if it's time to record stats.
nit: Current comment reads almost like this function should return a bool.
Better comment might be:

This function can be called repeatedly, and will record statistics if a
sufficient time period has elapsed.

[Randy Smith (Not in Mondays), 2010-06-18 19:20:30]

Next round CL.  Eric, I've pushed back on a couple of your comments, so if you'd
be willing to give a second lgtm after reading through the response (or not :-})
I'd be grateful.  You can probably skip looking at the code if you want--the
comments describe where I've done things differently than you suggested.

Thanks all for dealing with the churn on this CL; hopefully my next statistics
commit will be simpler :-J.

http://codereview.chromium.org/2718011/diff/18001/19002
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/18001/19002#newcode74
net/base/cookie_monster.cc:74: #define UMA_HISTOGRAM_MINUTES(name, sample) \
On 2010/06/18 18:48:12, jar wrote:
> +1.  I'd rather see this inline expanded, than to define another macro (which
> most readers would assume to be part of the histogram.h file).
> 
> On 2010/06/18 01:13:57, eroman wrote:
> > I discourage this style of naming, unless you are going to move it into
> > histogram.h.

That probably makes more sense.  Ok, done (I believe this satisfies both
comments.)

http://codereview.chromium.org/2718011/diff/18001/19002#newcode75
net/base/cookie_monster.cc:75: UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 10 *
365 * 24 * 60, 50)
On 2010/06/18 18:48:12, jar wrote:
> I'd suggest you define a constant like:
> kMinutesInTenYears
> 
> That constant will make it much clearer when you write out the macro inline.

Done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode120
net/base/cookie_monster.cc:120: last_statistic_record_time_(Time::Now()) {
On 2010/06/18 01:13:57, eroman wrote:
> I don't think we need to be calling Now() here, as the default 'null' time
> should work fine for our purposes.
> 
> (The default Time constructor will put it at 0, so we will definitely be over
> the interval on the very first call).
> 
> Or was it intentional to not record the stats for the first 10 minutes?

It was, actually, and even though this CL should be subtitled "Randy trips all
over his own cleverness" :-J I'd like to keep that behavior.  The ten minute
sampling as being used as conceptual proxy for continuous infinitesimal sampling
while the browser is active, and I'm worried about selection bias if we assume
the browser will be active whenever its started up.  For instance, I have Chrome
in my login init for both my laptops, and I don't necessarily immediately start
browsing.  So I'd like to keep this unless you see a specific minus to it being
"Now()" instead of the null time.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode696
net/base/cookie_monster.cc:696: const Time current = Time::Now();
On 2010/06/18 01:13:57, eroman wrote:
> This appears to be unused. Can you remove?

Whoops.  Done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode717
net/base/cookie_monster.cc:717: (current - cc->LastAccessDate()).InMinutes());
On 2010/06/18 01:13:57, eroman wrote:
> I wander if the .InMinutes() can't be done by the macro? (so just need to pass
> in a TimeDelta).

Superseded by the inlining suggested by jar (but I liked the idea.)

http://codereview.chromium.org/2718011/diff/18001/19002#newcode871
net/base/cookie_monster.cc:871: kDeleteCookieDontRecord /* Destruction. */);
On 2010/06/18 01:13:57, eroman wrote:
> indent another 4.
> 
> (that second case is a bit tricky).

Done.  Agreed on the second case; I flip-flopped between doing it this way or
having a separate enum bucket, but that bucket would be pretty full and thus 
distracting when we view it back here.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode974
net/base/cookie_monster.cc:974: // Probe to save statistics relatively
frequently.  Here rather than in
On 2010/06/18 01:13:57, eroman wrote:
> Can you start the second phrase with: "We do it here" ...

Done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode976
net/base/cookie_monster.cc:976: // statistics whenver the browser's being used.
On 2010/06/18 01:13:57, eroman wrote:
> typo: whenever.

Whoops; done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1194
net/base/cookie_monster.cc:1194: // kRecordStatisticsIntervalSeconds
On 2010/06/18 01:13:57, eroman wrote:
> nit: end with period.

Done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1195
net/base/cookie_monster.cc:1195: // Inline because this is probed a lot (every
request for cookies.)
On 2010/06/18 01:13:57, eroman wrote:
> Remove this comment, no longer applicable.

Done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1196
net/base/cookie_monster.cc:1196: void CookieMonster::RecordPeriodicStats(void) {
On 2010/06/18 01:13:57, eroman wrote:
> nit: we generally omit "void" in parameter list.

Done.

http://codereview.chromium.org/2718011/diff/18001/19002#newcode1199
net/base/cookie_monster.cc:1199: base::Time current(base::Time::Now());
On 2010/06/18 01:13:57, eroman wrote:
> very minor nit: can you make this "const" ?

Done.

http://codereview.chromium.org/2718011/diff/18001/19003
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/18001/19003#newcode74
net/base/cookie_monster.h:74: last_statistic_record_time_(base::Time::Now()) {
On 2010/06/18 01:13:57, eroman wrote:
> same comment as before -- can you remove the initialization to Now() ?

Same preference to keep as before.

http://codereview.chromium.org/2718011/diff/18001/19003#newcode242
net/base/cookie_monster.h:242: // deletion_cause argument is for collecting
statistics
On 2010/06/18 01:13:57, eroman wrote:
> nit:  |deletion_cause| (to be consistent with rest of this file). Also please
> end the comment with a period.

Done.

http://codereview.chromium.org/2718011/diff/18001/19003#newcode280
net/base/cookie_monster.h:280: // See if it's time to record stats.
On 2010/06/18 18:48:12, jar wrote:
> nit: Current comment reads almost like this function should return a bool.
> Better comment might be:
> 
> This function can be called repeatedly, and will record statistics if a
> sufficient time period has elapsed.

Done.

[eroman, 2010-06-18 20:05:34]

LGTM

http://codereview.chromium.org/2718011/diff/18001/19002
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2718011/diff/18001/19002#newcode120
net/base/cookie_monster.cc:120: last_statistic_record_time_(Time::Now()) {
On 2010/06/18 19:20:31, rdsmith wrote:
> On 2010/06/18 01:13:57, eroman wrote:
> > I don't think we need to be calling Now() here, as the default 'null' time
> > should work fine for our purposes.
> > 
> > (The default Time constructor will put it at 0, so we will definitely be
over
> > the interval on the very first call).
> > 
> > Or was it intentional to not record the stats for the first 10 minutes?
> 
> It was, actually, and even though this CL should be subtitled "Randy trips all
> over his own cleverness" :-J I'd like to keep that behavior.  The ten minute
> sampling as being used as conceptual proxy for continuous infinitesimal
sampling
> while the browser is active, and I'm worried about selection bias if we assume
> the browser will be active whenever its started up.  For instance, I have
Chrome
> in my login init for both my laptops, and I don't necessarily immediately
start
> browsing.  So I'd like to keep this unless you see a specific minus to it
being
> "Now()" instead of the null time.
> 
> 

Fair enough.

I recommend mentioning that in a comment somewhere (perhaps as part of
RecordPeriodicStats).

Another half-baked idea I had (altho it measures a bit differently) would be to
let the Time get default-constructed, and then in RecordPeriodicStats() test for
the initial update:

if (last_statsic_record_time_.is_null()) {
  // We won't record statistics for the first kRecordStatisticsIntervalSeconds.
  last_statistic_record_time_ = base::Time::Now();
}

I am cool with any approach.

[jar (doing other things), 2010-06-18 21:35:22]

All my comments were addressed... so LGTM (but be sure ERoman gives his LGTM
too).

Thanks!

[Randy Smith (Not in Mondays), 2010-06-20 02:28:34]

Sorry, one more round.  I'm good with just comments from Eric on this one.

I noticed a performance degradation in the CM performance tests (see below;
query_many_hosts was the one that bothered me), I believe due to calling Now()
in the critical path.  I moved RecordPeriodicStats() to a different place on the
cookie retrieval path, and switched computation of the expiry time to be based
on the cookie creation time (already computed, generally from Now()) rather than
directly from Now().  I also put in a comment explaining why I was initializing
last_statistic_recorded_time_ to Now() rather than Null.

Performance results from running net_perftests
--gtest_filter=CookieMonsterTest.* in Release mode:


                                      Trunk     PS4             PS5
Cookie_monster_add_single_host        99.204    110.888         100.291
Cookie_monster_query_single_host      802.299   816.071         811.674
Cookie_monster_deleteall_single_host  0.008     0.009           0.009
Cookie_monster_add_many_hosts         111.749   122.416         113.626
Cookie_monster_query_many_hosts       47.144    60.018          48.633
Cookie_monster_deleteall_many_hosts   0.439     0.518           1.879

[eroman, 2010-06-21 18:20:34]

Actually, I suggest using TimeTicks::Now() here rather than Time::Now().
TimeTicks::Now() is a bit faster, but more importantly it won't break if the
user updates their system clock once chrome is already running (i.e. if they
were to wind the system clock backwards, right now that would stop the periodic
stats from recording).

As an aside, I think we have this same issue in other parts of CookieMonster.
CookieMonster::CurrentTime() is pretty weird, and I did a quick search of
Time::Now() and saw it in a couple places.


Still LGTM tho.

[eroman, 2010-06-21 18:22:43]

http://codereview.chromium.org/2718011/diff/28002/34003
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/28002/34003#newcode283
net/base/cookie_monster.h:283: void RecordPeriodicStats(const base::Time
&current_time);
style nit:  |const base::Time& current_time|

[Randy Smith (Not in Mondays), 2010-06-21 20:47:44]

On 2010/06/21 18:20:34, eroman wrote:
> Actually, I suggest using TimeTicks::Now() here rather than Time::Now().
> TimeTicks::Now() is a bit faster, but more importantly it won't break if the
> user updates their system clock once chrome is already running (i.e. if they
> were to wind the system clock backwards, right now that would stop the
periodic
> stats from recording).
> 
> As an aside, I think we have this same issue in other parts of CookieMonster.
> CookieMonster::CurrentTime() is pretty weird, and I did a quick search of
> Time::Now() and saw it in a couple places.
> 
> 
> Still LGTM tho.

Long term that makes sense, but it should probably be done rethinking how the
various Now() calls are used within the CookieMonster.  If I do it now I lose
the ability to piggy-back on the existing calls to Time::Now() and my
performance degradation comes back.

[darin (slow to review), 2010-07-02 16:35:00]

http://codereview.chromium.org/2718011/diff/28002/34003
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2718011/diff/28002/34003#newcode235
net/base/cookie_monster.h:235: enum DeletionCause { kDeleteCookieExplicit,
"Though the style guide says to use kConstantNaming for enums now, we still use
MACRO_STYLE naming for enums for consistency with existing code."

Please see http://dev.chromium.org/developers/coding-style