Extend Histogram class to support custom range definitions...

Extend Histogram class to support custom range definitions

Some users have a small sparsely separated enumerated list
of plausible samples which don't fit well into a default
(log space) histogarm, or a consistently spaced (LinearHistogram).

This implementation does not yet support renderer histograms of this
sort, but it at least unblocks the dependent bug that needs support
for these sparse histograms in the browser.

The bulk of this patch was written by Raman Tenetti, in CL 1706012.

BUG=40953
r=eroman

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

[jar (doing other things), 2010-04-28 18:00:51]

Eric,

Since you were most interested in this, I was going to ask you to do the review.
 Note that this CL doesn't support renderer side histograms of this type, but
that shouldn't (I think) be an issue with your pressing need (histograms of
browser network errors).

Thanks,

Jim

[eroman, 2010-04-28 18:21:35]

LGTM.

Mostly just nits, my main comment is:
http://codereview.chromium.org/1737017/diff/5/5003#newcode745

http://codereview.chromium.org/1737017/diff/5/5003
File base/histogram.cc (right):

http://codereview.chromium.org/1737017/diff/5/5003#newcode704
base/histogram.cc:704: ranges.push_back(0);  // Ensure we have a zero value.
why do we need this?

http://codereview.chromium.org/1737017/diff/5/5003#newcode708
base/histogram.cc:708: if (ranges.size() == 1)
We just DCHECKed above that ranges has more than 1 element, why this check?

http://codereview.chromium.org/1737017/diff/5/5003#newcode711
base/histogram.cc:711: DCHECK(ranges.size() < kSampleType_MAX);
DCHECK_LT (might need to cast one of those in case there is a signed/unsigned
mismatch).

http://codereview.chromium.org/1737017/diff/5/5003#newcode714
base/histogram.cc:714: DCHECK(histogram.get() != NULL);
nit: I suggest omitting "!= NULL"

http://codereview.chromium.org/1737017/diff/5/5003#newcode715
base/histogram.cc:715: DCHECK(CUSTOM_HISTOGRAM == histogram->histogram_type());
DCHECK_EQ

