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

webrtc/system_wrappers/source/metrics_default.cc

 // Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
 //
 // Use of this source code is governed by a BSD-style license
 // that can be found in the LICENSE file in the root of the source
 // tree. An additional intellectual property rights grant can be found
 // in the file PATENTS.  All contributing project authors may
 // be found in the AUTHORS file in the root of the source tree.
 //
 
+#include "webrtc/system_wrappers/include/metrics_default.h"
+
+#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 webrtc {
 namespace metrics {
-
-Histogram* HistogramFactoryGetCounts(const std::string& name, int min, int max,
-    int bucket_count) { return NULL; }
-
+class Histogram;
+
+namespace {
+// Limit for the maximum number of sample values that can be stored.
+// TODO(asapersson): Consider using bucket count (and set up
+// linearly/exponentially spaced buckets) if samples are logged more frequently.
+const int kMaxSampleMapSize = 2000;
+
+class RtcHistogram {
+ public:
+  RtcHistogram(const std::string& name, int min, int max, int bucket_count)
+      : info_(name, min, max, bucket_count) {
+    RTC_CHECK_GT(bucket_count, 0);
+  }
+  ~RtcHistogram() {}

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

remove dtor

[åsapersson, 2016/05/18 09:09:21]

Done.

+
+  void Add(int sample) {
+    if (sample < info_.min)
+      sample = info_.min - 1;  // Underflow bucket.
+    if (sample > info_.max)
+      sample = info_.max;
+
+    rtc::CritScope cs(&crit_);
+    if (info_.samples.size() == kMaxSampleMapSize &&
+        info_.samples.find(sample) == info_.samples.end()) {
+      return;
+    }
+    ++info_.samples[sample];
+  }
+
+  // Returns a copy (or nullptr if there are no samples) and clears samples.
+  std::unique_ptr<SampleInfo> GetAndReset() {
+    rtc::CritScope cs(&crit_);
+    if (info_.samples.empty())
+      return nullptr;
+
+    SampleInfo* copy =
+        new SampleInfo(info_.name, info_.min, info_.max, info_.bucket_count);
+    copy->samples = info_.samples;
+    info_.samples.clear();
+    return std::unique_ptr<SampleInfo>(copy);
+  }
+
+  const std::string& name() const { return info_.name; }
+
+  // Functions only for testing.
+  void Reset() {
+    rtc::CritScope cs(&crit_);
+    info_.samples.clear();
+  }
+
+  int NumEvents(int sample) const {
+    rtc::CritScope cs(&crit_);
+    const auto it = info_.samples.find(sample);
+    return (it == info_.samples.end()) ? 0 : it->second;
+  }
+
+  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_);
+    return (info_.samples.empty()) ? -1 : info_.samples.begin()->first;
+  }
+
+ private:
+  rtc::CriticalSection crit_;
+  SampleInfo info_;  // |info_.samples| guarded by |crit_|

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

GUARDED_BY(crit_), add const int min_ and const int max_ here as well to use
outside of crit_.

[åsapersson, 2016/05/18 09:09:21]

Done.

+
+  RTC_DISALLOW_COPY_AND_ASSIGN(RtcHistogram);
+};
+
+class RtcHistogramMap {
+ public:
+  RtcHistogramMap() {}
+  ~RtcHistogramMap() {}
+
+  Histogram* GetCountsHistogram(const std::string& name,
+                                int min,
+                                int max,
+                                int bucket_count) {
+    rtc::CritScope cs(&crit_);
+    if (map_.find(name) == map_.end()) {
+      map_[name].reset(new RtcHistogram(name, min, max, bucket_count));
+    }
+    const auto& it = map_.find(name);

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

auto* it = &map_.find(name), .. maybe. But don't do find twice if you find(name)
at first.

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

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

[åsapersson, 2016/05/18 09:09:21]

Changed to not do find twice.

+    return reinterpret_cast<Histogram*>(it->second.get());
+  }
+
+  Histogram* GetEnumerationHistogram(const std::string& name, int boundary) {
+    rtc::CritScope cs(&crit_);
+    if (map_.find(name) == map_.end()) {

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

same here

[åsapersson, 2016/05/18 09:09:21]

Done.

+      map_[name].reset(new RtcHistogram(name, 1, boundary, boundary + 1));
+    }
+    const auto& it = map_.find(name);
+    return reinterpret_cast<Histogram*>(it->second.get());
+  }
+
+  void GetAndReset(
+      std::map<std::string, std::unique_ptr<SampleInfo>>* histograms) {
+    rtc::CritScope cs(&crit_);
+    for (const auto& kv : map_) {
+      std::unique_ptr<SampleInfo> info = kv.second->GetAndReset();
+      if (info.get() != nullptr)

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

Does "if (info)" work?

[åsapersson, 2016/05/18 09:09:21]

Done.

+        histograms->insert(std::make_pair(kv.first, std::move(info)));
+    }
+  }
+
+  // Functions only for testing.
+  void Reset() {
+    rtc::CritScope cs(&crit_);
+    for (const auto& kv : map_)
+      kv.second->Reset();
+  }
+
+  int NumEvents(const std::string& name, int sample) const {
+    rtc::CritScope cs(&crit_);
+    const auto& it = map_.find(name);
+    return (it == map_.end()) ? 0 : it->second->NumEvents(sample);
+  }
+
+  int NumSamples(const std::string& name) const {
+    rtc::CritScope cs(&crit_);
+    const auto& it = map_.find(name);
+    return (it == map_.end()) ? 0 : it->second->NumSamples();
+  }
+
+  int MinSample(const std::string& name) const {
+    rtc::CritScope cs(&crit_);
+    const auto& it = map_.find(name);
+    return (it == map_.end()) ? -1 : it->second->MinSample();
+  }
+
+ private:
+  rtc::CriticalSection crit_;
+  std::map<std::string, std::unique_ptr<RtcHistogram>> map_ GUARDED_BY(crit_);
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(RtcHistogramMap);
+};
+
+// RtcHistogramMap is allocated upon call to Enable().
+// The histogram getter functions, which return pointer values to the histograms
+// in the map, are cached in WebRTC. Therefore, this memory is not freed by the
+// application (the memory will be reclaimed by the OS).
+static RtcHistogramMap* volatile g_rtc_histogram_map = nullptr;
+
+void CreateMap() {
+  RtcHistogramMap* map = rtc::AtomicOps::AcquireLoadPtr(&g_rtc_histogram_map);
+  if (map == nullptr) {
+    RtcHistogramMap* new_map = new RtcHistogramMap();
+    RtcHistogramMap* old_map = rtc::AtomicOps::CompareAndSwapPtr(
+        &g_rtc_histogram_map, static_cast<RtcHistogramMap*>(nullptr), new_map);
+    if (old_map != nullptr)
+      delete new_map;
+  }
+}
+
+// Set (-1 -> 0 or 1) when the first histogram is created (and is not changed
+// thereafter). Calling Enable() after the status is set has no effect.
+//  0: not enabled - GetMap() -> nullptr
+//  1: enabled     - GetMap() -> g_rtc_histogram_map
+static volatile int g_rtc_histogram_status = -1;

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

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.

