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

webrtc/system_wrappers/source/metrics_default.cc

Index: webrtc/system_wrappers/source/metrics_default.cc
diff --git a/webrtc/system_wrappers/source/metrics_default.cc b/webrtc/system_wrappers/source/metrics_default.cc
index 48c9111e1b817397d91d7b140055fadbdcbdb75a..8c935ac230413983e4b9dd31eb57a5a03eca321c 100644
--- a/webrtc/system_wrappers/source/metrics_default.cc
+++ b/webrtc/system_wrappers/source/metrics_default.cc
@@ -7,23 +7,201 @@
 // be found in the AUTHORS file in the root of the source tree.
 //
 
+#include "webrtc/system_wrappers/include/metrics_default.h"
+
+#include <memory>
+
+#include "webrtc/base/criticalsection.h"
+#include "webrtc/base/thread_annotations.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 // Default implementation of histogram methods for WebRTC clients that do not
 // want to provide their own implementation.
 
+namespace {
+// Limit for the maximum number of sample values that can be stored.
+const int kMaxSampleMapSize = 10000;

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

I think this should be covered by bucket_count?

[åsapersson, 2016/05/09 14:47:41]

see other comment

+
+class RtcHistogram {
+ public:
+  RtcHistogram(const std::string& name, int min, int max, int bucket_count)
+      : info_(name, min, max, bucket_count) {}

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

I think the ctor here should set the size of samples_ and we should index into
it directly.

[åsapersson, 2016/05/09 14:47:41]

see other comment

+  ~RtcHistogram() {}
+
+  int NumSamples() const {
+    int num_samples = 0;
+    rtc::CritScope cs(&crit_);
+    for (const auto& sample : info_.samples) {
+      num_samples += sample.second;
+    }
+    return num_samples;
+  }
+
+  int MinSample() const {
+    rtc::CritScope cs(&crit_);
+    if (info_.samples.empty()) {
+      return -1;
+    }
+    return info_.samples.begin()->first;
+  }
+
+  int NumEvents(int sample) const {
+    rtc::CritScope cs(&crit_);
+    const auto it = info_.samples.find(sample);
+    if (info_.samples.find(sample) == info_.samples.end()) {
+      return 0;
+    }
+    return it->second;
+  }
+
+  void Add(int sample) {
+    if (sample < 0)
+      sample = 0;

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

Is this an invalid sample? E.g. should we DCHECK instead?

[åsapersson, 2016/05/09 14:47:41]

Changed to limit by min bucket value.

+    if (sample > info_.max)
+      sample = info_.max;
+
+    rtc::CritScope cs(&crit_);
+    if (info_.samples.size() == kMaxSampleMapSize &&

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

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.

[åsapersson, 2016/05/09 14:47:41]

see other comment

+        info_.samples.find(sample) == info_.samples.end()) {
+      return;
+    }
+    ++info_.samples[sample];
+  }
+
+  // Clears samples.
+  void Reset() {
+    rtc::CritScope cs(&crit_);
+    info_.samples.clear();
+  }
+
+  // Returns a copy (or nullptr if there are no samples) and clears samples.
+  webrtc::metrics::SampleInfo* GetAndReset() {

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

Can this just be SampleInfo? You can put the anonymous namespace inside
webrtc::metrics.

[åsapersson, 2016/05/09 14:47:41]

Done.

+    rtc::CritScope cs(&crit_);
+    if (info_.samples.empty())
+      return nullptr;
+
+    webrtc::metrics::SampleInfo* copy = new webrtc::metrics::SampleInfo(
+        info_.name, info_.min, info_.max, info_.bucket_count);
+    copy->samples = info_.samples;
+    info_.samples.clear();
+    return copy;
+  }
+
+  const std::string& name() const { return info_.name; }
+
+ private:
+  rtc::CriticalSection crit_;
+  webrtc::metrics::SampleInfo info_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(RtcHistogram);
+};
+
+// Map holding samples of a histogram (mapped by the histogram name).
+bool rtc_histograms_enabled_ = false;
+rtc::CriticalSection rtc_histograms_crit_;
+std::map<std::string, std::unique_ptr<RtcHistogram>> rtc_histograms_
+    GUARDED_BY(rtc_histograms_crit_);
+}  // namespace
+
 namespace webrtc {
 namespace metrics {
+class Histogram;
+
+// Implementation of histogram methods in
+// webrtc/system_wrappers/interface/metrics.h.
 
-Histogram* HistogramFactoryGetCounts(const std::string& name, int min, int max,
-    int bucket_count) { return NULL; }
+// Histogram with exponentially spaced buckets.
+Histogram* HistogramFactoryGetCounts(const std::string& name,
+                                     int min,
+                                     int max,
+                                     int bucket_count) {
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  if (rtc_histograms_.find(name) == rtc_histograms_.end()) {
+    rtc_histograms_[name].reset(new RtcHistogram(name, min, max, bucket_count));
+  }
+  const auto& it = rtc_histograms_.find(name);
+  return reinterpret_cast<Histogram*>(it->second.get());
+}
 
+// Histogram with linearly spaced buckets.
 Histogram* HistogramFactoryGetEnumeration(const std::string& name,
-    int boundary) { return NULL; }
+                                          int boundary) {
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  if (rtc_histograms_.find(name) == rtc_histograms_.end()) {
+    rtc_histograms_[name].reset(
+        new RtcHistogram(name, 1, boundary, boundary + 1));
+  }
+  const auto& it = rtc_histograms_.find(name);
+  return reinterpret_cast<Histogram*>(it->second.get());
+}
+
+void HistogramAdd(Histogram* histogram_pointer,
+                  const std::string& name,
+                  int sample) {
+  if (!rtc_histograms_enabled_)
+    return;
+
+  RtcHistogram* ptr = reinterpret_cast<RtcHistogram*>(histogram_pointer);
+  RTC_DCHECK(ptr->name() == name) << "The name should not vary.";

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

RTC_DCHECK_EQ(name, ptr->name())?

[åsapersson, 2016/05/09 14:47:41]

Done.

+  ptr->Add(sample);
+}
+
+SampleInfo::SampleInfo(const std::string& name,
+                       int min,
+                       int max,
+                       int bucket_count)
+    : name(name), min(min), max(max), bucket_count(bucket_count) {}
+
+SampleInfo::~SampleInfo() {}

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

Don't need a dtor I think

[åsapersson, 2016/05/09 14:47:41]

Gives compile error w/o.

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

Even without ~SampleInfo() in header?

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

yes

+
+// Implementation of methods in metrics_default.h.

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

s/methods/global functions/

[åsapersson, 2016/05/09 14:47:41]

Done.

+void Enable() {
+  rtc_histograms_enabled_ = true;
+}
+
+void GetAndReset(
+    std::map<std::string, std::unique_ptr<SampleInfo>>* histograms) {
+  histograms->clear();
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  for (const auto& kv : rtc_histograms_) {
+    SampleInfo* info = kv.second->GetAndReset();
+    if (info)
+      (*histograms)[kv.first].reset(info);
+  }
+}
 
-void HistogramAdd(
-    Histogram* histogram_pointer, const std::string& name, int sample) {}
+void Reset() {
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  for (const auto& kv : rtc_histograms_)
+    kv.second->Reset();
+}
+
+int NumEvents(const std::string& name, int sample) {
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  const auto& it = rtc_histograms_.find(name);
+  if (it == rtc_histograms_.end())
+    return 0;
+
+  return it->second->NumEvents(sample);
+}
+
+int NumSamples(const std::string& name) {
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  const auto& it = rtc_histograms_.find(name);
+  if (it == rtc_histograms_.end())
+    return 0;
+
+  return it->second->NumSamples();
+}
+
+int MinSample(const std::string& name) {
+  rtc::CritScope cs(&rtc_histograms_crit_);
+  const auto& it = rtc_histograms_.find(name);
+  if (it == rtc_histograms_.end())
+    return -1;
+
+  return it->second->MinSample();
+}
 
 }  // namespace metrics
 }  // namespace webrtc
-