Add a default implementation in metrics_default.cc of histograms methods in system_wrappers/interfac

Add a default implementation in metrics_default.cc of histograms methods in system_wrappers/interface/metrics.h.

Updated tests to use the default implementation and removed the test implementation (webrtc/test/histograms.h).

BUG=

Committed: https://crrev.com/01d70a3978d5a404aa8b490d1b84ff529123ad4b
Cr-Commit-Position: refs/heads/master@{#12829}

[juberti2, 2016-04-25 16:44:34]

Description was changed from

==========
Add Android implementation of histograms methods in in
system_wrappers/interface/metrics.h.

The histogram information is stored in a map in androidhistrograms_jni.cc.
The native histograms can be queried through an API from java.

BUG=
==========

to

==========
Add Android implementation of histograms methods in in
system_wrappers/interface/metrics.h.

The histogram information is stored in a map in androidhistrograms_jni.cc.
The native histograms can be queried through an API from java.

BUG=
==========

[åsapersson, 2016-04-26 12:39:12]

Patchset #7 (id:120001) has been deleted

[åsapersson, 2016-04-27 13:10:15]

Patchset #9 (id:180001) has been deleted

[åsapersson, 2016-04-27 13:10:29]

Patchset #9 (id:200001) has been deleted

[åsapersson, 2016-04-27 14:01:45]

Patchset #8 (id:160001) has been deleted

[åsapersson, 2016-04-27 15:03:54]

Patchset #11 (id:270001) has been deleted

[åsapersson, 2016-04-27 15:04:02]

Patchset #10 (id:250001) has been deleted

[åsapersson, 2016-04-27 15:20:38]

Patchset #9 (id:240001) has been deleted

[åsapersson, 2016-04-28 15:18:46]

asapersson@webrtc.org changed reviewers:
+ pbos@webrtc.org

[åsapersson, 2016-04-28 15:23:15]

pbos can you have a first look?

[pbos-webrtc, 2016-05-02 00:29:10]

I think this looks good, but I don't think that the new histograms functionality
should be Android- or java- specific.

Can we move the implementation to metrics_default, put that implementation in
webrtc/base and maybe remove the test-only target for metrics?

Chromium should still have their own implementation, but it looks like
everything should be able to depend on this one, right?

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
File webrtc/api/java/jni/androidhistograms_jni.cc (right):

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:1: /*
Shouldn't this file be part of metrics_default? I think the JNI parts of these
should be here, but metrics_default should contain all functionality. That way
if someone wants to wire up metrics through C++ code it'll be easier as well.

Imo metrics could also move to webrtc/base.

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:36: bool rtc_histograms_enabled_ =
false;
Can we expect this to be set before any webrtc code gets called? We do that for
chrome://tracing, and that way you shouldn't have to take rtc_histograms_crit_
before checking rtc_histograms_enabled_.

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:81: SampleInfo* ptr =
reinterpret_cast<SampleInfo*>(histogram_pointer);
This function shouldn't hold the rtc_histograms_crit_ lock right, for
performance reasons. We want all cached uses of histogram_pointer to be fast.

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:102: JOW(void,
Histograms_nativeEnable)(JNIEnv* jni, jclass) {
Can we have C++ versions of these functions as well?

And if we do, does that mean that we can remove the test-target histogram
implementation?

[pbos-webrtc, 2016-05-02 00:30:13]

On 2016/05/02 00:29:10, pbos-webrtc wrote:
> I think this looks good, but I don't think that the new histograms
functionality
> should be Android- or java- specific.
> 
> Can we move the implementation to metrics_default, put that implementation in
> webrtc/base and maybe remove the test-only target for metrics?
> 
> Chromium should still have their own implementation, but it looks like
> everything should be able to depend on this one, right?
> 
>
https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
> File webrtc/api/java/jni/androidhistograms_jni.cc (right):
> 
>
https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
> webrtc/api/java/jni/androidhistograms_jni.cc:1: /*
> Shouldn't this file be part of metrics_default? I think the JNI parts of these
> should be here, but metrics_default should contain all functionality. That way
> if someone wants to wire up metrics through C++ code it'll be easier as well.
> 
> Imo metrics could also move to webrtc/base.
> 
>
https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
> webrtc/api/java/jni/androidhistograms_jni.cc:36: bool rtc_histograms_enabled_
=
> false;
> Can we expect this to be set before any webrtc code gets called? We do that
for
> chrome://tracing, and that way you shouldn't have to take rtc_histograms_crit_
> before checking rtc_histograms_enabled_.
> 
>
https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
> webrtc/api/java/jni/androidhistograms_jni.cc:81: SampleInfo* ptr =
> reinterpret_cast<SampleInfo*>(histogram_pointer);
> This function shouldn't hold the rtc_histograms_crit_ lock right, for
> performance reasons. We want all cached uses of histogram_pointer to be fast.
> 
>
https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
> webrtc/api/java/jni/androidhistograms_jni.cc:102: JOW(void,
> Histograms_nativeEnable)(JNIEnv* jni, jclass) {
> Can we have C++ versions of these functions as well?
> 
> And if we do, does that mean that we can remove the test-target histogram
> implementation?

https://chromium.googlesource.com/external/webrtc/+/0a1f14e591e7a65ce21006c02...
should be able to build on top of this one as well, right?

[åsapersson, 2016-05-03 14:38:05]

Patchset #12 (id:350001) has been deleted

[åsapersson, 2016-05-03 15:14:10]

Patchset #13 (id:390001) has been deleted

[åsapersson, 2016-05-03 15:24:25]

Patchset #13 (id:410001) has been deleted

[åsapersson, 2016-05-04 13:10:43]

Patchset #16 (id:490001) has been deleted

[åsapersson, 2016-05-04 13:10:53]

Patchset #15 (id:470001) has been deleted

[åsapersson, 2016-05-04 13:11:30]

Patchset #14 (id:450001) has been deleted

[åsapersson, 2016-05-04 15:42:04]

Patchset #14 (id:510001) has been deleted

[åsapersson, 2016-05-04 16:12:12]

Patchset #14 (id:530001) has been deleted

[åsapersson, 2016-05-06 08:54:42]

Description was changed from

==========
Add Android implementation of histograms methods in in
system_wrappers/interface/metrics.h.

The histogram information is stored in a map in androidhistrograms_jni.cc.
The native histograms can be queried through an API from java.

BUG=
==========

to

==========
Add a default implementation in metrics_default.cc of histograms methods in
system_wrappers/interface/metrics.h.

Updated tests to use the default implementation and removed the test
implementation (webrtc/test/histograms.h).

BUG=
==========

[åsapersson, 2016-05-06 09:06:52]

Patchset #15 (id:570001) has been deleted

[åsapersson, 2016-05-06 09:17:50]

Patchset #15 (id:590001) has been deleted

[åsapersson, 2016-05-06 10:07:45]

Patchset #17 (id:650001) has been deleted

[åsapersson, 2016-05-06 10:21:29]

Patchset #17 (id:670001) has been deleted

[åsapersson, 2016-05-06 10:22:28]

Patchset #16 (id:630001) has been deleted

[åsapersson, 2016-05-06 10:22:43]

Patchset #15 (id:610001) has been deleted

[åsapersson, 2016-05-06 10:23:20]

Patchset #14 (id:550001) has been deleted

[åsapersson, 2016-05-06 10:25:14]

Patchset #5 (id:80001) has been deleted

[åsapersson, 2016-05-06 10:25:24]

Patchset #4 (id:60001) has been deleted

[åsapersson, 2016-05-06 10:25:40]

Patchset #3 (id:40001) has been deleted

[åsapersson, 2016-05-06 10:26:03]

Patchset #5 (id:220001) has been deleted

[åsapersson, 2016-05-06 10:26:16]

Patchset #6 (id:310001) has been deleted

[åsapersson, 2016-05-06 10:26:29]

Patchset #5 (id:290001) has been deleted

[åsapersson, 2016-05-06 11:56:29]

Patchset #8 (id:690001) has been deleted

[åsapersson, 2016-05-06 13:24:04]

Patchset #9 (id:730001) has been deleted

[åsapersson, 2016-05-06 13:24:37]

Patchset #8 (id:710001) has been deleted

[åsapersson, 2016-05-06 13:31:57]

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
File webrtc/api/java/jni/androidhistograms_jni.cc (right):

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:1: /*
On 2016/05/02 00:29:10, pbos-webrtc wrote:
> Shouldn't this file be part of metrics_default? I think the JNI parts of these
> should be here, but metrics_default should contain all functionality. That way
> if someone wants to wire up metrics through C++ code it'll be easier as well.
> 
> Imo metrics could also move to webrtc/base.

Added implementation to metrics_default and have removed the test
implementation, ptal. 

Java specific parts have been moved to a separate CL:
https://codereview.webrtc.org/1952223007/

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:36: bool rtc_histograms_enabled_ =
false;
On 2016/05/02 00:29:10, pbos-webrtc wrote:
> Can we expect this to be set before any webrtc code gets called? We do that
for
> chrome://tracing, and that way you shouldn't have to take rtc_histograms_crit_
> before checking rtc_histograms_enabled_.

Done.

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:81: SampleInfo* ptr =
reinterpret_cast<SampleInfo*>(histogram_pointer);
On 2016/05/02 00:29:10, pbos-webrtc wrote:
> This function shouldn't hold the rtc_histograms_crit_ lock right, for
> performance reasons. We want all cached uses of histogram_pointer to be fast.

Done.

https://codereview.webrtc.org/1915523002/diff/330001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidhistograms_jni.cc:102: JOW(void,
Histograms_nativeEnable)(JNIEnv* jni, jclass) {
On 2016/05/02 00:29:10, pbos-webrtc wrote:
> Can we have C++ versions of these functions as well?
> 
> And if we do, does that mean that we can remove the test-target histogram
> implementation?

Done.

[pbos-webrtc, 2016-05-06 19:58:40]

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:29: #include
"webrtc/system_wrappers/include/metrics_default.h"
Can this (easily) be moved to webrtc/base?

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
File webrtc/system_wrappers/include/metrics_default.h (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:22: SampleInfo(const
std::string& name, int min, int max, int bucket_count);
size_t bucket_count

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:28: const int bucket_count;
size_t

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:29: std::map<int, int> samples;
 // <value, # of events>
Shouldn't this be a std::vector of ints that's initialized to the size of
bucket_count? That way we can do a fast lookup without samples.find(), but we
need to map |sample| to bucket_index.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:46: int NumEvents(const
std::string& name, int sample);
If we do go with respecting buckets, I think we should rename this
NumEventsInBucket or something like that. Same below.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:23: const int kMaxSampleMapSize
= 10000;
I think this should be covered by bucket_count?

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:28: : info_(name, min, max,
bucket_count) {}
I think the ctor here should set the size of samples_ and we should index into
it directly.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:59: sample = 0;
Is this an invalid sample? E.g. should we DCHECK instead?

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:64: if (info_.samples.size() ==
kMaxSampleMapSize &&
samples here should be fixed-size to number of buckets, right? And we should
then just index directly into it (e.g. translate index to bucket_index based on
min,max as well). Introduce a SampleToBucketIndex(sample) member function that
uses info_.min, info_.max and info_.bucket_count.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:78:
webrtc::metrics::SampleInfo* GetAndReset() {
Can this just be SampleInfo? You can put the anonymous namespace inside
webrtc::metrics.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:145: RTC_DCHECK(ptr->name() ==
name) << "The name should not vary.";
RTC_DCHECK_EQ(name, ptr->name())?

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:155: SampleInfo::~SampleInfo()
{}
Don't need a dtor I think

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:157: // Implementation of
methods in metrics_default.h.
s/methods/global functions/

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:52: using metrics::MinSample;
Pref not using using here, metrics::NumSamples is good I think.

[åsapersson, 2016-05-09 09:03:19]

Patchset #9 (id:770001) has been deleted

[åsapersson, 2016-05-09 14:47:42]

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:29: #include
"webrtc/system_wrappers/include/metrics_default.h"
On 2016/05/06 19:58:39, pbos-webrtc wrote:
> Can this (easily) be moved to webrtc/base?

Maybe this can be done in a separate CL if we would like to move the files?

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
File webrtc/system_wrappers/include/metrics_default.h (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:22: SampleInfo(const
std::string& name, int min, int max, int bucket_count);
On 2016/05/06 19:58:39, pbos-webrtc wrote:
> size_t bucket_count

Done.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:28: const int bucket_count;
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> size_t

Done.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/i...
webrtc/system_wrappers/include/metrics_default.h:29: std::map<int, int> samples;
 // <value, # of events>
On 2016/05/06 19:58:39, pbos-webrtc wrote:
> Shouldn't this be a std::vector of ints that's initialized to the size of
> bucket_count? That way we can do a fast lookup without samples.find(), but we
> need to map |sample| to bucket_index.

Since most metrics are an aggregate over the call and not logged frequently,
this might not be needed right now? I added a todo that we should consider
changing this...

sample.find() is only used in test functionality now, right?

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:23: const int kMaxSampleMapSize
= 10000;
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> I think this should be covered by bucket_count?

see other comment

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:28: : info_(name, min, max,
bucket_count) {}
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> I think the ctor here should set the size of samples_ and we should index into
> it directly.

see other comment

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:59: sample = 0;
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> Is this an invalid sample? E.g. should we DCHECK instead?

Changed to limit by min bucket value.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:64: if (info_.samples.size() ==
kMaxSampleMapSize &&
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> samples here should be fixed-size to number of buckets, right? And we should
> then just index directly into it (e.g. translate index to bucket_index based
on
> min,max as well). Introduce a SampleToBucketIndex(sample) member function that
> uses info_.min, info_.max and info_.bucket_count.

see other comment

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:78:
webrtc::metrics::SampleInfo* GetAndReset() {
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> Can this just be SampleInfo? You can put the anonymous namespace inside
> webrtc::metrics.

Done.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:145: RTC_DCHECK(ptr->name() ==
name) << "The name should not vary.";
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> RTC_DCHECK_EQ(name, ptr->name())?

Done.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:155: SampleInfo::~SampleInfo()
{}
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> Don't need a dtor I think

Gives compile error w/o.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:157: // Implementation of
methods in metrics_default.h.
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> s/methods/global functions/

Done.

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:52: using metrics::MinSample;
On 2016/05/06 19:58:40, pbos-webrtc wrote:
> Pref not using using here, metrics::NumSamples is good I think.

Done.

[åsapersson, 2016-05-09 14:48:02]

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

[åsapersson, 2016-05-09 14:50:08]

Magnus, can you also have a look?

[pbos-webrtc, 2016-05-09 21:21:13]

lgtm

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:155: SampleInfo::~SampleInfo()
{}
On 2016/05/09 14:47:41, åsapersson wrote:
> On 2016/05/06 19:58:40, pbos-webrtc wrote:
> > Don't need a dtor I think
> 
> Gives compile error w/o.

Even without ~SampleInfo() in header?

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:26: // linearly/exponentially
spaced buckets) if samples are logged more frequently.
Note that this would be done for performance reasons (to not have to look up
into the map).

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:84: SampleInfo* GetAndReset() {
Should this return a std::unique_ptr?

[juberti2, 2016-05-10 05:29:32]

juberti@chromium.org changed reviewers:
+ juberti@chromium.org

[juberti2, 2016-05-10 05:29:33]

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:107: rtc::CriticalSection
rtc_histograms_crit_;
non-pod data at global scope is not allowed..

[åsapersson, 2016-05-13 09:09:33]

Patchset #13 (id:870001) has been deleted

[åsapersson, 2016-05-13 09:15:53]

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/750001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:155: SampleInfo::~SampleInfo()
{}
On 2016/05/09 21:21:13, pbos-webrtc wrote:
> On 2016/05/09 14:47:41, åsapersson wrote:
> > On 2016/05/06 19:58:40, pbos-webrtc wrote:
> > > Don't need a dtor I think
> > 
> > Gives compile error w/o.
> 
> Even without ~SampleInfo() in header?

yes

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:26: // linearly/exponentially
spaced buckets) if samples are logged more frequently.
On 2016/05/09 21:21:13, pbos-webrtc wrote:
> Note that this would be done for performance reasons (to not have to look up
> into the map).

Acknowledged.

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:84: SampleInfo* GetAndReset() {
On 2016/05/09 21:21:13, pbos-webrtc wrote:
> Should this return a std::unique_ptr?

Done.

https://codereview.webrtc.org/1915523002/diff/790001/webrtc/system_wrappers/s...
webrtc/system_wrappers/source/metrics_default.cc:107: rtc::CriticalSection
rtc_histograms_crit_;
On 2016/05/10 05:29:33, juberti2 wrote:
> non-pod data at global scope is not allowed..

Done.

[åsapersson, 2016-05-13 09:57:55]

Patchset #13 (id:890001) has been deleted

[åsapersson, 2016-05-13 10:53:14]

Patchset #13 (id:910001) has been deleted

[åsapersson, 2016-05-13 10:57:25]

Patchset #13 (id:930001) has been deleted

[åsapersson, 2016-05-13 12:55:58]

Patchset #14 (id:970001) has been deleted

[åsapersson, 2016-05-14 15:47:54]

Patchset #14 (id:990001) has been deleted

[åsapersson, 2016-05-16 06:54:46]

Patchset #14 (id:1010001) has been deleted

[åsapersson, 2016-05-16 08:44:31]

Patchset #14 (id:1030001) has been deleted

[pbos-webrtc, 2016-05-17 16:11:04]

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:35: ~RtcHistogram() {}
remove dtor

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:94: SampleInfo info_;  //
|info_.samples| guarded by |crit_|
GUARDED_BY(crit_), add const int min_ and const int max_ here as well to use
outside of crit_.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:112: const auto& it =
map_.find(name);
auto* it = &map_.find(name), .. maybe. But don't do find twice if you find(name)
at first.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:118: if (map_.find(name) ==
map_.end()) {
same here

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:130: if (info.get() != nullptr)
Does "if (info)" work?

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:188: static volatile int
g_rtc_histogram_status = -1;
I think this should only exist under RTC_DCHECK_IS_ON:

#if defined(RTC_DCHECK_IS_ON)
static volatile int g_rtc_histogram_status = 0;
#endif  // defined(RTC_DCHECK_IS_ON)

in GetMap():

#if defined(RTC_DCHECK_IS_ON)
ReleaseStore(&g_histograms_called, 1);
#endif  // defined(RTC_DCHECK_IS_ON)

in Enable():

RTC_DCHECK_EQ(0, AcquireLoad(&g_histograms_called));
RTC_DCHECK(g_histogram_map == nullptr);
g_histogram_map = new RtcHistogramMap();

This should fairly safely make sure that we never Enable after we start using
histograms.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default_unittest.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default_unittest.cc:60: metrics::Enable();
I think this should go into test_main.cc files etc, not be called per-test ever.

[pbos-webrtc, 2016-05-17 16:12:17]

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:112: const auto& it =
map_.find(name);
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> auto* it = &map_.find(name), .. maybe. But don't do find twice if you
find(name)
> at first.

Something like ...::const_iterator = is probably right, not auto*

[åsapersson, 2016-05-18 09:09:22]

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:35: ~RtcHistogram() {}
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> remove dtor

Done.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:94: SampleInfo info_;  //
|info_.samples| guarded by |crit_|
On 2016/05/17 16:11:03, pbos-webrtc wrote:
> GUARDED_BY(crit_), add const int min_ and const int max_ here as well to use
> outside of crit_.

Done.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:112: const auto& it =
map_.find(name);
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> auto* it = &map_.find(name), .. maybe. But don't do find twice if you
find(name)
> at first.

Changed to not do find twice.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:118: if (map_.find(name) ==
map_.end()) {
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> same here

Done.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:130: if (info.get() != nullptr)
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> Does "if (info)" work?

Done.

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:188: static volatile int
g_rtc_histogram_status = -1;
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> I think this should only exist under RTC_DCHECK_IS_ON:
> 
> #if defined(RTC_DCHECK_IS_ON)
> static volatile int g_rtc_histogram_status = 0;
> #endif  // defined(RTC_DCHECK_IS_ON)
> 
> in GetMap():
> 
> #if defined(RTC_DCHECK_IS_ON)
> ReleaseStore(&g_histograms_called, 1);
> #endif  // defined(RTC_DCHECK_IS_ON)
> 
> in Enable():
> 
> RTC_DCHECK_EQ(0, AcquireLoad(&g_histograms_called));
> RTC_DCHECK(g_histogram_map == nullptr);
> g_histogram_map = new RtcHistogramMap();
> 
> This should fairly safely make sure that we never Enable after we start using
> histograms.

Added check for g_rtc_histogram_called 0 in Enable.
g_rtc_histogram_called set in SetStartedAndGetMap (and not in GetMap since we
don't want to set it when for example calling GetMap in GetAndReset).

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default_unittest.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default_unittest.cc:60: metrics::Enable();
On 2016/05/17 16:11:04, pbos-webrtc wrote:
> I think this should go into test_main.cc files etc, not be called per-test
ever.

Done.

[pbos-webrtc, 2016-05-18 10:30:04]

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:188: static volatile int
g_rtc_histogram_status = -1;
On 2016/05/18 09:09:21, åsapersson wrote:
> On 2016/05/17 16:11:04, pbos-webrtc wrote:
> > I think this should only exist under RTC_DCHECK_IS_ON:
> > 
> > #if defined(RTC_DCHECK_IS_ON)
> > static volatile int g_rtc_histogram_status = 0;
> > #endif  // defined(RTC_DCHECK_IS_ON)
> > 
> > in GetMap():
> > 
> > #if defined(RTC_DCHECK_IS_ON)
> > ReleaseStore(&g_histograms_called, 1);
> > #endif  // defined(RTC_DCHECK_IS_ON)
> > 
> > in Enable():
> > 
> > RTC_DCHECK_EQ(0, AcquireLoad(&g_histograms_called));
> > RTC_DCHECK(g_histogram_map == nullptr);
> > g_histogram_map = new RtcHistogramMap();
> > 
> > This should fairly safely make sure that we never Enable after we start
using
> > histograms.
> 
> Added check for g_rtc_histogram_called 0 in Enable.
> g_rtc_histogram_called set in SetStartedAndGetMap (and not in GetMap since we
> don't want to set it when for example calling GetMap in GetAndReset).

I do think you want to do that in GetMap (but only if DCHECK is enabled), I
think you should initialize metrics before calling GetMap.

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:195: return
rtc::AtomicOps::AcquireLoadPtr(&g_rtc_histogram_map);
You shouldn't have to do atomic operations on g_rtc_histogram_map if it's not
supposed to be racy. g_rtc_histogram_called is there to DCHECK against the data
races, but it should only happen in debug builds.

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:199:
rtc::AtomicOps::ReleaseStore(&g_rtc_histogram_called, 1);
Only in debug, but I think this should be in GetMap()

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:256: void Enable() {
I don't think you should be allowed to call this more than once.

[pbos-webrtc, 2016-05-18 10:32:35]

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:200: RtcHistogramMap* GetMap()
{
I think this should just be:

ReleaseStore(&g_rtc_histogram_status, 1);
return g_rtc_histogram_map; (non-atomically)

[mflodman, 2016-05-18 11:41:25]

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:66: void Reset() {
Do we really need this one too, since it's for testing only we could still use
GetAndReset(). Even if it's a bit more overhead.

[åsapersson, 2016-05-18 12:06:19]

Patchset #16 (id:1090001) has been deleted

[åsapersson, 2016-05-18 12:49:25]

Patchset #16 (id:1110001) has been deleted

[åsapersson, 2016-05-18 12:53:32]

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1050001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:200: RtcHistogramMap* GetMap()
{
On 2016/05/18 10:32:35, pbos-webrtc wrote:
> I think this should just be:
> 
> ReleaseStore(&g_rtc_histogram_status, 1);
> return g_rtc_histogram_map; (non-atomically)

Done.

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:66: void Reset() {
On 2016/05/18 11:41:24, mflodman wrote:
> Do we really need this one too, since it's for testing only we could still use
> GetAndReset(). Even if it's a bit more overhead.

Might be nice to not have to create the map (std::map<std::string,
std::unique_ptr<SampleInfo>>)?

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:195: return
rtc::AtomicOps::AcquireLoadPtr(&g_rtc_histogram_map);
On 2016/05/18 10:30:04, pbos-webrtc wrote:
> You shouldn't have to do atomic operations on g_rtc_histogram_map if it's not
> supposed to be racy. g_rtc_histogram_called is there to DCHECK against the
data
> races, but it should only happen in debug builds.

Done.

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:199:
rtc::AtomicOps::ReleaseStore(&g_rtc_histogram_called, 1);
On 2016/05/18 10:30:03, pbos-webrtc wrote:
> Only in debug, but I think this should be in GetMap()

Done.

https://codereview.webrtc.org/1915523002/diff/1070001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:256: void Enable() {
On 2016/05/18 10:30:04, pbos-webrtc wrote:
> I don't think you should be allowed to call this more than once.

Done.

[pbos-webrtc, 2016-05-19 13:04:25]

lgtm if you fix my nit

https://codereview.webrtc.org/1915523002/diff/1130001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1130001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:191: static volatile int
g_rtc_histogram_called = 0;
Hide within #if defined(RTC_RTC_DCHECK_IS_ON)

https://codereview.webrtc.org/1915523002/diff/1130001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:195:
rtc::AtomicOps::ReleaseStore(&g_rtc_histogram_called, 1);
Hide within #if defined(RTC_RTC_DCHECK_IS_ON)

[åsapersson, 2016-05-19 14:36:33]

https://codereview.webrtc.org/1915523002/diff/1130001/webrtc/system_wrappers/...
File webrtc/system_wrappers/source/metrics_default.cc (right):

https://codereview.webrtc.org/1915523002/diff/1130001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:191: static volatile int
g_rtc_histogram_called = 0;
On 2016/05/19 13:04:25, pbos-webrtc wrote:
> Hide within #if defined(RTC_RTC_DCHECK_IS_ON)

Done.

https://codereview.webrtc.org/1915523002/diff/1130001/webrtc/system_wrappers/...
webrtc/system_wrappers/source/metrics_default.cc:195:
rtc::AtomicOps::ReleaseStore(&g_rtc_histogram_called, 1);
On 2016/05/19 13:04:25, pbos-webrtc wrote:
> Hide within #if defined(RTC_RTC_DCHECK_IS_ON)

Done. And needed below..

[mflodman, 2016-05-20 05:25:04]

Took another careful look, LGTM.

I still have a few minor comments regarding naming etc, but I see this is very
similar to the version in Chromium and I see the point of keeping it similar.

[åsapersson, 2016-05-20 08:33:52]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2016-05-20 08:33:52]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1915523002/#ps1170001
(title: " ")

[commit-bot: I haz the power, 2016-05-20 08:34:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915523002/1170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915523002/1170001

[commit-bot: I haz the power, 2016-05-20 10:09:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-20 10:09:59]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[åsapersson, 2016-05-20 13:27:28]

The CQ bit was checked by asapersson@webrtc.org

[commit-bot: I haz the power, 2016-05-20 13:27:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915523002/1170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915523002/1170001

[commit-bot: I haz the power, 2016-05-20 13:29:49]

Description was changed from

==========
Add a default implementation in metrics_default.cc of histograms methods in
system_wrappers/interface/metrics.h.

Updated tests to use the default implementation and removed the test
implementation (webrtc/test/histograms.h).

BUG=
==========

to

==========
Add a default implementation in metrics_default.cc of histograms methods in
system_wrappers/interface/metrics.h.

Updated tests to use the default implementation and removed the test
implementation (webrtc/test/histograms.h).

BUG=
==========

[commit-bot: I haz the power, 2016-05-20 13:29:52]

Committed patchset #18 (id:1170001)

[commit-bot: I haz the power, 2016-05-20 13:29:56]

Description was changed from

==========
Add a default implementation in metrics_default.cc of histograms methods in
system_wrappers/interface/metrics.h.

Updated tests to use the default implementation and removed the test
implementation (webrtc/test/histograms.h).

BUG=
==========

to

==========
Add a default implementation in metrics_default.cc of histograms methods in
system_wrappers/interface/metrics.h.

Updated tests to use the default implementation and removed the test
implementation (webrtc/test/histograms.h).

BUG=

Committed: https://crrev.com/01d70a3978d5a404aa8b490d1b84ff529123ad4b
Cr-Commit-Position: refs/heads/master@{#12829}
==========

[commit-bot: I haz the power, 2016-05-20 13:29:58]

Patchset 18 (id:??) landed as
https://crrev.com/01d70a3978d5a404aa8b490d1b84ff529123ad4b
Cr-Commit-Position: refs/heads/master@{#12829}