Allow histograms in cache stats.cc to bypass corruption checks...

Allow histograms in cache stats.cc to bypass corruption checks

This is a relanding of http://codereview.chromium.org/4174002

The change in this CL is that I've made the function
to look for errors virtual, and over-rode it in StatsHistograms.
The current override doesn't actually check for inconsistencies,
so that the UMA checks (and renderer-to-histogram checks) won't
complain and refuse to transport the histograms.

BUG=61281
r=rvargas

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

[rvargas (doing something else), 2010-11-03 18:08:38]

LGTM for StatsHistogram + rbstmp for the rest.

http://codereview.chromium.org/4364001/diff/9001/10002
File base/metrics/histogram.h (right):

http://codereview.chromium.org/4364001/diff/9001/10002#newcode339
base/metrics/histogram.h:339: // updated on several threads simultaneously), the
tallies might mismatch,
I hope we don't do that... I believe there is an open bug against histograms
collecting data on multiple threads.

http://codereview.chromium.org/4364001/diff/9001/10007
File net/disk_cache/stats_histogram.h (right):

http://codereview.chromium.org/4364001/diff/9001/10007#newcode47
net/disk_cache/stats_histogram.h:47: 
nit: remove this line

[jar (doing other things), 2010-11-03 18:21:44]

Comments and change per notes from rvargas.

http://codereview.chromium.org/4364001/diff/9001/10002
File base/metrics/histogram.h (right):

http://codereview.chromium.org/4364001/diff/9001/10002#newcode339
base/metrics/histogram.h:339: // updated on several threads simultaneously), the
tallies might mismatch,
On 2010/11/03 18:08:38, rvargas wrote:
> I hope we don't do that... I believe there is an open bug against histograms
> collecting data on multiple threads.

We can relatively safely support gathering histograms on several threads, and
only have to be careful about running the static initializer (if it is used) on
a single thread.  The likelihood of an error (in a count) is very small compared
to the signal gathered (as typically, work case, we'd have an off-by-one error
in a single bucket's tally.  This is slightly worse now, as we can also be
off-by-one in our redundant count).

The common request is for a safe way to handle the static initializer... or
probably equivalent... to not use the static variable and hence be thread safe. 
There is a pattern that handles this.... but it is uncommonly used.

http://codereview.chromium.org/4364001/diff/9001/10007
File net/disk_cache/stats_histogram.h (right):

http://codereview.chromium.org/4364001/diff/9001/10007#newcode47
net/disk_cache/stats_histogram.h:47: 
On 2010/11/03 18:08:38, rvargas wrote:
> nit: remove this line

Done.

[rvargas (doing something else), 2010-11-03 21:09:04]

http://codereview.chromium.org/4364001/diff/9001/10002
File base/metrics/histogram.h (right):

http://codereview.chromium.org/4364001/diff/9001/10002#newcode339
base/metrics/histogram.h:339: // updated on several threads simultaneously), the
tallies might mismatch,
On 2010/11/03 18:21:44, jar wrote:
> On 2010/11/03 18:08:38, rvargas wrote:
> > I hope we don't do that... I believe there is an open bug against histograms
> > collecting data on multiple threads.
> 
> We can relatively safely support gathering histograms on several threads, and
> only have to be careful about running the static initializer (if it is used)
on
> a single thread.  The likelihood of an error (in a count) is very small
compared
> to the signal gathered (as typically, work case, we'd have an off-by-one error
> in a single bucket's tally.  This is slightly worse now, as we can also be
> off-by-one in our redundant count).
> 
> The common request is for a safe way to handle the static initializer... or
> probably equivalent... to not use the static variable and hence be thread
safe. 
> There is a pattern that handles this.... but it is uncommonly used.

Yes, I understood the comment in the code as referring to the second case, not
the first one (although both patterns will trigger errors on valrind & co so
even the first one is not really that nice). I have never seen code using the
second pattern correctly, but maybe I have not looked hard enough :)

[jam, 2010-11-18 00:16:48]

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

http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc#n...
base/metrics/histogram.cc:617: DCHECK_EQ(total, redundant_count_);
this DCHECK is firing a lot for me.  is it really needed?

[jar (doing other things), 2010-11-18 17:30:28]

At a minimum, you can comment it out in your code.

Do you hit it during shutdown?
Uma upload time? (usually disabled in debug mode)
Visiting about:histograms?

The DCHECK is probably instigated by racy histogram updates, which are
generally acceptable.  IT should be VERY rare that this is hit, unless
something in pretty wrong, as the likelihood of hitting this should be in
the one-in-a-million category.

For clarification, could you tell me what histogram is causing the problem?
 You can usually look up the stack a frame or two and see a histogram
instance... and it will have a member |name| which will answer my question.

Thanks,

Jim

On Wed, Nov 17, 2010 at 4:16 PM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc
> File base/metrics/histogram.cc (right):
>
>
>
http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc#n...
> base/metrics/histogram.cc:617: DCHECK_EQ(total, redundant_count_);
> this DCHECK is firing a lot for me.  is it really needed?
>
> http://codereview.chromium.org/4364001/
>

[jar (doing other things), 2010-11-18 17:31:27]