[åsapersson, 2016/05/18 09:09:21]

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).

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

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.

+
+void InitStatus() {
+  int status = rtc::AtomicOps::AcquireLoad(&g_rtc_histogram_status);
+  if (status == -1) {
+    RtcHistogramMap* map = rtc::AtomicOps::AcquireLoadPtr(&g_rtc_histogram_map);
+    int new_status = (map == nullptr) ? 0 : 1;
+    rtc::AtomicOps::CompareAndSwap(&g_rtc_histogram_status, -1, new_status);
+  }
+}
+
+// Gets the map (or nullptr).
+RtcHistogramMap* GetMap() {

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

I think this should just be:

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

[åsapersson, 2016/05/18 12:53:31]

Done.

+  RtcHistogramMap* map = nullptr;
+  int status = rtc::AtomicOps::AcquireLoad(&g_rtc_histogram_status);
+  if (status == 1) {
+    map = rtc::AtomicOps::AcquireLoadPtr(&g_rtc_histogram_map);
+  }
+  return map;
+}
+
+RtcHistogramMap* InitStatusAndGetMap() {
+  InitStatus();
+  return GetMap();
+}
+}  // namespace
+
+// Implementation of histogram methods in
+// webrtc/system_wrappers/interface/metrics.h.
+
+// Histogram with exponentially spaced buckets.
+// Creates (or finds) histogram.
+// The returned histogram pointer is cached (and used for adding samples in
+// subsequent calls).
+Histogram* HistogramFactoryGetCounts(const std::string& name,
+                                     int min,
+                                     int max,
+                                     int bucket_count) {
+  RtcHistogramMap* map = InitStatusAndGetMap();
+  if (!map)
+    return nullptr;
+
+  return map->GetCountsHistogram(name, min, max, bucket_count);
+}
+
+// Histogram with linearly spaced buckets.
+// Creates (or finds) histogram.
+// The returned histogram pointer is cached (and used for adding samples in
+// subsequent calls).
 Histogram* HistogramFactoryGetEnumeration(const std::string& name,
-    int boundary) { return NULL; }
-
-void HistogramAdd(
-    Histogram* histogram_pointer, const std::string& name, int sample) {}
+                                          int boundary) {
+  RtcHistogramMap* map = InitStatusAndGetMap();
+  if (!map)
+    return nullptr;
+
+  return map->GetEnumerationHistogram(name, boundary);
+}
+
+// Fast path. Adds |sample| to cached |histogram_pointer|.
+void HistogramAdd(Histogram* histogram_pointer,
+                  const std::string& name,
+                  int sample) {
+  if (!histogram_pointer)
+    return;
+
+  RtcHistogram* ptr = reinterpret_cast<RtcHistogram*>(histogram_pointer);
+  RTC_DCHECK_EQ(name, ptr->name()) << "The name should not vary.";
+  ptr->Add(sample);
+}
+
+SampleInfo::SampleInfo(const std::string& name,
+                       int min,
+                       int max,
+                       size_t bucket_count)
+    : name(name), min(min), max(max), bucket_count(bucket_count) {}
+
+SampleInfo::~SampleInfo() {}
+
+// Implementation of global functions in metrics_default.h.
+void Enable() {
+  CreateMap();
+}
+
+void GetAndReset(
+    std::map<std::string, std::unique_ptr<SampleInfo>>* histograms) {
+  histograms->clear();
+  RtcHistogramMap* map = GetMap();
+  if (map)
+    map->GetAndReset(histograms);
+}
+
+void Reset() {
+  RtcHistogramMap* map = GetMap();
+  if (map)
+    map->Reset();
+}
+
+int NumEvents(const std::string& name, int sample) {
+  RtcHistogramMap* map = GetMap();
+  return map ? map->NumEvents(name, sample) : 0;
+}
+
+int NumSamples(const std::string& name) {
+  RtcHistogramMap* map = GetMap();
+  return map ? map->NumSamples(name) : 0;
+}
+
+int MinSample(const std::string& name) {
+  RtcHistogramMap* map = GetMap();
+  return map ? map->MinSample(name) : -1;
+}
 
 }  // namespace metrics
 }  // namespace webrtc
-