[UMA] Remove redundant bucket_count variable from base::Histogram.

[UMA] Remove redundant bucket_count variable from base::Histogram.

BUG=248623
TEST=none
R=jar@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208917

[Alexei Svitkine (slow), 2013-06-20 05:53:32]

https://codereview.chromium.org/17451016/diff/1/base/metrics/bucket_ranges.h
File base/metrics/bucket_ranges.h (right):

https://codereview.chromium.org/17451016/diff/1/base/metrics/bucket_ranges.h#...
base/metrics/bucket_ranges.h:46: size_t BucketCount() const { return
ranges_.size() - 1; }
Drive by nit:

This should probably either use hacker_style or be called GetBucketCount() and
be defined out of line.

[Ilya Sherman, 2013-06-20 05:57:34]

https://codereview.chromium.org/17451016/diff/1/base/metrics/bucket_ranges.h
File base/metrics/bucket_ranges.h (right):

https://codereview.chromium.org/17451016/diff/1/base/metrics/bucket_ranges.h#...
base/metrics/bucket_ranges.h:46: size_t BucketCount() const { return
ranges_.size() - 1; }
On 2013/06/20 05:53:32, Alexei Svitkine wrote:
> Drive by nit:
> 
> This should probably either use hacker_style or be called GetBucketCount() and
> be defined out of line.

I don't have a strong preference.  Jim, which of these would you prefer?

[jar (doing other things), 2013-06-20 16:41:18]

https://codereview.chromium.org/17451016/diff/1/base/metrics/bucket_ranges.h
File base/metrics/bucket_ranges.h (right):

https://codereview.chromium.org/17451016/diff/1/base/metrics/bucket_ranges.h#...
base/metrics/bucket_ranges.h:46: size_t BucketCount() const { return
ranges_.size() - 1; }
On 2013/06/20 05:57:34, Ilya Sherman wrote:
> On 2013/06/20 05:53:32, Alexei Svitkine wrote:
> > Drive by nit:
> > 
> > This should probably either use hacker_style or be called GetBucketCount()
and
> > be defined out of line.
> 
> I don't have a strong preference.  Jim, which of these would you prefer?

I was a little tentative about the cost... but I guess size() on a vector should
be super fast, and I shouldn't sweat it.  The place where it *might* be an issue
is when we are looking for corruption, and we use this call to find the max
bound (time and again).  I guess I should wait until we have a profiler telling
me this was a hot spot.


I'm fine with retaining hacker style, and putting this in the header as you did.

https://codereview.chromium.org/17451016/diff/1/base/metrics/statistics_recor...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/17451016/diff/1/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:41:
StatisticsRecorder::RegisterOrDeleteDuplicateRanges(ranges);
This tries to de-dup a set of ranges that are fully fleshed out.

It is not clear how we could have fleshed out the ranges if we haven't even
supplied the min and max values (that were given to us as arguments).

Perhaps you need to call:
 base::Histogram::InitializeBucketRanges(min, max, ranges)
after line 39 in onder to set these up.

[Ilya Sherman, 2013-06-21 04:03:05]

https://chromiumcodereview.appspot.com/17451016/diff/1/base/metrics/bucket_ra...
File base/metrics/bucket_ranges.h (right):

https://chromiumcodereview.appspot.com/17451016/diff/1/base/metrics/bucket_ra...
base/metrics/bucket_ranges.h:46: size_t BucketCount() const { return
ranges_.size() - 1; }
On 2013/06/20 16:41:18, jar wrote:
> On 2013/06/20 05:57:34, Ilya Sherman wrote:
> > On 2013/06/20 05:53:32, Alexei Svitkine wrote:
> > > Drive by nit:
> > > 
> > > This should probably either use hacker_style or be called GetBucketCount()
> and
> > > be defined out of line.
> > 
> > I don't have a strong preference.  Jim, which of these would you prefer?
> 
> I was a little tentative about the cost... but I guess size() on a vector
should
> be super fast, and I shouldn't sweat it.  The place where it *might* be an
issue
> is when we are looking for corruption, and we use this call to find the max
> bound (time and again).  I guess I should wait until we have a profiler
telling
> me this was a hot spot.
> 
> 
> I'm fine with retaining hacker style, and putting this in the header as you
did.

Done.

https://chromiumcodereview.appspot.com/17451016/diff/1/base/metrics/statistic...
File base/metrics/statistics_recorder_unittest.cc (right):

https://chromiumcodereview.appspot.com/17451016/diff/1/base/metrics/statistic...
base/metrics/statistics_recorder_unittest.cc:41:
StatisticsRecorder::RegisterOrDeleteDuplicateRanges(ranges);
On 2013/06/20 16:41:18, jar wrote:
> This tries to de-dup a set of ranges that are fully fleshed out.
> 
> It is not clear how we could have fleshed out the ranges if we haven't even
> supplied the min and max values (that were given to us as arguments).
> 
> Perhaps you need to call:
>  base::Histogram::InitializeBucketRanges(min, max, ranges)
> after line 39 in onder to set these up.

Done.

[jar (doing other things), 2013-06-22 01:23:14]

LGTM, with an optional nit to consider.

https://chromiumcodereview.appspot.com/17451016/diff/13001/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/17451016/diff/13001/base/metrics/histo...
base/metrics/histogram.cc:253: (bucket_count == this->bucket_count()));
nit: This is made me re-read a few times.... perhaps better would be to rename
the argument |new_bucket_count| and then you don't need the "this->" to
disambiguate.

WDYT?

[Ilya Sherman, 2013-06-27 04:00:10]

https://chromiumcodereview.appspot.com/17451016/diff/13001/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/17451016/diff/13001/base/metrics/histo...
base/metrics/histogram.cc:253: (bucket_count == this->bucket_count()));
On 2013/06/22 01:23:15, jar wrote:
> nit: This is made me re-read a few times.... perhaps better would be to rename
> the argument |new_bucket_count| and then you don't need the "this->" to
> disambiguate.
> 
> WDYT?

Done.

[commit-bot: I haz the power, 2013-06-27 04:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/17451016/38001

[commit-bot: I haz the power, 2013-06-27 14:38:46]

Change committed as 208917