diff --git a/cartographer_ros/cartographer_ros/metrics/internal/counter.h b/cartographer_ros/cartographer_ros/metrics/internal/counter.h index d7747dd..52f35fe 100644 --- a/cartographer_ros/cartographer_ros/metrics/internal/counter.h +++ b/cartographer_ros/cartographer_ros/metrics/internal/counter.h @@ -29,11 +29,11 @@ class Counter : public ::cartographer::metrics::Counter { explicit Counter(const std::map& labels) : gauge_(labels) {} - void Increment(const double by_value) override { gauge_.Increment(by_value); } + void Increment(const double value) override { gauge_.Increment(value); } void Increment() override { gauge_.Increment(); } - double Value() const { return gauge_.Value(); } + double Value() { return gauge_.Value(); } cartographer_ros_msgs::Metric ToRosMessage() { cartographer_ros_msgs::Metric msg = gauge_.ToRosMessage(); diff --git a/cartographer_ros/cartographer_ros/metrics/internal/family.h b/cartographer_ros/cartographer_ros/metrics/internal/family.h index 56f118d..7e21f58 100644 --- a/cartographer_ros/cartographer_ros/metrics/internal/family.h +++ b/cartographer_ros/cartographer_ros/metrics/internal/family.h @@ -62,7 +62,7 @@ class HistogramFamily : public ::cartographer::metrics::Family< public: HistogramFamily(const std::string& name, const std::string& description, const BucketBoundaries& boundaries) - : boundaries_(boundaries) {} + : name_(name), description_(description), boundaries_(boundaries) {} Histogram* Add(const std::map& labels) override; diff --git a/cartographer_ros/cartographer_ros/metrics/internal/gauge.h b/cartographer_ros/cartographer_ros/metrics/internal/gauge.h index 7f1c173..e7e4006 100644 --- a/cartographer_ros/cartographer_ros/metrics/internal/gauge.h +++ b/cartographer_ros/cartographer_ros/metrics/internal/gauge.h @@ -17,10 +17,10 @@ #ifndef CARTOGRAPHER_ROS_METRICS_INTERNAL_GAUGE_H #define CARTOGRAPHER_ROS_METRICS_INTERNAL_GAUGE_H -#include #include #include +#include "cartographer/common/mutex.h" #include "cartographer/metrics/gauge.h" #include "cartographer_ros_msgs/Metric.h" @@ -30,23 +30,25 @@ namespace metrics { class Gauge : public ::cartographer::metrics::Gauge { public: explicit Gauge(const std::map& labels) - : labels_(labels) {} + : labels_(labels), value_(0.) {} - void Decrement(const double by_value) override { Add(-1. * by_value); } + void Decrement(const double value) override { Add(-1. * value); } void Decrement() override { Decrement(1.); } - void Increment(const double by_value) override { Add(by_value); } + void Increment(const double value) override { Add(value); } void Increment() override { Increment(1.); } void Set(double value) override { - double expected = value_.load(); - while (!value_.compare_exchange_weak(expected, value)) { - ; - } + ::cartographer::common::MutexLocker lock(&mutex_); + value_ = value; + } + + double Value() { + ::cartographer::common::MutexLocker lock(&mutex_); + return value_; } - double Value() const { return value_.load(); } cartographer_ros_msgs::Metric ToRosMessage() { cartographer_ros_msgs::Metric msg; @@ -63,12 +65,13 @@ class Gauge : public ::cartographer::metrics::Gauge { private: void Add(const double value) { - double current = Value(); - Set(current + value); + ::cartographer::common::MutexLocker lock(&mutex_); + value_ += value; } + cartographer::common::Mutex mutex_; const std::map labels_; - std::atomic value_{0.}; + double value_ GUARDED_BY(mutex_); }; } // namespace metrics diff --git a/cartographer_ros/cartographer_ros/metrics/internal/histogram.cc b/cartographer_ros/cartographer_ros/metrics/internal/histogram.cc index ee9123a..6e209f2 100644 --- a/cartographer_ros/cartographer_ros/metrics/internal/histogram.cc +++ b/cartographer_ros/cartographer_ros/metrics/internal/histogram.cc @@ -36,15 +36,11 @@ Histogram::Histogram(const std::map& labels, std::end(bucket_boundaries_))); } -Histogram::Histogram(const BucketBoundaries& bucket_boundaries) - : Histogram({}, bucket_boundaries) {} - void Histogram::Observe(double value) { auto bucket_index = std::distance(bucket_boundaries_.begin(), std::upper_bound(bucket_boundaries_.begin(), bucket_boundaries_.end(), value)); - // TODO: check lock contention and potential for atomic operations. ::cartographer::common::MutexLocker lock(&mutex_); sum_ += value; bucket_counts_[bucket_index] += 1; diff --git a/cartographer_ros/cartographer_ros/metrics/internal/histogram.h b/cartographer_ros/cartographer_ros/metrics/internal/histogram.h index 46ac758..4846f34 100644 --- a/cartographer_ros/cartographer_ros/metrics/internal/histogram.h +++ b/cartographer_ros/cartographer_ros/metrics/internal/histogram.h @@ -36,8 +36,6 @@ class Histogram : public ::cartographer::metrics::Histogram { explicit Histogram(const std::map& labels, const BucketBoundaries& bucket_boundaries); - explicit Histogram(const BucketBoundaries& bucket_boundaries); - void Observe(double value) override; std::map CountsByBucket(); diff --git a/cartographer_ros/cartographer_ros/metrics/internal/metrics_test.cc b/cartographer_ros/cartographer_ros/metrics/internal/metrics_test.cc index 8a01a7d..d0c8681 100644 --- a/cartographer_ros/cartographer_ros/metrics/internal/metrics_test.cc +++ b/cartographer_ros/cartographer_ros/metrics/internal/metrics_test.cc @@ -53,7 +53,7 @@ TEST(Metrics, CounterTest) { TEST(Metrics, HistogramFixedWidthTest) { auto boundaries = ::cartographer::metrics::Histogram::FixedWidth(1, 3); - Histogram histogram(boundaries); + Histogram histogram({}, boundaries); // Observe some values that fit in finite buckets. std::array values = {{0., 2, 2.5}}; @@ -78,7 +78,7 @@ TEST(Metrics, HistogramFixedWidthTest) { TEST(Metrics, HistogramScaledPowersOfTest) { auto boundaries = ::cartographer::metrics::Histogram::ScaledPowersOf(2, 1, 2048); - Histogram histogram(boundaries); + Histogram histogram({}, boundaries); // Observe some values that fit in finite buckets. std::array values = {{256, 512, 666}};