Improve the display of LoadLogs when truncation occurs....

Improve the display of LoadLogs when truncation occurs.
Rather than drop all subsequent entries, we now preserve the final entry that was appended to the log.
This way, even if entries have been dropped, we can still infer what the total time was, and what the exit condition was.

Also makes LoadLog take the bound as a required parameter.

BUG=none
TEST=LoadLogUtilTest.DisplayOfTruncated


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

[eroman, 2009-11-05 07:37:14]

http://codereview.chromium.org/363025/diff/3001/4006
File net/base/load_log.h (right):

http://codereview.chromium.org/363025/diff/3001/4006#newcode59
Line 59: LoadLog(size_t max_num_entries = 40);
our style guide does not permit default arguments... should i change all of the
callers (basically unit-tests) to provide a default instead?

[willchan no longer on Chromium, 2009-11-05 22:59:01]

http://codereview.chromium.org/363025/diff/3001/4006
File net/base/load_log.h (right):

http://codereview.chromium.org/363025/diff/3001/4006#newcode59
Line 59: LoadLog(size_t max_num_entries = 40);
On 2009/11/05 07:37:15, eroman wrote:
> our style guide does not permit default arguments... should i change all of
the
> callers (basically unit-tests) to provide a default instead?

Yeah, you should probably do that.  You could also create a TestLoadLog or
something in load_log_unittest.h that used a default.

[willchan no longer on Chromium, 2009-11-05 23:13:26]

Can we not just increase the number of entries we use for LoadLog?  It looks
like each Entry is 16 bytes or so.  If we had like 40 entries, that'd still only
be 640 bytes per LoadLog.  Let's say there are like 50 concurrent URLRequests
(sounds high to me), that's like 20 something kb.  I think that's small enough
to be mostly negligible.  If we needed to save space, we could easily do so by
not using ints for EventType and EventPhase, instead packing them into int16. 
That'd save a quarter of the space.

http://codereview.chromium.org/363025/diff/3001/4002
File net/base/load_log.cc (right):

http://codereview.chromium.org/363025/diff/3001/4002#newcode7
Line 7: #include "net/base/load_log.h"
load_log.h should be first, like the example given in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_....

http://codereview.chromium.org/363025/diff/3001/4002#newcode30
Line 30: events_.reserve(std::min(static_cast<size_t>(20), max_num_entries_ /
2));
Is 20 a good default?  It looks like our LoadLog now already exceeds 20 entries
when nothing is in cache.

http://codereview.chromium.org/363025/diff/3001/4006
File net/base/load_log.h (right):

http://codereview.chromium.org/363025/diff/3001/4006#newcode59
Line 59: LoadLog(size_t max_num_entries = 40);
On 2009/11/05 22:59:01, willchan wrote:
> On 2009/11/05 07:37:15, eroman wrote:
> > our style guide does not permit default arguments... should i change all of
> the
> > callers (basically unit-tests) to provide a default instead?
> 
> Yeah, you should probably do that.  You could also create a TestLoadLog or
> something in load_log_unittest.h that used a default.

Oh, and the constructor should also then be explicit.

[eroman, 2009-11-06 02:39:02]

http://codereview.chromium.org/363025/diff/3001/4002
File net/base/load_log.cc (right):

http://codereview.chromium.org/363025/diff/3001/4002#newcode30
Line 30: events_.reserve(std::min(static_cast<size_t>(20), max_num_entries_ /
2));
On 2009/11/05 23:13:26, willchan wrote:
> Is 20 a good default?  It looks like our LoadLog now already exceeds 20
entries
> when nothing is in cache.

I changed this to a constant, 10.
(it was just a minor optimization to avoid reallocating too often while growing
the vector).

http://codereview.chromium.org/363025/diff/3001/4006
File net/base/load_log.h (right):

http://codereview.chromium.org/363025/diff/3001/4006#newcode59
Line 59: LoadLog(size_t max_num_entries = 40);
On 2009/11/05 23:13:26, willchan wrote:
> On 2009/11/05 22:59:01, willchan wrote:
> > On 2009/11/05 07:37:15, eroman wrote:
> > > our style guide does not permit default arguments... should i change all
of
> > the
> > > callers (basically unit-tests) to provide a default instead?
> > 
> > Yeah, you should probably do that.  You could also create a TestLoadLog or
> > something in load_log_unittest.h that used a default.
> 
> Oh, and the constructor should also then be explicit.

Done.

[eroman, 2009-11-06 02:57:12]

OK, i made 1 more change, to allow passing in -1 to mean "unbounded".

Ready for review again.

[eroman, 2009-11-06 07:20:20]

Made the suggested change to use a constant in place of -1.

[willchan no longer on Chromium, 2009-11-06 18:38:48]

lgtm

http://codereview.chromium.org/363025/diff/7038/7040
File net/base/load_log.cc (right):

http://codereview.chromium.org/363025/diff/7038/7040#newcode12
Line 12: DCHECK(max_num_entries == -1 || max_num_entries > 0);
How about DCHECK_GE(max_num_entries, -1)?

[eroman, 2009-11-06 19:16:26]

http://codereview.chromium.org/363025/diff/7038/7040
File net/base/load_log.cc (right):

http://codereview.chromium.org/363025/diff/7038/7040#newcode12
Line 12: DCHECK(max_num_entries == -1 || max_num_entries > 0);
On 2009/11/06 18:38:48, willchan wrote:
> How about DCHECK_GE(max_num_entries, -1)?

You must have reviewed an old patchset; in the latest this is:

DCHECK_GT(max_num_entries, 0u);

[willchan no longer on Chromium, 2009-11-06 19:31:19]

Heh, that was an old cached draft comment from last night :P  Sounds good then.

On Fri, Nov 6, 2009 at 11:16 AM,  <eroman@chromium.org> wrote:
>
> http://codereview.chromium.org/363025/diff/7038/7040
> File net/base/load_log.cc (right):
>
> http://codereview.chromium.org/363025/diff/7038/7040#newcode12
> Line 12: DCHECK(max_num_entries == -1 || max_num_entries > 0);
> On 2009/11/06 18:38:48, willchan wrote:
>>
>> How about DCHECK_GE(max_num_entries, -1)?
>
> You must have reviewed an old patchset; in the latest this is:
>
> DCHECK_GT(max_num_entries, 0u);
>
> http://codereview.chromium.org/363025
>