Oh... and I should add.... a memory trashing bug would also cause this to
fire... and that is what it is really watching for.

Jim

On Thu, Nov 18, 2010 at 9:30 AM, Jim Roskind <jar@chromium.org> wrote:

> At a minimum, you can comment it out in your code.
>
> Do you hit it during shutdown?
> Uma upload time? (usually disabled in debug mode)
> Visiting about:histograms?
>
> The DCHECK is probably instigated by racy histogram updates, which are
> generally acceptable.  IT should be VERY rare that this is hit, unless
> something in pretty wrong, as the likelihood of hitting this should be in
> the one-in-a-million category.
>
> For clarification, could you tell me what histogram is causing the problem?
>  You can usually look up the stack a frame or two and see a histogram
> instance... and it will have a member |name| which will answer my question.
>
> Thanks,
>
> Jim
>
>
> On Wed, Nov 17, 2010 at 4:16 PM, <jam@chromium.org> wrote:
>
>>
>>
>> http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc
>> File base/metrics/histogram.cc (right):
>>
>>
>>
http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc#n...
>> base/metrics/histogram.cc:617: DCHECK_EQ(total, redundant_count_);
>> this DCHECK is firing a lot for me.  is it really needed?
>>
>> http://codereview.chromium.org/4364001/
>>
>
>

[jam, 2010-11-18 18:12:02]

I'll send you a stack trace next time this happens.  I've seen it happen at
least 10 times so far, with a build with no changes from trunk..  I wasn't
visiting about:histograms.  Most of the time might have been in
--single-process mode.

On Thu, Nov 18, 2010 at 9:31 AM, Jim Roskind <jar@chromium.org> wrote:

> Oh... and I should add.... a memory trashing bug would also cause this to
> fire... and that is what it is really watching for.
>
> Jim
>
>
> On Thu, Nov 18, 2010 at 9:30 AM, Jim Roskind <jar@chromium.org> wrote:
>
>> At a minimum, you can comment it out in your code.
>>
>> Do you hit it during shutdown?
>> Uma upload time? (usually disabled in debug mode)
>> Visiting about:histograms?
>>
>> The DCHECK is probably instigated by racy histogram updates, which are
>> generally acceptable.  IT should be VERY rare that this is hit, unless
>> something in pretty wrong, as the likelihood of hitting this should be in
>> the one-in-a-million category.
>>
>> For clarification, could you tell me what histogram is causing the
>> problem?  You can usually look up the stack a frame or two and see a
>> histogram instance... and it will have a member |name| which will answer my
>> question.
>>
>> Thanks,
>>
>> Jim
>>
>>
>> On Wed, Nov 17, 2010 at 4:16 PM, <jam@chromium.org> wrote:
>>
>>>
>>>
>>> http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc
>>> File base/metrics/histogram.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc#n...
>>> base/metrics/histogram.cc:617: DCHECK_EQ(total, redundant_count_);
>>> this DCHECK is firing a lot for me.  is it really needed?
>>>
>>> http://codereview.chromium.org/4364001/
>>>
>>
>>
>

[jam, 2010-11-22 22:47:58]

btw this is pretty reproducible for me when I run browser_tests with
--single-process

On Thu, Nov 18, 2010 at 10:11 AM, John Abd-El-Malek <jam@chromium.org>wrote:

> I'll send you a stack trace next time this happens.  I've seen it happen at
> least 10 times so far, with a build with no changes from trunk..  I wasn't
> visiting about:histograms.  Most of the time might have been in
> --single-process mode.
>
>
> On Thu, Nov 18, 2010 at 9:31 AM, Jim Roskind <jar@chromium.org> wrote:
>
>> Oh... and I should add.... a memory trashing bug would also cause this to
>> fire... and that is what it is really watching for.
>>
>> Jim
>>
>>
>> On Thu, Nov 18, 2010 at 9:30 AM, Jim Roskind <jar@chromium.org> wrote:
>>
>>> At a minimum, you can comment it out in your code.
>>>
>>> Do you hit it during shutdown?
>>> Uma upload time? (usually disabled in debug mode)
>>> Visiting about:histograms?
>>>
>>> The DCHECK is probably instigated by racy histogram updates, which are
>>> generally acceptable.  IT should be VERY rare that this is hit, unless
>>> something in pretty wrong, as the likelihood of hitting this should be in
>>> the one-in-a-million category.
>>>
>>> For clarification, could you tell me what histogram is causing the
>>> problem?  You can usually look up the stack a frame or two and see a
>>> histogram instance... and it will have a member |name| which will answer my
>>> question.
>>>
>>> Thanks,
>>>
>>> Jim
>>>
>>>
>>> On Wed, Nov 17, 2010 at 4:16 PM, <jam@chromium.org> wrote:
>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc
>>>> File base/metrics/histogram.cc (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/4364001/diff/28001/base/metrics/histogram.cc#n...
>>>> base/metrics/histogram.cc:617: DCHECK_EQ(total, redundant_count_);
>>>> this DCHECK is firing a lot for me.  is it really needed?
>>>>
>>>> http://codereview.chromium.org/4364001/
>>>>
>>>
>>>
>>
>