Disk cache: Remove stats_histogram.

Disk cache: Remove stats_histogram.

StatsHistograms is the only "external" subclass of base::Histogram and as such
it often requires extra considerations when modifying the base class. On the
other hand, almost the same data can be gathered by using a regular log
histogram with a specific range.

This CL does that, generating a 75 bucket histogram to map the 27 buckets
from the original data. Most of the buckets will be empty, and a couple of
buckets will be merged on the new histogram, but the compromise looks
good enough.

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

[rvargas (doing something else), 2014-05-27 18:36:27]

I went through the data again and concluded that it generally works and provides
valuable data (except for M30, where a GPU change screwed all disk cache
histograms).

I also went trough the code but I could not find a reason for the sum being out
of range. The code is relatively straight forward, and it seems correct... and
in fact the results are correct for histograms that don't have a lot of users.
One thing that is quite different from other histograms is that the sum should
_always_ match the lower bound of the range, so a little noise will send it out
of the range (and the larger buckets easily drive the total).

Anyhow, I went ahead and removed the dedicated subclass. It should help with
maintenance (and sends the sum somewhere in the middle of the range).

[Alexei Svitkine (slow), 2014-05-27 18:59:46]

Thanks!

This looks like a sufficiently large change that it makes sense to have a
crbug.com for (e.g. in case it gets reverted and needs to reland, etc).

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
File net/disk_cache/blockfile/stats.cc (right):

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:129: if (!first_time)
It seems that in the new version of the code, you're only recording the stats
once on start up (when this function is called), whereas the old code I believe
may be doing whenever SnapshotSamples() is called. (I'm not super sure what that
actually means - perhaps it ends up being once every UMA upload?).

If the old code did indeed do it once every UMA upload, then this can be
supported too in the new code by hooking in to
ChromeMetricsServiceClient::CollectFinalMetrics() - and we do this for other
things like memory stats, etc.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:140: "DiskCache.SizeStats2", min, max,
num_buckets,
Please add a histograms.xml entry for this histogram with yourself as owner in
the same CL.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:145: if (data_sizes_[i] < 0)
This could use a comment.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:146: data_sizes_[i] = 0;
Does it make sense for this to actually change data_sizes_? Or would it be
better to simply skip the Accumulate() call in this case? (In the old code, you
don't actually change data_sizes).

[rvargas (doing something else), 2014-05-27 19:36:38]

Thanks!

Please take another look.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
File net/disk_cache/blockfile/stats.cc (right):

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:129: if (!first_time)
The logic is pretty much the same: The histogram is generated only once and it
is not updated after that. The old code was changing the uma flag from 1 to 0
after the first snapshot so it was only uploaded once (for some reason I don't
fully remember ATM), but the result on the server should be the same as the next
snapshot will see a delta of 0 and do nothing. The only difference would be if
there is memory corruption and the data stored by base::Histogram changes 'cause
of that. The old code seems immune to that.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:140: "DiskCache.SizeStats2", min, max,
num_buckets,
On 2014/05/27 18:59:47, Alexei Svitkine wrote:
> Please add a histograms.xml entry for this histogram with yourself as owner in
> the same CL.

Done.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:145: if (data_sizes_[i] < 0)
On 2014/05/27 18:59:47, Alexei Svitkine wrote:
> This could use a comment.

Done.

https://codereview.chromium.org/295913014/diff/1/net/disk_cache/blockfile/sta...
net/disk_cache/blockfile/stats.cc:146: data_sizes_[i] = 0;
On 2014/05/27 18:59:47, Alexei Svitkine wrote:
> Does it make sense for this to actually change data_sizes_? Or would it be
> better to simply skip the Accumulate() call in this case? (In the old code,
you
> don't actually change data_sizes).

That's something that I saw when testing with my development profile... because
it crashes a lot, there was a bunch of negative numbers in there so it makes
sense to fix them at some point, and this seems like the perfect time to do it
(this is the only place where we actually go through the stored data).

[Alexei Svitkine (slow), 2014-05-27 19:39:16]

LGTM, thanks

https://codereview.chromium.org/295913014/diff/10001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/295913014/diff/10001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:3376: +  <summary>The size distribution
of data stored on the HTTP cache.</summary>
Nit: on -> in

[rvargas (doing something else), 2014-05-27 19:42:37]

Thanks again.

https://codereview.chromium.org/295913014/diff/10001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/295913014/diff/10001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:3376: +  <summary>The size distribution
of data stored on the HTTP cache.</summary>
On 2014/05/27 19:39:16, Alexei Svitkine wrote:
> Nit: on -> in

Done.

[rvargas (doing something else), 2014-05-27 19:42:44]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-05-27 19:43:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/295913014/30001

[commit-bot: I haz the power, 2014-05-27 23:49:42]

Change committed as 273074