Cache the ranges_ vector and share the ranges_ vector

Cache the ranges_ vector and share the ranges_ vector
across histograms if the ranges in the ranges_ vector
is same. This change saves around 100k of memory in the
browser (around 400 histograms sharing the same ranges_
vector and each ranges_ vector has around 50 elements).
In each renderer process we are sharing ranges_ vector
for around 30 histograms (a savings of 6k of memory).

R=jar
TEST=histogram unit tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106425

[ramant (doing other things), 2011-08-24 17:30:01]

Hi Jim,
  Would appreciate if you could look at this CL (this is my first cut). It is
saving on average around 100k of memory (when I opened couple of pages, browser
had around 450 histograms).

thanks,
raman

[jar (doing other things), 2011-10-04 22:32:38]

Mostly nits below.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1036: delete ranges;
I'd suggest we explicity comment that we are going to leak the histograms, and
leak the ranges.  I'd suggest putting line 1029 next to line 1036, putting them
both in a comment, and note that we will leak them deliberately.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1083: CachedRanges* cached_ranges =
histogram->cached_ranges();
nit: suggest name change for the local variable here:

cached_ranges -->> histogram_ranges

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1091: std::list<CachedRanges*>* ranges_list(new
std::list<CachedRanges*>());
nit: suggest name change for local variable:

ranges_list   -->> checksum_matching_list

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1126:
UMA_HISTOGRAM_COUNTS_10000("Histogram.FirstUpload.Count." + suffix,
For histogram naming, it is always nice to have similar histograms alphabetized
near each other.  To get this, you need the least consequential stuff at teh
tail of the name.  Hence better would be something like:

Histogram.SharedRange.Count.FirstUpload
Histogram.SharedRange.RangesSaved.FirstUpload
Histogram.SharedRange.ElementsSaved.FirstUpload

I think this will work nicely, putting the three histograms about Count near
each other, etc.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h#new...
base/metrics/histogram.h:286: class CachedRanges;
nit: Probably alphabetize list would be nice (not sure if it is standard).

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h#new...
base/metrics/histogram.h:822: // For each index, show the least value that can
be stored in the
nit: Suggest changing comment to:

A monotonically increasing list values which determine which bucket to put a
sample into.  For each index, show the smallest sample that can be added to the
corresponding bucket.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h#new...
base/metrics/histogram.h:826: // |range_checksum_| is a checksum of all the
sample in ranges_ vector.
nit: Suggest changing comment to:
Checksum for the conntents of ranges_.  Used to detect random over-writes of our
data, and to quickly see if some other CachedRanges instance is possibly Equal()
to this instance.

http://codereview.chromium.org/7696017/diff/7011/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7696017/diff/7011/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:785:
base::StatisticsRecorder::CollectHistogramStats("Browser");
We probably don't want to collect these here, as they will confuse the upload
stats.  It is possibly nice for debugging, and if you'd like, you can put this
in an ifndef NDEBUG.

[ramant (doing other things), 2011-10-19 01:09:52]

Hi Jim,
  Made all the changes you have suggested. Would appreciate if you could take
another look at this CL.

thanks much,
raman

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1036: delete ranges;
On 2011/10/04 22:32:38, jar wrote:
> I'd suggest we explicity comment that we are going to leak the histograms, and
> leak the ranges.  I'd suggest putting line 1029 next to line 1036, putting
them
> both in a comment, and note that we will leak them deliberately.

Done.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1083: CachedRanges* cached_ranges =
histogram->cached_ranges();
On 2011/10/04 22:32:38, jar wrote:
> nit: suggest name change for the local variable here:
> 
> cached_ranges -->> histogram_ranges
> 

Done.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1091: std::list<CachedRanges*>* ranges_list(new
std::list<CachedRanges*>());
On 2011/10/04 22:32:38, jar wrote:
> nit: suggest name change for local variable:
> 
> ranges_list   -->> checksum_matching_list

Done.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.cc#ne...
base/metrics/histogram.cc:1126:
UMA_HISTOGRAM_COUNTS_10000("Histogram.FirstUpload.Count." + suffix,
On 2011/10/04 22:32:38, jar wrote:
> For histogram naming, it is always nice to have similar histograms
alphabetized
> near each other.  To get this, you need the least consequential stuff at teh
> tail of the name.  Hence better would be something like:
> 
> Histogram.SharedRange.Count.FirstUpload
> Histogram.SharedRange.RangesSaved.FirstUpload
> Histogram.SharedRange.ElementsSaved.FirstUpload
> 
> I think this will work nicely, putting the three histograms about Count near
> each other, etc.

Done.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h#new...
base/metrics/histogram.h:286: class CachedRanges;
On 2011/10/04 22:32:38, jar wrote:
> nit: Probably alphabetize list would be nice (not sure if it is standard).

Done.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h#new...
base/metrics/histogram.h:822: // For each index, show the least value that can
be stored in the
On 2011/10/04 22:32:38, jar wrote:
> nit: Suggest changing comment to:
> 
> A monotonically increasing list values which determine which bucket to put a
> sample into.  For each index, show the smallest sample that can be added to
the
> corresponding bucket.

Done.

http://codereview.chromium.org/7696017/diff/7011/base/metrics/histogram.h#new...
base/metrics/histogram.h:826: // |range_checksum_| is a checksum of all the
sample in ranges_ vector.
On 2011/10/04 22:32:38, jar wrote:
> nit: Suggest changing comment to:
> Checksum for the conntents of ranges_.  Used to detect random over-writes of
our
> data, and to quickly see if some other CachedRanges instance is possibly
Equal()
> to this instance.

Done.

http://codereview.chromium.org/7696017/diff/7011/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7696017/diff/7011/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:785:
base::StatisticsRecorder::CollectHistogramStats("Browser");
On 2011/10/04 22:32:38, jar wrote:
> We probably don't want to collect these here, as they will confuse the upload
> stats.  It is possibly nice for debugging, and if you'd like, you can put this
> in an ifndef NDEBUG.

Done.

[jar (doing other things), 2011-10-19 23:33:03]

One nit below.

Also...

There is a small chance that this will induce memory leaks (that we have not
filed suppression for).  Be sure you watch the bots when you land.

LGTM

http://codereview.chromium.org/7696017/diff/12001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/7696017/diff/12001/base/metrics/histogram.h#ne...
base/metrics/histogram.h:862: // A monotonically increasing list values which
determine which bucket to put
nit: typo (in my suggestion)

...list values ... ===>>> ...list of values ...

[ramant (doing other things), 2011-10-20 00:11:35]

Fixed the comment. Thanks much Jim,
raman

http://codereview.chromium.org/7696017/diff/12001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/7696017/diff/12001/base/metrics/histogram.h#ne...
base/metrics/histogram.h:862: // A monotonically increasing list values which
determine which bucket to put
On 2011/10/19 23:33:03, jar wrote:
> nit: typo (in my suggestion)
> 
> ...list values ... ===>>> ...list of values ...

Done.

[commit-bot: I haz the power, 2011-10-20 00:12:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/7696017/19001