Use lock-free lazy initialization for static histogram references

Use lock-free lazy initialization for static histogram references

Make all histogram macros thread safe, and fast by again
using statics to achieve performance.

...at the cost of:
Leak all histograms to avoid races at shutdown.

Also included leak suppression for valgrind.

r=rtenneti
BUG=78207
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80412

[jar (doing other things), 2011-04-01 18:08:18]

Note that the "leak" is not an ongoing leak, and hence would use the same amount
of memory as we ever used.  The only difference is that we don't clean up during
shutdown, so we don't have to worry about static destruction races.

[rvargas (doing something else), 2011-04-01 19:20:52]

Please use the valgrind bots (try --bot=xx_valgrind)

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

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:38: // pointer is NOT deleted, and we leak the
histograms at process termination.
Have you considered using LazyInstance<> instead? It was designed to provide
exactly this functionality, and it doesn't leak.

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:687: // |histogram| will deleted.  The returns value is
always the registered
nit: the returned value

http://codereview.chromium.org/6780035/diff/10071/net/disk_cache/stats.h
File net/disk_cache/stats.h (right):

http://codereview.chromium.org/6780035/diff/10071/net/disk_cache/stats.h#newc...
net/disk_cache/stats.h:89: StatsHistogram* size_histogram_;
Can we have a scoped_ptr instead?. Probably not... never mind then.

[ramant (doing other things), 2011-04-01 20:05:38]

Hi Jim,
  Most of my suggestions are nit. Changes LGTM.
thanks,
raman

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

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:99: // DO NOT USE THIS.  It is being phased out, in
favor of HISTOGRAM_CUSTOM_TIMES. ....
More than 80 characters. Did you want to delete this macro?

http://codereview.chromium.org/6780035/diff/10071/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6780035/diff/10071/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:567: if (!should_sniff_counter)
Should we make this static?

[ramant (doing other things), 2011-04-01 20:07:26]

Missed one more nit:

http://codereview.chromium.org/6780035/diff/10071/chrome/browser/jankometer.cc
File chrome/browser/jankometer.cc (right):

http://codereview.chromium.org/6780035/diff/10071/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:145: ;
Extra semicolon?

[jar (doing other things), 2011-04-01 21:50:27]

Changes and responses to comments by ramant and rvargas

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

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:38: // pointer is NOT deleted, and we leak the
histograms at process termination.
As per discussion, one major point is that this code runs in any thread,
including worker threads (that are not joined at shutdown).  In addition, the
"statics" can often refer to a single "shared" (and registered) histogram.  This
precludes use of a singular static for a single named histogram (and is part of
why we call a factory, rather than new).

Bottom line: I couldn't see a way to reuse LazyInstance code.


On 2011/04/01 19:20:52, rvargas wrote:
> Have you considered using LazyInstance<> instead? It was designed to provide
> exactly this functionality, and it doesn't leak.

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:99: // DO NOT USE THIS.  It is being phased out, in
favor of HISTOGRAM_CUSTOM_TIMES. ....
On 2011/04/01 20:05:38, ramant wrote:
> More than 80 characters. Did you want to delete this macro?

Done.

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:687: // |histogram| will deleted.  The returns value is
always the registered
On 2011/04/01 19:20:52, rvargas wrote:
> nit: the returned value

Done.

http://codereview.chromium.org/6780035/diff/10071/chrome/browser/jankometer.cc
File chrome/browser/jankometer.cc (right):

http://codereview.chromium.org/6780035/diff/10071/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:145: ;
On 2011/04/01 20:07:26, ramant wrote:
> Extra semicolon?

Done.

http://codereview.chromium.org/6780035/diff/10071/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6780035/diff/10071/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:567: if (!should_sniff_counter)
On 2011/04/01 20:05:38, ramant wrote:
> Should we make this static?

Done.

http://codereview.chromium.org/6780035/diff/10071/net/disk_cache/stats.h
File net/disk_cache/stats.h (right):

http://codereview.chromium.org/6780035/diff/10071/net/disk_cache/stats.h#newc...
net/disk_cache/stats.h:89: StatsHistogram* size_histogram_;
On 2011/04/01 19:20:52, rvargas wrote:
> Can we have a scoped_ptr instead?. Probably not... never mind then.


Since we're deliberately leaking all of these, we don't want to delete, since
others are depending on the eternally correct raw pointer (to memory that will
be leaked).

[jar (doing other things), 2011-04-02 07:25:41]

This version includes all required valgrind suppressions.  Valgrind runs fine on
linux, mac, and chromeos (although unrelated memory leaks keep the trybots a tad
red).

[willchan no longer on Chromium, 2011-04-02 07:43:31]

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

http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
base/metrics/histogram.h:38: // pointer is NOT deleted, and we leak the
histograms at process termination.
On 2011/04/01 21:50:27, jar wrote:
> As per discussion, one major point is that this code runs in any thread,
> including worker threads (that are not joined at shutdown).  In addition, the
> "statics" can often refer to a single "shared" (and registered) histogram. 
This
> precludes use of a singular static for a single named histogram (and is part
of
> why we call a factory, rather than new).
> 
> Bottom line: I couldn't see a way to reuse LazyInstance code.

