pw_allocator: Refactor metric collection

This CL refactors `pw::allocator::AllocatorMetricProxy` to make metric
collection a build time decision. It moves metric collection to an
`internal::Metrics` class which can be replaced by simple stubs unless a
new `pw_allocator_COLLECT_METRICS` build argument is enabled.

Change-Id: Ie7f9d2c663e10f26f138a482ac9448765d04b0ff
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/180456
Reviewed-by: Taylor Cramer <cramertj@google.com>
Presubmit-Verified: CQ Bot Account <pigweed-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Aaron Green <aarongreen@google.com>

pw_allocator/BUILD.bazel

@@ -57,6 +57,17 @@ pw_cc_library(
     ],
 )
 
+pw_cc_library(
+    name = "allocator_metric_proxy_for_test",
+    hdrs = [
+        "public/pw_allocator/allocator_metric_proxy_for_test.h",
+    ],
+    includes = ["public"],
+    deps = [
+        ":allocator_metric_proxy",
+    ],
+)
+
 pw_cc_library(
     name = "block",
     srcs = ["block.cc"],
@@ -85,8 +96,11 @@ pw_cc_library(
     includes = ["public"],
     deps = [
         ":allocator",
+        ":allocator_metric_proxy",
         "//pw_assert",
+        "//pw_metric:metric",
         "//pw_status",
+        "//pw_tokenizer",
     ],
 )
 
@@ -220,7 +234,7 @@ pw_cc_test(
         "allocator_metric_proxy_test.cc",
     ],
     deps = [
-        ":allocator_metric_proxy",
+        ":allocator_metric_proxy_for_test",
         ":allocator_testing",
         "//pw_unit_test",
     ],

pw_allocator/BUILD.gn

@@ -30,6 +30,12 @@ config("enable_heap_poison") {
   }
 }
 
