Hold histograms in static variables instead of allocating heap.

Hold histograms in static variables instead of allocating heap.

If instance variables holding pointers to Histograms, it would cause
crash bug when the instance is torn down and the metrics services use
and recording of their contents.

The current "standard" patten for using Histograms includes making sure
that the Histogram instances are not destroyed prematurely, held in
static variables.

BUG=28096
TEST=none

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

[Yuzo, 2009-11-18 05:39:56]

Linux bot reports a compilation error:

websocket_experiment_runner.cc:280:1: error: pasting
""WebSocketExperiment.Basic."" and ""LastState"" does not give a valid
preprocessing token

[Yuzo, 2009-11-18 05:40:12]

Linux bot reports a compilation error:

websocket_experiment_runner.cc:280:1: error: pasting
""WebSocketExperiment.Basic."" and ""LastState"" does not give a valid
preprocessing token

[tyoshino (SeeGerritForStatus), 2009-11-18 05:44:36]

http://codereview.chromium.org/404022/diff/1/2
File chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
(right):

http://codereview.chromium.org/404022/diff/1/2#newcode284
Line 284: UPDATE_HISTOGRAM(kWebSocketConnectName, task_result.last_state,
kLastStateName

[ukai, 2009-11-18 05:50:19]

On 2009/11/18 05:39:56, Yuzo wrote:
> Linux bot reports a compilation error:
> 
> websocket_experiment_runner.cc:280:1: error: pasting
> ""WebSocketExperiment.Basic."" and ""LastState"" does not give a valid
> preprocessing token

Oops. Sorry. ## was wrong here.

[ukai, 2009-11-18 05:50:44]

http://codereview.chromium.org/404022/diff/1/2
File chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
(right):

http://codereview.chromium.org/404022/diff/1/2#newcode284
Line 284: UPDATE_HISTOGRAM(kWebSocketConnectName, task_result.last_state,
On 2009/11/18 05:44:36, tyoshino wrote:
> kLastStateName

Done.

[Yuzo, 2009-11-18 06:00:03]

http://codereview.chromium.org/404022/diff/5001/5002
File chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
(right):

http://codereview.chromium.org/404022/diff/5001/5002#newcode28
Line 28: // TODO(ukai): Use new thread-safe-reference-counted Histograms.
So this is currently thread-unsafe?
What is the consequence? Lost data? Corrupted data? Or crash?

http://codereview.chromium.org/404022/diff/5001/5002#newcode285
Line 285: 50);
How about defining kBucketCount = 50 ?

If it is always 50, you might want to remove the count
from the macro arguments and use the number inside macro
definitions.

[ukai, 2009-11-18 06:13:07]

http://codereview.chromium.org/404022/diff/5001/5002
File chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
(right):

http://codereview.chromium.org/404022/diff/5001/5002#newcode28
Line 28: // TODO(ukai): Use new thread-safe-reference-counted Histograms.
On 2009/11/18 06:00:03, Yuzo wrote:
> So this is currently thread-unsafe?
> What is the consequence? Lost data? Corrupted data? Or crash?
> 
I think thread-safeness isn't issue here.
The issue is in histogram instance lifetime.
It must be alive while the metric service is alive.
But old code owns histogram in WebSocketExperimentRunner, so it would crash
after WebSocketExperimentRunner is destroyed.
Current idiom to use histogram is to hold in static variables in
methods/functions, and this CL changes to the style.

http://codereview.chromium.org/404022/diff/5001/5002#newcode285
Line 285: 50);
On 2009/11/18 06:00:03, Yuzo wrote:
> How about defining kBucketCount = 50 ?
> 
> If it is always 50, you might want to remove the count
> from the macro arguments and use the number inside macro
> definitions.

Done.

[tyoshino (SeeGerritForStatus), 2009-11-18 06:16:47]

LGTM

[Yuzo, 2009-11-18 06:19:18]

LGTM

http://codereview.chromium.org/404022/diff/5001/5002
File chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
(right):

http://codereview.chromium.org/404022/diff/5001/5002#newcode28
Line 28: // TODO(ukai): Use new thread-safe-reference-counted Histograms.
On 2009/11/18 06:13:07, ukai wrote:
> On 2009/11/18 06:00:03, Yuzo wrote:
> > So this is currently thread-unsafe?
> > What is the consequence? Lost data? Corrupted data? Or crash?
> > 
> I think thread-safeness isn't issue here.
> The issue is in histogram instance lifetime.
> It must be alive while the metric service is alive.
> But old code owns histogram in WebSocketExperimentRunner, so it would crash
> after WebSocketExperimentRunner is destroyed.
> Current idiom to use histogram is to hold in static variables in
> methods/functions, and this CL changes to the style.
> 
> 

Hmm, then why the TODO comment?

I'm fine as far as the static variable idiom is safe, though.

[jar (doing other things), 2009-11-18 06:58:13]

The transition to combat the destruction problem looks fine.

One question: Will these data-recording functions be called from a single
thread, or from a multitude of threads asynchronously?

With static initializers, care must be taken to be sure that two threads can't
try to initialize the static at the same time.  Once initialized, the code paths
adding samples into the static will work well enough without locks, but the
initializer is the only issue.

If the code is only called on one thread (the IO thread?), then there is no
problem. IF the code is called from a multitude of (worker?) threads, then there
could be a problem.  The two patterns for protection are to be sure we run the
static initializer while we're still single threaded  (or on one thread before
really using the counters), or by using a lock around the static initializer
point.

As a side question, is this "experiment" written up anywhere?  I'd like to
understand what it is measuring, and how.  Thanks!

[ukai, 2009-11-18 07:36:43]

On 2009/11/18 06:58:13, jar wrote:
> The transition to combat the destruction problem looks fine.
> 
> One question: Will these data-recording functions be called from a single
> thread, or from a multitude of threads asynchronously?
> 
> With static initializers, care must be taken to be sure that two threads can't
> try to initialize the static at the same time.  Once initialized, the code
paths
> adding samples into the static will work well enough without locks, but the
> initializer is the only issue.
> 
> If the code is only called on one thread (the IO thread?), then there is no
> problem. IF the code is called from a multitude of (worker?) threads, then
there
> could be a problem.  The two patterns for protection are to be sure we run the
> static initializer while we're still single threaded  (or on one thread before
> really using the counters), or by using a lock around the static initializer
> point.

These code always runs on the IO thread.
 
> As a side question, is this "experiment" written up anywhere?  I'd like to
> understand what it is measuring, and how.  Thanks!

There is a document at
https://docs.google.com/a/google.com/Doc?docid=0AQA1QKSx60lWZGNkOGI3em1fMTc3N...
(in google.com).


Thanks for review!

[jar (doing other things), 2009-11-18 09:11:09]

LGTM (assuming as you noted that you're on the IO thread during all data
recording).