From 5590deda1e18277d3f0bd0e89d301d45a85ef8d1 Mon Sep 17 00:00:00 2001
From: Robert Bindar <robertbindar@gmail.com>
Date: Thu, 8 Feb 2024 16:58:21 +0200
Subject: [PATCH 1/3] Remove serialization non C41 constructors from stats

With this change in, PR #4685 and others to follow should:

a) remove Subarray::set_stats and respective call from serialization/query.cc
b) where Subarray is constructed in serialization/query.cc, change constructor
call with the following code snippet:
 auto &stats_data = stats_from_capnp(reader.getStats());
 Subarray subarray(array, layout, query_stats, stats_data, dummy_logger, true);
c) The constructor calls parent_stats->create_child(prefix, stats_data);
d) When all migrations are done, make Stats::populate_with_data private
---
 tiledb/sm/query/query.cc                   |  4 ++
 tiledb/sm/query/query.h                    |  5 ++
 tiledb/sm/query/strategy_base.cc           |  4 ++
 tiledb/sm/query/strategy_base.h            |  5 ++
 tiledb/sm/serialization/query.cc           | 77 +++++++---------------
 tiledb/sm/stats/global_stats.h             | 68 ++++++++++++++++++-
 tiledb/sm/stats/stats.cc                   | 26 +++++++-
 tiledb/sm/stats/stats.h                    | 44 ++++++++++++-
 tiledb/sm/subarray/subarray.cc             |  4 ++
 tiledb/sm/subarray/subarray.h              |  5 ++
 tiledb/sm/subarray/subarray_partitioner.cc |  4 ++
 tiledb/sm/subarray/subarray_partitioner.h  |  5 ++
 12 files changed, 192 insertions(+), 59 deletions(-)

diff --git a/tiledb/sm/query/query.cc b/tiledb/sm/query/query.cc
index 32bcd7199b6..849eac4b294 100644
--- a/tiledb/sm/query/query.cc
+++ b/tiledb/sm/query/query.cc
@@ -1663,6 +1663,10 @@ stats::Stats* Query::stats() const {
   return stats_;
 }
 
+void Query::set_stats(const stats::StatsData& data) {
+  stats_->populate_with_data(data);
+}
+
 shared_ptr<Buffer> Query::rest_scratch() const {
   return rest_scratch_;
 }
diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h
index c5c8ecba2db..125268f140b 100644
--- a/tiledb/sm/query/query.h
+++ b/tiledb/sm/query/query.h
@@ -655,6 +655,11 @@ class Query {
   /** Returns the internal stats object. */
   stats::Stats* stats() const;
 
+  /** Populate the owned stats instance with data.
+   * To be removed when the class will get a C41 constructor.
+   */
+  void set_stats(const stats::StatsData& data);
+
   /** Returns the scratch space used for REST requests. */
   shared_ptr<Buffer> rest_scratch() const;
 
diff --git a/tiledb/sm/query/strategy_base.cc b/tiledb/sm/query/strategy_base.cc
index 393a1bff0c6..09406c734de 100644
--- a/tiledb/sm/query/strategy_base.cc
+++ b/tiledb/sm/query/strategy_base.cc
@@ -65,6 +65,10 @@ stats::Stats* StrategyBase::stats() const {
   return stats_;
 }
 
+void StrategyBase::set_stats(const stats::StatsData& data) {
+  stats_->populate_with_data(data);
+}
+
 /* ****************************** */
 /*       PROTECTED METHODS        */
 /* ****************************** */
diff --git a/tiledb/sm/query/strategy_base.h b/tiledb/sm/query/strategy_base.h
index 91bc7194640..9b23c133125 100644
--- a/tiledb/sm/query/strategy_base.h
+++ b/tiledb/sm/query/strategy_base.h
@@ -208,6 +208,11 @@ class StrategyBase {
   /** Returns `stats_`. */
   stats::Stats* stats() const;
 
+  /** Populate the owned stats instance with data.
+   * To be removed when the class will get a C41 constructor.
+   */
+  void set_stats(const stats::StatsData& data);
+
   /** Returns the configured offsets format mode. */
   std::string offsets_mode() const;
 
diff --git a/tiledb/sm/serialization/query.cc b/tiledb/sm/serialization/query.cc
index 75162c69347..45c20ff6e2b 100644
--- a/tiledb/sm/serialization/query.cc
+++ b/tiledb/sm/serialization/query.cc
@@ -118,27 +118,27 @@ Status stats_to_capnp(Stats& stats, capnp::Stats::Builder* stats_builder) {
   return Status::Ok();
 }
 
-Status stats_from_capnp(
-    const capnp::Stats::Reader& stats_reader, Stats* stats) {
+StatsData stats_from_capnp(const capnp::Stats::Reader& stats_reader) {
+  std::unordered_map<std::string, uint64_t> counters;
+  std::unordered_map<std::string, double> timers;
+
   if (stats_reader.hasCounters()) {
-    auto counters = stats->counters();
     auto counters_reader = stats_reader.getCounters();
     for (const auto entry : counters_reader.getEntries()) {
       auto key = std::string_view{entry.getKey().cStr(), entry.getKey().size()};
-      (*counters)[std::string{key}] = entry.getValue();
+      counters[std::string(key)] = entry.getValue();
     }
   }
 
   if (stats_reader.hasTimers()) {
-    auto timers = stats->timers();
     auto timers_reader = stats_reader.getTimers();
     for (const auto entry : timers_reader.getEntries()) {
       auto key = std::string_view{entry.getKey().cStr(), entry.getKey().size()};
-      (*timers)[std::string{key}] = entry.getValue();
+      timers[std::string(key)] = entry.getValue();
     }
   }
 
-  return Status::Ok();
+  return stats::StatsData(counters, timers);
 }
 
 void range_buffers_to_capnp(
@@ -328,11 +328,8 @@ Status subarray_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader.hasStats()) {
-    stats::Stats* stats = subarray->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader.getStats());
+    subarray->set_stats(stats_data);
   }
 
   if (reader.hasRelevantFragments()) {
@@ -566,11 +563,8 @@ Status subarray_partitioner_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader.hasStats()) {
-    auto stats = partitioner->stats();
-    // We should always have stats
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader.getStats());
+    partitioner->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1146,11 +1140,8 @@ Status reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1187,11 +1178,8 @@ Status index_reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1229,11 +1217,8 @@ Status dense_reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1252,11 +1237,8 @@ Status delete_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (delete_reader.hasStats()) {
-    stats::Stats* stats = delete_strategy->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(delete_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(delete_reader.getStats());
+    delete_strategy->set_stats(stats_data);
   }
 
   return Status::Ok();
@@ -1341,11 +1323,8 @@ Status writer_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (writer_reader.hasStats()) {
-    stats::Stats* stats = writer->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(writer_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(writer_reader.getStats());
+    writer->set_stats(stats_data);
   }
 
   if (query.layout() == Layout::GLOBAL_ORDER &&
@@ -2270,11 +2249,8 @@ Status query_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (query_reader.hasStats()) {
-    stats::Stats* stats = query->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      RETURN_NOT_OK(stats_from_capnp(query_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(query_reader.getStats());
+    query->set_stats(stats_data);
   }
 
   if (query_reader.hasWrittenFragmentInfo()) {
@@ -3212,11 +3188,8 @@ void ordered_dim_label_reader_from_capnp(
 
   // If cap'n proto object has stats set it on c++ object
   if (reader_reader.hasStats()) {
-    stats::Stats* stats = reader->stats();
-    // We should always have a stats here
-    if (stats != nullptr) {
-      throw_if_not_ok(stats_from_capnp(reader_reader.getStats(), stats));
-    }
+    auto stats_data = stats_from_capnp(reader_reader.getStats());
+    reader->set_stats(stats_data);
   }
 }
 
diff --git a/tiledb/sm/stats/global_stats.h b/tiledb/sm/stats/global_stats.h
index 7d5635ef483..a276366fce0 100644
--- a/tiledb/sm/stats/global_stats.h
+++ b/tiledb/sm/stats/global_stats.h
@@ -49,6 +49,71 @@
 #include "tiledb/common/heap_memory.h"
 #include "tiledb/sm/stats/stats.h"
 
+/**
+ * Documenting the current stats behavior and architecture as close as
+ * possible to the code so it's helpful next time some tries to refactor.
+ *
+ * Statistics collection is done at the top level via the GlobalStats class
+ * defined in this file.
+ * We maintain a global object called `all_stats` which is used to register
+ * Stats objects, enable/disable collection, reset or dumping the collected
+ * stats.
+ *
+ * The TileDB C API uses the `all_stats` object directly to execute the
+ * actions iterated above.
+ *
+ * The GlobalStats class owns a list called `registered_stats` that has one
+ * Stats object registered for each Context used. In consequence,
+ * ContextResources register a Stats object for each Context created, this
+ * object serves as the root for the tree of all children Stats used in a
+ * Context.
+ *
+ * As mentioned above, the Stats objects used under a Context form a tree.
+ * Each Stats object mentains a list of children Stats and a pointer to the
+ * parent Stats object.
+ * The Stats object created by ContextResources(named "Context.StorageManager")
+ * is the only Stats constructed in a standalone fashion using the Stats
+ * constructor, all the other objects under this root Stats are created via
+ * the Stats::create_child API.
+ *
+ * The (current, please update if not accurate anymore) exhaustive list of
+ * Stats we maintain under a Context is:
+ * ---------------------------
+ * ContextResources
+ *    - Query
+ *    - Reader
+ *    - Writer
+ *        - DenseTiler
+ *        - Subarray
+ *    - Deletes
+ *    - Subarray
+ *    - subSubarray
+ *        - SubarrayPartitioner
+ *    - VFS
+ *        - S3
+ *        - ArrayDirectory
+ *    - RestClient
+ *    - Consolidator
+ * ---------------------------
+ * Please visualize this as a tree, it was much easier to write
+ * like this, the tree is too wide.
+ *
+ *
+ * Observed issues:
+ * - Stats objects currently are created via Stats::create_child API from a
+ *   parent stats object. Child objects such as e.g. Subarray only hold a
+ *   pointer to the Stats object, this means that the Stats objects outlive
+ *   the objects they represent and are kept alive by the tree like structure
+ *   defined by the Stats class.
+ *   In theory, a Context running for a long time would OOM the machine with
+ *   Stats objects.
+ *
+ * - Stats::populate_flattened_stats aggregate the collected statistic via
+ *   summing. But we also collect ".min", ".max" statistics as well,
+ *   sum-aggregating those is incorrect. Currently the dump function just
+ *   doesn't print those statistics.
+ */
+
 namespace tiledb {
 namespace sm {
 namespace stats {
@@ -158,8 +223,7 @@ class GlobalStats {
 /* ********************************* */
 
 /**
- * The singleton instance holding all global stats counters. The report will
- * be automatically made when this object is destroyed (at program termination).
+ * The singleton instance holding all global stats counters and timers.
  */
 extern GlobalStats all_stats;
 
diff --git a/tiledb/sm/stats/stats.cc b/tiledb/sm/stats/stats.cc
index 5773c312b58..76c6294b9f6 100644
--- a/tiledb/sm/stats/stats.cc
+++ b/tiledb/sm/stats/stats.cc
@@ -48,9 +48,14 @@ namespace stats {
 /* ****************************** */
 
 Stats::Stats(const std::string& prefix)
+    : Stats(prefix, StatsData{}) {
+}
+
+Stats::Stats(const std::string& prefix, const StatsData& data)
     : enabled_(true)
     , prefix_(prefix + ".")
     , parent_(nullptr) {
+  this->populate_with_data(data);
 }
 
 /* ****************************** */
@@ -246,8 +251,12 @@ Stats* Stats::parent() {
 }
 
 Stats* Stats::create_child(const std::string& prefix) {
+  return create_child(prefix, StatsData{});
+}
+
+Stats* Stats::create_child(const std::string& prefix, const StatsData& data) {
   std::unique_lock<std::mutex> lck(mtx_);
-  children_.emplace_back(prefix_ + prefix);
+  children_.emplace_back(prefix_ + prefix, data);
   Stats* const child = &children_.back();
   child->parent_ = this;
   return child;
@@ -272,15 +281,26 @@ void Stats::populate_flattened_stats(
   }
 }
 
-std::unordered_map<std::string, double>* Stats::timers() {
+const std::unordered_map<std::string, double>* Stats::timers() const {
   return &timers_;
 }
 
 /** Return pointer to conters map, used for serialization only. */
-std::unordered_map<std::string, uint64_t>* Stats::counters() {
+const std::unordered_map<std::string, uint64_t>* Stats::counters() const {
   return &counters_;
 }
 
+void Stats::populate_with_data(const StatsData& data) {
+  auto timers = data.timers();
+  for (const auto& timer : timers) {
+    timers_[timer.first] = timer.second;
+  }
+  auto counters = data.counters();
+  for (const auto& counter : counters) {
+    counters_[counter.first] = counter.second;
+  }
+}
+
 }  // namespace stats
 }  // namespace sm
 }  // namespace tiledb
diff --git a/tiledb/sm/stats/stats.h b/tiledb/sm/stats/stats.h
index 018ed721f63..e18842be2df 100644
--- a/tiledb/sm/stats/stats.h
+++ b/tiledb/sm/stats/stats.h
@@ -51,6 +51,28 @@ namespace tiledb {
 namespace sm {
 namespace stats {
 
+class StatsData {
+ public:
+  StatsData() = default;
+
+  StatsData(
+      std::unordered_map<std::string, uint64_t> counters,
+      std::unordered_map<std::string, double> timers)
+      : counters_(counters)
+      , timers_(timers) {
+  }
+  const std::unordered_map<std::string, uint64_t>& counters() const {
+    return counters_;
+  }
+  const std::unordered_map<std::string, double>& timers() const {
+    return timers_;
+  }
+
+ private:
+  std::unordered_map<std::string, uint64_t> counters_;
+  std::unordered_map<std::string, double> timers_;
+};
+
 /**
  * Class that defines stats counters and methods to manipulate them.
  */
@@ -72,6 +94,14 @@ class Stats {
    */
   Stats(const std::string& prefix);
 
+  /**
+   * Value constructor.
+   *
+   * @param prefix The stat name prefix.
+   * @param data Initial data to populate the Stats object with
+   */
+  Stats(const std::string& prefix, const StatsData& data);
+
   /** Destructor. */
   ~Stats() = default;
 
@@ -116,11 +146,21 @@ class Stats {
   /** Creates a child instance, managed by this instance. */
   Stats* create_child(const std::string& prefix);
 
+  /** Creates a child instance, managed by this instance, the instance is
+   * constructed with initial data. */
+  Stats* create_child(const std::string& prefix, const StatsData& data);
+
   /** Return pointer to timers map, used for serialization only. */
-  std::unordered_map<std::string, double>* timers();
+  const std::unordered_map<std::string, double>* timers() const;
 
   /** Return pointer to conters map, used for serialization only. */
-  std::unordered_map<std::string, uint64_t>* counters();
+  const std::unordered_map<std::string, uint64_t>* counters() const;
+
+  /** Populate the counters and timers internal maps from a StatsData object
+   * Please be aware that the data is not being added up, it will override the
+   * existing data on the Stats object.
+   */
+  void populate_with_data(const StatsData& data);
 
  private:
   /* ****************************** */
diff --git a/tiledb/sm/subarray/subarray.cc b/tiledb/sm/subarray/subarray.cc
index 486c88b9c94..b9c8ac3dd66 100644
--- a/tiledb/sm/subarray/subarray.cc
+++ b/tiledb/sm/subarray/subarray.cc
@@ -3084,6 +3084,10 @@ stats::Stats* Subarray::stats() const {
   return stats_;
 }
 
+void Subarray::set_stats(const stats::StatsData& data) {
+  stats_->populate_with_data(data);
+}
+
 tuple<Status, optional<bool>> Subarray::non_overlapping_ranges_for_dim(
     const uint64_t dim_idx) {
   const auto& ranges = range_subset_[dim_idx].ranges();
diff --git a/tiledb/sm/subarray/subarray.h b/tiledb/sm/subarray/subarray.h
index a16e92b376e..f4f7f9d59ee 100644
--- a/tiledb/sm/subarray/subarray.h
+++ b/tiledb/sm/subarray/subarray.h
@@ -1278,6 +1278,11 @@ class Subarray {
   /** Returns `stats_`. */
   stats::Stats* stats() const;
 
+  /** Populate the owned stats instance with data.
+   * To be removed when the class will get a C41 constructor.
+   */
+  void set_stats(const stats::StatsData& data);
+
   /** Stores a vector of 1D ranges per dimension. */
   std::vector<std::vector<uint64_t>> original_range_idx_;
 
diff --git a/tiledb/sm/subarray/subarray_partitioner.cc b/tiledb/sm/subarray/subarray_partitioner.cc
index db6dab81cf4..440795f89ba 100644
--- a/tiledb/sm/subarray/subarray_partitioner.cc
+++ b/tiledb/sm/subarray/subarray_partitioner.cc
@@ -670,6 +670,10 @@ stats::Stats* SubarrayPartitioner::stats() const {
   return stats_;
 }
 
+void SubarrayPartitioner::set_stats(const stats::StatsData& data) {
+  stats_->populate_with_data(data);
+}
+
 /* ****************************** */
 /*          PRIVATE METHODS       */
 /* ****************************** */
diff --git a/tiledb/sm/subarray/subarray_partitioner.h b/tiledb/sm/subarray/subarray_partitioner.h
index 919b9a1b626..3d5ad9dafb1 100644
--- a/tiledb/sm/subarray/subarray_partitioner.h
+++ b/tiledb/sm/subarray/subarray_partitioner.h
@@ -335,6 +335,11 @@ class SubarrayPartitioner {
   /** Returns `stats_`. */
   stats::Stats* stats() const;
 
+  /** Populate the owned stats instance with data.
+   * To be removed when the class will get a C41 constructor.
+   */
+  void set_stats(const stats::StatsData& data);
+
  private:
   /* ********************************* */
   /*         PRIVATE ATTRIBUTES        */

From c1e5b58550a1af425d90e7dc05869bb9c035133e Mon Sep 17 00:00:00 2001
From: Robert Bindar <robertbindar@gmail.com>
Date: Mon, 12 Feb 2024 14:16:07 +0200
Subject: [PATCH 2/3] address review comments

---
 tiledb/sm/query/query.cc                   |  2 +
 tiledb/sm/query/query.h                    |  3 +-
 tiledb/sm/query/strategy_base.h            |  3 +-
 tiledb/sm/serialization/query.cc           | 74 ++++++++--------------
 tiledb/sm/stats/global_stats.h             |  2 +-
 tiledb/sm/stats/stats.cc                   |  4 +-
 tiledb/sm/stats/stats.h                    | 45 +++++++++++--
 tiledb/sm/subarray/subarray.cc             |  4 +-
 tiledb/sm/subarray/subarray.h              |  5 +-
 tiledb/sm/subarray/subarray_partitioner.cc |  4 +-
 tiledb/sm/subarray/subarray_partitioner.h  |  2 +-
 11 files changed, 84 insertions(+), 64 deletions(-)

diff --git a/tiledb/sm/query/query.cc b/tiledb/sm/query/query.cc
index 849eac4b294..713dd5ec691 100644
--- a/tiledb/sm/query/query.cc
+++ b/tiledb/sm/query/query.cc
@@ -827,6 +827,7 @@ Status Query::process() {
 
     if (!only_dim_label_query()) {
       if (strategy_ != nullptr) {
+        // The strategy destructor should reset its own Stats object here
         dynamic_cast<StrategyBase*>(strategy_.get())->stats()->reset();
         strategy_ = nullptr;
       }
@@ -917,6 +918,7 @@ Status Query::reset_strategy_with_layout(
     Layout layout, bool force_legacy_reader) {
   force_legacy_reader_ = force_legacy_reader;
   if (strategy_ != nullptr) {
+    // The strategy destructor should reset its own Stats object here
     dynamic_cast<StrategyBase*>(strategy_.get())->stats()->reset();
     strategy_ = nullptr;
   }
diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h
index 125268f140b..df8f21e15a0 100644
--- a/tiledb/sm/query/query.h
+++ b/tiledb/sm/query/query.h
@@ -655,7 +655,8 @@ class Query {
   /** Returns the internal stats object. */
   stats::Stats* stats() const;
 
-  /** Populate the owned stats instance with data.
+  /**
+   * Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
    */
   void set_stats(const stats::StatsData& data);
diff --git a/tiledb/sm/query/strategy_base.h b/tiledb/sm/query/strategy_base.h
index 9b23c133125..9f8a325b861 100644
--- a/tiledb/sm/query/strategy_base.h
+++ b/tiledb/sm/query/strategy_base.h
@@ -208,7 +208,8 @@ class StrategyBase {
   /** Returns `stats_`. */
   stats::Stats* stats() const;
 
-  /** Populate the owned stats instance with data.
+  /**
+   * Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
    */
   void set_stats(const stats::StatsData& data);
diff --git a/tiledb/sm/serialization/query.cc b/tiledb/sm/serialization/query.cc
index 45c20ff6e2b..92414cd51d5 100644
--- a/tiledb/sm/serialization/query.cc
+++ b/tiledb/sm/serialization/query.cc
@@ -88,7 +88,7 @@ namespace serialization {
 
 shared_ptr<Logger> dummy_logger = make_shared<Logger>(HERE(), "");
 
-Status stats_to_capnp(Stats& stats, capnp::Stats::Builder* stats_builder) {
+void stats_to_capnp(const Stats& stats, capnp::Stats::Builder* stats_builder) {
   // Build counters
   const auto counters = stats.counters();
   if (counters != nullptr && !counters->empty()) {
@@ -114,8 +114,6 @@ Status stats_to_capnp(Stats& stats, capnp::Stats::Builder* stats_builder) {
       ++index;
     }
   }
-
-  return Status::Ok();
 }
 
 StatsData stats_from_capnp(const capnp::Stats::Reader& stats_reader) {
@@ -246,11 +244,9 @@ Status subarray_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = subarray->stats();
-  if (stats != nullptr) {
-    auto stats_builder = builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = subarray->stats();
+  auto stats_builder = builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   if (subarray->relevant_fragments().relevant_fragments_size() > 0) {
     auto relevant_fragments_builder = builder->initRelevantFragments(
@@ -425,11 +421,9 @@ Status subarray_partitioner_to_capnp(
   builder->setMemoryBudgetValidity(mem_budget_validity);
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = partitioner.stats();
-  if (stats != nullptr) {
-    auto stats_builder = builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = partitioner.stats();
+  auto stats_builder = builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -907,11 +901,9 @@ Status reader_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -941,11 +933,9 @@ Status index_reader_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -976,11 +966,9 @@ Status dense_reader_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -1255,11 +1243,9 @@ Status delete_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = delete_strategy.stats();
-  if (stats != nullptr) {
-    auto stats_builder = delete_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *delete_strategy.stats();
+  auto stats_builder = delete_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   return Status::Ok();
 }
@@ -1283,11 +1269,9 @@ Status writer_to_capnp(
   }
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = writer.stats();
-  if (stats != nullptr) {
-    auto stats_builder = writer_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *writer.stats();
+  auto stats_builder = writer_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 
   if (query.layout() == Layout::GLOBAL_ORDER) {
     auto& global_writer = dynamic_cast<GlobalOrderWriter&>(writer);
@@ -1536,10 +1520,10 @@ Status query_to_capnp(
   RETURN_NOT_OK(config_to_capnp(query.config(), &config_builder));
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = query.stats();
-  if (stats != nullptr) {
+  auto stats = query.stats();
+  if (stats) {
     auto stats_builder = query_builder->initStats();
-    RETURN_NOT_OK(stats_to_capnp(*stats, &stats_builder));
+    stats_to_capnp(*stats, &stats_builder);
   }
 
   auto& written_fragment_info = query.get_written_fragment_info();
@@ -3156,11 +3140,9 @@ void ordered_dim_label_reader_to_capnp(
       query.dimension_label_increasing_order());
 
   // If stats object exists set its cap'n proto object
-  stats::Stats* stats = reader.stats();
-  if (stats != nullptr) {
-    auto stats_builder = reader_builder->initStats();
-    throw_if_not_ok(stats_to_capnp(*stats, &stats_builder));
-  }
+  const auto& stats = *reader.stats();
+  auto stats_builder = reader_builder->initStats();
+  stats_to_capnp(stats, &stats_builder);
 }
 
 void ordered_dim_label_reader_from_capnp(
diff --git a/tiledb/sm/stats/global_stats.h b/tiledb/sm/stats/global_stats.h
index a276366fce0..66d9e3dfab5 100644
--- a/tiledb/sm/stats/global_stats.h
+++ b/tiledb/sm/stats/global_stats.h
@@ -51,7 +51,7 @@
 
 /**
  * Documenting the current stats behavior and architecture as close as
- * possible to the code so it's helpful next time some tries to refactor.
+ * possible to the code so it's helpful next time someone tries to refactor.
  *
  * Statistics collection is done at the top level via the GlobalStats class
  * defined in this file.
diff --git a/tiledb/sm/stats/stats.cc b/tiledb/sm/stats/stats.cc
index 76c6294b9f6..82de3ed2549 100644
--- a/tiledb/sm/stats/stats.cc
+++ b/tiledb/sm/stats/stats.cc
@@ -291,11 +291,11 @@ const std::unordered_map<std::string, uint64_t>* Stats::counters() const {
 }
 
 void Stats::populate_with_data(const StatsData& data) {
-  auto timers = data.timers();
+  auto& timers = data.timers();
   for (const auto& timer : timers) {
     timers_[timer.first] = timer.second;
   }
-  auto counters = data.counters();
+  auto& counters = data.counters();
   for (const auto& counter : counters) {
     counters_[counter.first] = counter.second;
   }
diff --git a/tiledb/sm/stats/stats.h b/tiledb/sm/stats/stats.h
index e18842be2df..84d70a61b72 100644
--- a/tiledb/sm/stats/stats.h
+++ b/tiledb/sm/stats/stats.h
@@ -51,25 +51,55 @@ namespace tiledb {
 namespace sm {
 namespace stats {
 
+/**
+ * Class that holds measurement data that Stats objects can be
+ * initialized with.
+ */
 class StatsData {
  public:
+  /* ****************************** */
+  /*   CONSTRUCTORS & DESTRUCTORS   */
+  /* ****************************** */
+
+  /* Default constructor */
   StatsData() = default;
 
+  /**
+   * Value constructor.
+   *
+   * @param counters A map of counters
+   * @param timers A map of timers
+   */
   StatsData(
-      std::unordered_map<std::string, uint64_t> counters,
-      std::unordered_map<std::string, double> timers)
+      std::unordered_map<std::string, uint64_t>& counters,
+      std::unordered_map<std::string, double>& timers)
       : counters_(counters)
       , timers_(timers) {
   }
+
+  /* ****************************** */
+  /*              API               */
+  /* ****************************** */
+
+  /** Get a reference to internal counters */
   const std::unordered_map<std::string, uint64_t>& counters() const {
     return counters_;
   }
+
+  /** Get a reference to internal timers */
   const std::unordered_map<std::string, double>& timers() const {
     return timers_;
   }
 
  private:
+  /* ****************************** */
+  /*       PRIVATE ATTRIBUTES       */
+  /* ****************************** */
+
+  /** Map of counters and values */
   std::unordered_map<std::string, uint64_t> counters_;
+
+  /** Map of timers and values */
   std::unordered_map<std::string, double> timers_;
 };
 
@@ -98,7 +128,7 @@ class Stats {
    * Value constructor.
    *
    * @param prefix The stat name prefix.
-   * @param data Initial data to populate the Stats object with
+   * @param data Initial data to populate the Stats object with.
    */
   Stats(const std::string& prefix, const StatsData& data);
 
@@ -146,8 +176,10 @@ class Stats {
   /** Creates a child instance, managed by this instance. */
   Stats* create_child(const std::string& prefix);
 
-  /** Creates a child instance, managed by this instance, the instance is
-   * constructed with initial data. */
+  /**
+   * Creates a child instance, managed by this instance, the instance is
+   * constructed with initial data.
+   */
   Stats* create_child(const std::string& prefix, const StatsData& data);
 
   /** Return pointer to timers map, used for serialization only. */
@@ -156,7 +188,8 @@ class Stats {
   /** Return pointer to conters map, used for serialization only. */
   const std::unordered_map<std::string, uint64_t>* counters() const;
 
-  /** Populate the counters and timers internal maps from a StatsData object
+  /**
+   * Populate the counters and timers internal maps from a StatsData object
    * Please be aware that the data is not being added up, it will override the
    * existing data on the Stats object.
    */
diff --git a/tiledb/sm/subarray/subarray.cc b/tiledb/sm/subarray/subarray.cc
index b9c8ac3dd66..d58584717d3 100644
--- a/tiledb/sm/subarray/subarray.cc
+++ b/tiledb/sm/subarray/subarray.cc
@@ -3080,8 +3080,8 @@ RelevantFragments& Subarray::relevant_fragments() {
   return relevant_fragments_;
 }
 
-stats::Stats* Subarray::stats() const {
-  return stats_;
+const stats::Stats& Subarray::stats() const {
+  return *stats_;
 }
 
 void Subarray::set_stats(const stats::StatsData& data) {
diff --git a/tiledb/sm/subarray/subarray.h b/tiledb/sm/subarray/subarray.h
index f4f7f9d59ee..ea9977540fc 100644
--- a/tiledb/sm/subarray/subarray.h
+++ b/tiledb/sm/subarray/subarray.h
@@ -1276,9 +1276,10 @@ class Subarray {
       std::vector<uint64_t>* end_coords) const;
 
   /** Returns `stats_`. */
-  stats::Stats* stats() const;
+  const stats::Stats& stats() const;
 
-  /** Populate the owned stats instance with data.
+  /**
+   * Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
    */
   void set_stats(const stats::StatsData& data);
diff --git a/tiledb/sm/subarray/subarray_partitioner.cc b/tiledb/sm/subarray/subarray_partitioner.cc
index 440795f89ba..e22fbf28ac5 100644
--- a/tiledb/sm/subarray/subarray_partitioner.cc
+++ b/tiledb/sm/subarray/subarray_partitioner.cc
@@ -666,8 +666,8 @@ Subarray& SubarrayPartitioner::subarray() {
   return subarray_;
 }
 
-stats::Stats* SubarrayPartitioner::stats() const {
-  return stats_;
+const stats::Stats& SubarrayPartitioner::stats() const {
+  return *stats_;
 }
 
 void SubarrayPartitioner::set_stats(const stats::StatsData& data) {
diff --git a/tiledb/sm/subarray/subarray_partitioner.h b/tiledb/sm/subarray/subarray_partitioner.h
index 3d5ad9dafb1..0cb9e6439cc 100644
--- a/tiledb/sm/subarray/subarray_partitioner.h
+++ b/tiledb/sm/subarray/subarray_partitioner.h
@@ -333,7 +333,7 @@ class SubarrayPartitioner {
   Subarray& subarray();
 
   /** Returns `stats_`. */
-  stats::Stats* stats() const;
+  const stats::Stats& stats() const;
 
   /** Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.

From 075ca36b0ea6270654045033f7cc2bb8c39cf0b7 Mon Sep 17 00:00:00 2001
From: KiterLuc <67824247+KiterLuc@users.noreply.github.com>
Date: Mon, 12 Feb 2024 16:55:13 +0100
Subject: [PATCH 3/3] Small doxygen fixes.

---
 tiledb/sm/query/query.h                   | 2 ++
 tiledb/sm/query/strategy_base.h           | 2 ++
 tiledb/sm/stats/stats.h                   | 5 +++++
 tiledb/sm/subarray/subarray.h             | 2 ++
 tiledb/sm/subarray/subarray_partitioner.h | 2 ++
 5 files changed, 13 insertions(+)

diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h
index df8f21e15a0..e54c0342f90 100644
--- a/tiledb/sm/query/query.h
+++ b/tiledb/sm/query/query.h
@@ -658,6 +658,8 @@ class Query {
   /**
    * Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
+   *
+   * @param data Data to populate the stats with.
    */
   void set_stats(const stats::StatsData& data);
 
diff --git a/tiledb/sm/query/strategy_base.h b/tiledb/sm/query/strategy_base.h
index 9f8a325b861..b5d1abb983a 100644
--- a/tiledb/sm/query/strategy_base.h
+++ b/tiledb/sm/query/strategy_base.h
@@ -211,6 +211,8 @@ class StrategyBase {
   /**
    * Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
+   *
+   * @param data Data to populate the stats with.
    */
   void set_stats(const stats::StatsData& data);
 
diff --git a/tiledb/sm/stats/stats.h b/tiledb/sm/stats/stats.h
index 84d70a61b72..47691c8d294 100644
--- a/tiledb/sm/stats/stats.h
+++ b/tiledb/sm/stats/stats.h
@@ -179,6 +179,9 @@ class Stats {
   /**
    * Creates a child instance, managed by this instance, the instance is
    * constructed with initial data.
+   *
+   * @param prefix The stat name prefix.
+   * @param data Initial data to populate the Stats object with.
    */
   Stats* create_child(const std::string& prefix, const StatsData& data);
 
@@ -192,6 +195,8 @@ class Stats {
    * Populate the counters and timers internal maps from a StatsData object
    * Please be aware that the data is not being added up, it will override the
    * existing data on the Stats object.
+   *
+   * @param data Data to populate the stats with.
    */
   void populate_with_data(const StatsData& data);
 
diff --git a/tiledb/sm/subarray/subarray.h b/tiledb/sm/subarray/subarray.h
index ea9977540fc..84c986f61e3 100644
--- a/tiledb/sm/subarray/subarray.h
+++ b/tiledb/sm/subarray/subarray.h
@@ -1281,6 +1281,8 @@ class Subarray {
   /**
    * Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
+   *
+   * @param data Data to populate the stats with.
    */
   void set_stats(const stats::StatsData& data);
 
diff --git a/tiledb/sm/subarray/subarray_partitioner.h b/tiledb/sm/subarray/subarray_partitioner.h
index 0cb9e6439cc..2915fd7aefe 100644
--- a/tiledb/sm/subarray/subarray_partitioner.h
+++ b/tiledb/sm/subarray/subarray_partitioner.h
@@ -337,6 +337,8 @@ class SubarrayPartitioner {
 
   /** Populate the owned stats instance with data.
    * To be removed when the class will get a C41 constructor.
+   *
+   * @param data Data to populate the stats with.
    */
   void set_stats(const stats::StatsData& data);