Remove static locals from histograms.

Remove static locals from histograms.

Replaces RTC_HISTOGRAM_* usages with construction-time factory getter
calls, which removes the need for compare-and-swap atomic usage whenever
these are updated.

BUG=chromium:681103
R=tommi@webrtc.org

[pbos-webrtc, 2017-01-23 13:52:12]

remove old array include

[pbos-webrtc, 2017-01-23 14:16:01]

do check for Enabled() inside HistogramAdd

[pbos-webrtc, 2017-01-23 14:39:36]

remove one sizeof array size

[tommi, 2017-01-23 16:21:54]

is the biggest win here the removal of atomic operations?

[pbos-webrtc, 2017-01-23 16:37:25]

On 2017/01/23 16:21:54, tommi (webrtc) wrote:
> is the biggest win here the removal of atomic operations?

Nope, static locals and code size reduction. See the bug before checking the CL
here, this is an example of what we'd have to do across webrtc to get rid of the
static locals.

[tommi, 2017-01-24 10:20:23]

On 2017/01/23 16:37:25, pbos-webrtc wrote:
> On 2017/01/23 16:21:54, tommi (webrtc) wrote:
> > is the biggest win here the removal of atomic operations?
> 
> Nope, static locals and code size reduction. See the bug before checking the
CL
> here, this is an example of what we'd have to do across webrtc to get rid of
the
> static locals.

I think this is interesting and it does have the benefit of not relying on
macros as much.  However, I think we could make it more user friendly.  E.g. by
having a small class that takes care of specific counter type details such as
HistogramCounts, HistogramCountsLinear, HistogramPercentage, HistogramBoolean,
etc.  These classes could internally own the pointer and the constructors could
make arguments type safe and correct for the counter type.

Basically, if in the end, this change makes the code easier to read, smaller,
faster with clear argument types, then it's easier to make the case for changing
:)

[pbos-webrtc, 2017-01-24 19:01:20]

On 2017/01/24 10:20:23, tommi (webrtc) wrote:
> On 2017/01/23 16:37:25, pbos-webrtc wrote:
> > On 2017/01/23 16:21:54, tommi (webrtc) wrote:
> > > is the biggest win here the removal of atomic operations?
> > 
> > Nope, static locals and code size reduction. See the bug before checking the
> CL
> > here, this is an example of what we'd have to do across webrtc to get rid of
> the
> > static locals.
> 
> I think this is interesting and it does have the benefit of not relying on
> macros as much.  However, I think we could make it more user friendly.  E.g.
by
> having a small class that takes care of specific counter type details such as
> HistogramCounts, HistogramCountsLinear, HistogramPercentage, HistogramBoolean,
> etc.  These classes could internally own the pointer and the constructors
could
> make arguments type safe and correct for the counter type.
> 
> Basically, if in the end, this change makes the code easier to read, smaller,
> faster with clear argument types, then it's easier to make the case for
changing
> :)

Yeah I think there's some work we can do here to make it more readable, similar
to the RTC_HISTOGRAMS_100 etc. macros. I'll consider you supportive, and do
another suggestion when I have some spare time.

[pbos-webrtc, 2017-01-26 22:22:41]

implement a convenience rtc::Histogram class with type checking and less
arguments

[pbos-webrtc, 2017-01-26 22:23:00]

add histogram.h,cc

[pbos-webrtc, 2017-01-26 22:24:55]

Something like this tommi@?

You'd instantiate and use rtc::Histogram{,Linear,Boolean,Percentage} with a lot
more arguments optional + debug range checking for
enumerations/booleans/percentages.

[pbos-webrtc, 2017-01-26 22:35:56]

use another HistogramBase constructor for enumerations

[tommi, 2017-01-26 22:48:46]

On 2017/01/26 22:24:55, pbos-webrtc wrote:
> Something like this tommi@?
> 
> You'd instantiate and use rtc::Histogram{,Linear,Boolean,Percentage} with a
lot
> more arguments optional + debug range checking for
> enumerations/booleans/percentages.

hmm.. that's not exactly how I would do it myself (and btw enums in chromium and
webrtc don't follow google style).  I'll take another look in the coming days
(tomorrow looks to be completely booked :()

[mflodman, 2017-01-27 07:03:23]

Description was changed from

==========
Remove static locals from histograms.

Replaces RTC_HISTOGRAM_* usages with construction-time factory getter
calls, which removes the need for compare-and-swap atomic usage whenever
these are updated.

BUG=chromium:681103
R=tommi@webrtc.org
==========

to

==========
Remove static locals from histograms.

Replaces RTC_HISTOGRAM_* usages with construction-time factory getter
calls, which removes the need for compare-and-swap atomic usage whenever
these are updated.

BUG=chromium:681103
R=tommi@webrtc.org
==========

[mflodman, 2017-01-27 07:03:50]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2017-01-27 07:03:51]

+asapersson

[mflodman, 2017-01-27 07:27:03]

mflodman@webrtc.org changed reviewers:
- mflodman@webrtc.org