http://codereview.chromium.org/1737017/diff/5/5003#newcode720
base/histogram.cc:720: if (registered_histogram.get() != NULL &&
nit: I suggest omitting "!= NULL"

http://codereview.chromium.org/1737017/diff/5/5003#newcode725
base/histogram.cc:725: DCHECK(CUSTOM_HISTOGRAM == histogram->histogram_type());
DCHECK_EQ

http://codereview.chromium.org/1737017/diff/5/5003#newcode735
base/histogram.cc:735: custom_ranges[1],
Please indent this to line up with the opening paren of the "Histogram(" line.

http://codereview.chromium.org/1737017/diff/5/5003#newcode738
base/histogram.cc:738: DCHECK_GT(custom_ranges.size(), 1u);
nit: can you reverse the ordering? (expectation first, then actual).

http://codereview.chromium.org/1737017/diff/5/5003#newcode739
base/histogram.cc:739: DCHECK_EQ(custom_ranges[0], 0);
ditto.

http://codereview.chromium.org/1737017/diff/5/5003#newcode745
base/histogram.cc:745: void CustomHistogram::InitializeBucketRange() {
Can you call:
    ranges_vector_.clear();

at the end of this function?

It looks like ranges_vector_ is only a member variable so it can be consumed by
InitializeBucketRange(), but after this initialization has happened there is no
point keeping the extra memory around.

http://codereview.chromium.org/1737017/diff/5/5001
File base/histogram.h (right):

http://codereview.chromium.org/1737017/diff/5/5001#newcode211
base/histogram.h:211: class CustomHistogram;
nit: can you sort this forward declaration section?

http://codereview.chromium.org/1737017/diff/5/5001#newcode550
base/histogram.h:550: class CustomHistogram : public Histogram {
I think this name could be improved, but I don't have any good recommendations.

http://codereview.chromium.org/1737017/diff/5/5001#newcode561
base/histogram.h:561: virtual ~CustomHistogram() {}
nit: I would suggest omitting this dtor.

http://codereview.chromium.org/1737017/diff/5/5002
File base/histogram_unittest.cc (right):

http://codereview.chromium.org/1737017/diff/5/5002#newcode222
base/histogram_unittest.cc:222: // Don't include a zero.
I am not sure I understand why we always need to have a bucket for 0. Is this
used as the overflow bucket or something?

[jar (doing other things), 2010-04-28 19:07:50]

Changes per suggestions by eroman.

http://codereview.chromium.org/1737017/diff/5/5003
File base/histogram.cc (right):

http://codereview.chromium.org/1737017/diff/5/5003#newcode704
base/histogram.cc:704: ranges.push_back(0);  // Ensure we have a zero value.
On 2010/04/28 18:21:36, eroman wrote:
> why do we need this?

As currently defined, histograms (of all sorts) support any positive value as
samples.  This is an invariant that is used in the base class at various times,
and CHECK()'ed explicitly, so rather than trying to re-architect the base class,
it seemed easier to support this "feature."

http://codereview.chromium.org/1737017/diff/5/5003#newcode711
base/histogram.cc:711: DCHECK(ranges.size() < kSampleType_MAX);
On 2010/04/28 18:21:36, eroman wrote:
> DCHECK_LT (might need to cast one of those in case there is a signed/unsigned
> mismatch).

Done.

http://codereview.chromium.org/1737017/diff/5/5003#newcode714
base/histogram.cc:714: DCHECK(histogram.get() != NULL);
On 2010/04/28 18:21:36, eroman wrote:
> nit: I suggest omitting "!= NULL"

Done.

http://codereview.chromium.org/1737017/diff/5/5003#newcode715
base/histogram.cc:715: DCHECK(CUSTOM_HISTOGRAM == histogram->histogram_type());
On 2010/04/28 18:21:36, eroman wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/1737017/diff/5/5003#newcode720
base/histogram.cc:720: if (registered_histogram.get() != NULL &&
On 2010/04/28 18:21:36, eroman wrote:
> nit: I suggest omitting "!= NULL"

Done.

http://codereview.chromium.org/1737017/diff/5/5003#newcode725
base/histogram.cc:725: DCHECK(CUSTOM_HISTOGRAM == histogram->histogram_type());
On 2010/04/28 18:21:36, eroman wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/1737017/diff/5/5003#newcode735
base/histogram.cc:735: custom_ranges[1],
On 2010/04/28 18:21:36, eroman wrote:
> Please indent this to line up with the opening paren of the "Histogram(" line.

Done.

http://codereview.chromium.org/1737017/diff/5/5003#newcode738
base/histogram.cc:738: DCHECK_GT(custom_ranges.size(), 1u);
On 2010/04/28 18:21:36, eroman wrote:
> nit: can you reverse the ordering? (expectation first, then actual).

I at least changed the file to be consistent.

http://codereview.chromium.org/1737017/diff/5/5003#newcode745
base/histogram.cc:745: void CustomHistogram::InitializeBucketRange() {
I switched to using a pointer as a member, and just pointed to the stack version
during the initialization.  This avoids saving the data, and avoids the array
copy as well.

On 2010/04/28 18:21:36, eroman wrote:
> Can you call:
>     ranges_vector_.clear();
> 
> at the end of this function?
> 
> It looks like ranges_vector_ is only a member variable so it can be consumed
by
> InitializeBucketRange(), but after this initialization has happened there is
no
> point keeping the extra memory around.

http://codereview.chromium.org/1737017/diff/5/5001
File base/histogram.h (right):

http://codereview.chromium.org/1737017/diff/5/5001#newcode211
base/histogram.h:211: class CustomHistogram;
On 2010/04/28 18:21:36, eroman wrote:
> nit: can you sort this forward declaration section?

Done.

http://codereview.chromium.org/1737017/diff/5/5001#newcode550
base/histogram.h:550: class CustomHistogram : public Histogram {
On 2010/04/28 18:21:36, eroman wrote:
> I think this name could be improved, but I don't have any good
recommendations.

I've thought of using the word "sparse," but I have expectations that there will
be a "sparse dynamic enumerated" histogram in our future, where the ranges will
not be specified at declaration time, but rather will be implied as unique
samples.  As a result, I'm wary of using that term :-/.

The pattern in these class names seems to be xxxxxHistogram, so the question is
what to make xxxxx without being too verbose.  I'll think about it some more....
but I too don't have a better idea at this time.

http://codereview.chromium.org/1737017/diff/5/5001#newcode561
base/histogram.h:561: virtual ~CustomHistogram() {}
On 2010/04/28 18:21:36, eroman wrote:
> nit: I would suggest omitting this dtor.

Done. (and also deleted in LinearHistogram)

http://codereview.chromium.org/1737017/diff/5/5002
File base/histogram_unittest.cc (right):

http://codereview.chromium.org/1737017/diff/5/5002#newcode222
base/histogram_unittest.cc:222: // Don't include a zero.
On 2010/04/28 18:21:36, eroman wrote:
> I am not sure I understand why we always need to have a bucket for 0. Is this
> used as the overflow bucket or something?

It is indeed part of the "catch all".  Any positive number below the min gets
tossed in here, and any giant positive number goes into the highest bucket.

[eroman, 2010-04-28 19:41:57]

lgtm

[jar (doing other things), 2010-04-28 19:46:53]

One comment response to eroman was somehow lost... so I entered it again below.

http://codereview.chromium.org/1737017/diff/5/5003
File base/histogram.cc (right):

http://codereview.chromium.org/1737017/diff/5/5003#newcode708
base/histogram.cc:708: if (ranges.size() == 1)
On 2010/04/28 18:21:36, eroman wrote:
> We just DCHECKed above that ranges has more than 1 element, why this check?

This was meant to be defensive coding.  I moved around the DCHECK to make this
clearer (and added a comment).