I think you misunderstand LazyInstance. Both LazyInstance and Singleton provide
"leaky" traits (LeakyLazyInstanceTraits) that don't delete on shutdown. This
means they are not vulnerable to shutdown races/crashes as you imply.

Secondly, just like the "statics" can often refer to a single "shared" (and
registered) histogram, LazyInstance can do exactly the same thing. Singleton is
globally scoped (hence being called a Singleton), but LayInstance is simply a
lazily constructed (using a spinlock) instance. So, it's somewhat equivalent to
a threadsafe initialized function static which may register itself to the
AtExitManager (DefaultLazyInstanceTraits) for destruction on shutdown, or may
just leak (LeakyLazyInstanceTraits).

The static is cheaper because it doesn't have to pay for the spinlock on each
access.

> 
> 
> On 2011/04/01 19:20:52, rvargas wrote:
> > Have you considered using LazyInstance<> instead? It was designed to provide
> > exactly this functionality, and it doesn't leak.
>

[rvargas (doing something else), 2011-04-04 19:16:07]

LGTM

[rvargas (doing something else), 2011-04-04 19:24:54]

http://codereview.chromium.org/6780035/diff/24012/tools/valgrind/memcheck/sup...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/6780035/diff/24012/tools/valgrind/memcheck/sup...
tools/valgrind/memcheck/suppressions.txt:4123:
fun:_ZN14RunnableMethodI15DownloadManagerMS0_FvP18DownloadCreateInfoE6Tuple1vIS2_EE3RunEv
?

[jar (doing other things), 2011-04-04 22:13:09]

Corrected suppressions.txt

http://codereview.chromium.org/6780035/diff/24012/tools/valgrind/memcheck/sup...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/6780035/diff/24012/tools/valgrind/memcheck/sup...
tools/valgrind/memcheck/suppressions.txt:4123:
fun:_ZN14RunnableMethodI15DownloadManagerMS0_FvP18DownloadCreateInfoE6Tuple1vIS2_EE3RunEv
On 2011/04/04 19:24:55, rvargas wrote:
> ?

Spurious character removed.

THANKS for spotting this!!!!

[commit-bot: I haz the power, 2011-04-04 22:14:38]

Presubmit check for 6780035-24023 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/buffered_resource_handler.cc

Presubmit checks took 6.9s to calculate.

[Avi (use Gerrit), 2011-04-04 22:19:33]

Owner rubber-stamp LGTM.

[jar (doing other things), 2011-04-04 22:30:09]

I'd be curious to see how you'd craft the macro.  Would it use a templatized
type, including the parameters for construction?  How does a templatized type
deal with a string (histogram name)?  Especially, how does it deal with a string
name that is not known until runtime?   ...and how does it avoid re-evaluation
of that run-time expression?  More than once? Is that string expression another
LazyInstance?  Could you elaborate on some of this, or point me at the
header/code that you think would be applicable?

Thanks,

Jim (it wasn't obvious to me) Roskind

On 2011/04/02 07:43:31, willchan wrote:
> http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h
> File base/metrics/histogram.h (right):
> 
>
http://codereview.chromium.org/6780035/diff/10071/base/metrics/histogram.h#ne...
> base/metrics/histogram.h:38: // pointer is NOT deleted, and we leak the
> histograms at process termination.
> On 2011/04/01 21:50:27, jar wrote:
> > As per discussion, one major point is that this code runs in any thread,
> > including worker threads (that are not joined at shutdown).  In addition,
the
> > "statics" can often refer to a single "shared" (and registered) histogram. 
> This
> > precludes use of a singular static for a single named histogram (and is part
> of
> > why we call a factory, rather than new).
> > 
> > Bottom line: I couldn't see a way to reuse LazyInstance code.
> 
> I think you misunderstand LazyInstance. Both LazyInstance and Singleton
provide
> "leaky" traits (LeakyLazyInstanceTraits) that don't delete on shutdown. This
> means they are not vulnerable to shutdown races/crashes as you imply.
> 
> Secondly, just like the "statics" can often refer to a single "shared" (and
> registered) histogram, LazyInstance can do exactly the same thing. Singleton
is
> globally scoped (hence being called a Singleton), but LayInstance is simply a
> lazily constructed (using a spinlock) instance. So, it's somewhat equivalent
to
> a threadsafe initialized function static which may register itself to the
> AtExitManager (DefaultLazyInstanceTraits) for destruction on shutdown, or may
> just leak (LeakyLazyInstanceTraits).
> 
> The static is cheaper because it doesn't have to pay for the spinlock on each
> access.
> 
> > 
> > 
> > On 2011/04/01 19:20:52, rvargas wrote:
> > > Have you considered using LazyInstance<> instead? It was designed to
provide
> > > exactly this functionality, and it doesn't leak.
> >

[commit-bot: I haz the power, 2011-04-04 23:10:40]

Try job failure for 6780035-24023 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...