+config("collect_metrics") {
+  if (pw_allocator_COLLECT_METRICS) {
+    defines = [ "PW_ALLOCATOR_COLLECT_METRICS=1" ]
+  }
+}
+
 group("pw_allocator") {
   public_deps = [
     ":allocator",
@@ -51,7 +57,10 @@ pw_source_set("allocator") {
 }
 
 pw_source_set("allocator_metric_proxy") {
-  public_configs = [ ":default_config" ]
+  public_configs = [
+    ":default_config",
+    ":collect_metrics",
+  ]
   public = [ "public/pw_allocator/allocator_metric_proxy.h" ]
   public_deps = [
     ":allocator",
@@ -62,6 +71,11 @@ pw_source_set("allocator_metric_proxy") {
   sources = [ "allocator_metric_proxy.cc" ]
 }
 
+pw_source_set("allocator_metric_proxy_for_test") {
+  public = [ "public/pw_allocator/allocator_metric_proxy_for_test.h" ]
+  public_deps = [ ":allocator_metric_proxy" ]
+}
+
 pw_source_set("block") {
   public_configs = [
     ":default_config",
@@ -84,9 +98,12 @@ pw_source_set("fallback_allocator") {
   public = [ "public/pw_allocator/fallback_allocator.h" ]
   public_deps = [
     ":allocator",
-    dir_pw_assert,
+    ":allocator_metric_proxy",
+    dir_pw_metric,
     dir_pw_status,
+    dir_pw_tokenizer,
   ]
+  deps = [ dir_pw_assert ]
   sources = [ "fallback_allocator.cc" ]
 }
 
@@ -218,7 +235,7 @@ pw_test("allocator_test") {
 
 pw_test("allocator_metric_proxy_test") {
   deps = [
-    ":allocator_metric_proxy",
+    ":allocator_metric_proxy_for_test",
     ":allocator_testing",
   ]
   sources = [ "allocator_metric_proxy_test.cc" ]

pw_allocator/CMakeLists.txt

@@ -19,6 +19,10 @@ if(pw_allocator_POISON_HEAP)
   set(enable_heap_poison "PW_ALLOCATOR_POISON_ENABLE=1")
 endif()
 
+if(pw_allocator_COLLECT_METRICS)
+  set(collect_metrics "PW_ALLOCATOR_COLLECT_METRICS=1")
+endif()
+
 pw_add_library(pw_allocator.allocator STATIC
   HEADERS
     public/pw_allocator/allocator.h
@@ -34,6 +38,8 @@ pw_add_library(pw_allocator.allocator STATIC
 )
 
 pw_add_library(pw_allocator.allocator_metric_proxy STATIC
+  PUBLIC_DEFINES
+    ${collect_metrics}
   HEADERS
     public/pw_allocator/allocator_metric_proxy.h
   PUBLIC_INCLUDES
@@ -48,6 +54,15 @@ pw_add_library(pw_allocator.allocator_metric_proxy STATIC
     allocator_metric_proxy.cc
 )
 
+pw_add_library(pw_allocator.allocator_metric_proxy_for_test INTERFACE
+  HEADERS
+    public/pw_allocator/allocator_metric_proxy_for_test.h
+  PUBLIC_INCLUDES
+    public
+  PUBLIC_DEPS
+    pw_allocator.allocator_metric_proxy
+)
+
 pw_add_library(pw_allocator.block STATIC
   PUBLIC_DEFINES
     ${enable_heap_poison}
@@ -74,8 +89,12 @@ pw_add_library(pw_allocator.fallback_allocator STATIC
     public
   PUBLIC_DEPS
     pw_allocator.allocator
-    pw_assert
+    pw_allocator.allocator_metric_proxy
+    pw_metric
     pw_status
+    pw_tokenizer
+  PRIVATE_DEPS
+    pw_assert
   SOURCES
     fallback_allocator.cc
 )
@@ -192,7 +211,7 @@ pw_add_test(pw_allocator.allocator_metric_proxy_test
   SOURCES
     allocator_metric_proxy_test.cc
   PRIVATE_DEPS
-    pw_allocator.allocator_metric_proxy
+    pw_allocator.allocator_metric_proxy_for_test
     pw_allocator.allocator_testing
   GROUPS
     modules

pw_allocator/allocator.cmake

@@ -19,3 +19,8 @@ set(pw_allocator_POISON_HEAP OFF CACHE STRING
     "When heap poisoning is enabled, a hard-coded randomized pattern will be \
     added before and after the usable space of each Block. The allocator will \
     check that the pattern is unchanged when freeing a block.")
+
+set(pw_allocator_COLLECT_METRICS OFF CACHE STRING
+    "Adds a `pw::metric::MerticAccumulation` to `AllocatorProxy`. This \
+    increases the code size a non-trivial amount, but allows tracking how much \
+    memory each allocator proxy has allocated.")

pw_allocator/allocator.gni

@@ -17,4 +17,9 @@ declare_args() {
   # added before and after the usable space of each Block. The allocator will
   # check that the pattern is unchanged when freeing a block.
   pw_allocator_POISON_HEAP = false
+
+  # Adds a `pw::metric::MerticAccumulation` to `AllocatorProxy`. This increases
+  # the code size a non-trivial amount, but allows tracking how much memory
+  # each allocator proxy has allocated.
+  pw_allocator_COLLECT_METRICS = false
 }

pw_allocator/allocator_metric_proxy.cc

@@ -19,60 +19,34 @@
 #include "pw_assert/check.h"
 #include "pw_metric/metric.h"
 
-namespace pw::allocator {
+namespace pw::allocator::internal {
 
-void AllocatorMetricProxy::Initialize(Allocator& allocator) {
-  PW_DCHECK(allocator_ == nullptr);
-  allocator_ = &allocator;
-  // Manually add the metrics to the metric group to allow the constructor to
-  // remain constexpr.
-  memusage_.Add(used_);
-  memusage_.Add(peak_);
-  memusage_.Add(count_);
+void Metrics::Init() {
+  Add(used_);
+  Add(peak_);
+  Add(count_);
 }
 
-Status AllocatorMetricProxy::DoQuery(const void* ptr, Layout layout) const {
-  PW_DCHECK_NOTNULL(allocator_);
-  return allocator_->Query(ptr, layout);
-}
-
-void* AllocatorMetricProxy::DoAllocate(Layout layout) {
-  PW_DCHECK_NOTNULL(allocator_);
-  void* ptr = allocator_->Allocate(layout);
-  if (ptr == nullptr) {
-    return nullptr;
-  }
-  used_.Increment(layout.size());
-  if (used_.value() > peak_.value()) {
-    peak_.Set(used_.value());
-  }
-  count_.Increment();
-  return ptr;
-}
-
-void AllocatorMetricProxy::DoDeallocate(void* ptr, Layout layout) {
-  PW_DCHECK_NOTNULL(allocator_);
-  allocator_->Deallocate(ptr, layout);
-  if (ptr == nullptr) {
-    return;
+void Metrics::Update(size_t old_size, size_t new_size) {
+  size_t used = used_.value();
+  size_t count = count_.value();
+  if (old_size != 0) {
+    PW_DCHECK_UINT_GE(used, old_size);
+    PW_DCHECK_UINT_GT(count, 0);
+    used -= old_size;
+    --count;
   }
-  PW_DCHECK_UINT_GE(used_.value(), layout.size());
-  PW_DCHECK_UINT_NE(count_.value(), 0);
-  used_.Set(used_.value() - layout.size());
-  count_.Set(count_.value() - 1);
-}
-
-bool AllocatorMetricProxy::DoResize(void* ptr, Layout layout, size_t new_size) {
-  PW_DCHECK_NOTNULL(allocator_);
-  if (!allocator_->Resize(ptr, layout, new_size)) {
-    return false;
+  if (new_size != 0) {
+    used += new_size;
+    ++count;
+    PW_DCHECK_UINT_GE(used, new_size);
+    PW_DCHECK_UINT_GT(count, 0);
   }
-  PW_DCHECK_UINT_GE(used_.value(), layout.size());
-  used_.Set(used_.value() - layout.size() + new_size);
-  if (used_.value() > peak_.value()) {
-    peak_.Set(used_.value());
+  if (used > peak_.value()) {
+    peak_.Set(used);
   }
-  return true;
+  used_.Set(used);
+  count_.Set(count);
 }
 
-}  // namespace pw::allocator
+}  // namespace pw::allocator::internal