Changes from PR review

TileDB-Inc · Feb 8, 2024 · 3d380f0 · 3d380f0
1 parent 2b124b5
commit 3d380f0
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 30 deletions.
diff --git a/tiledb/common/memory_tracker.cc b/tiledb/common/memory_tracker.cc
@@ -114,28 +114,34 @@ bool MemoryTrackerResource::do_is_equal(
 }
 
 MemoryTracker::~MemoryTracker() {
-  // TODO: Make this spit out some loud log noise if we've not
-  //       properly deallocated all our things.
+  assert(
+      total_counter_.fetch_add(0) == 0 &&
+      "MemoryTracker destructed with outstanding allocations.");
 }
 
 tdb::pmr::memory_resource* MemoryTracker::get_resource(MemoryType type) {
   std::lock_guard<std::mutex> lg(mutex_);
-  auto iter = resources_.find(type);
 
+  // If we've already created an instance for this type, return it.
+  auto iter = resources_.find(type);
   if (iter != resources_.end()) {
     return iter->second.get();
   }
 
-  if (counters_.find(type) != counters_.end() && counters_[type] != 0) {
-    throw MemoryTrackerException("Invalid internal state");
+  // Add a new counter if it doesn't exist.
+  if (counters_.find(type) == counters_.end()) {
+    counters_.emplace(type, 0);
+  } else {
+    // There's no outstanding memory resource for this type, so it must be zero.
+    assert(counters_[type] == 0 && "Invalid memory tracking state.");
   }
 
-  counters_.emplace(type, 0);
-
+  // Create and track a shared_ptr to the new memory resource.
   auto ret = make_shared<MemoryTrackerResource>(
       HERE(), upstream_, total_counter_, counters_[type]);
   resources_.emplace(type, ret);
 
+  // Return the raw memory resource pointer for use by pmr containers.
   return ret.get();
 }
 

diff --git a/tiledb/common/pmr.h b/tiledb/common/pmr.h
@@ -34,16 +34,6 @@
 #ifndef TILEDB_COMMON_PMR_H
 #define TILEDB_COMMON_PMR_H
 
-// Currently, this is fairly broken when building with XCode 15 on macOS 13.x.
-// For now I'm just going to default to using Pablo Halpern's implementation
-// rather than spend time fighting compiler detection and build chains.
-//
-// #if __has_include(<memory_resource>)
-// #include <memory_resource>
-// #else
-// #error Missing memory_resources header.
-// #endif
-
 #include <vector>
 
 #include <boost/container/pmr/memory_resource.hpp>

diff --git a/tiledb/sm/fragment/fragment_metadata.cc b/tiledb/sm/fragment/fragment_metadata.cc
@@ -73,20 +73,14 @@ class FragmentMetadataStatusException : public StatusException {
   }
 };
 
-// Temporary until these changes are verified.
-inline shared_ptr<MemoryTracker> verify_tracker(shared_ptr<MemoryTracker> mt) {
-  assert(mt != nullptr);
-  return mt;
-}
-
 /* ****************************** */
 /*   CONSTRUCTORS & DESTRUCTORS   */
 /* ****************************** */
 
 FragmentMetadata::FragmentMetadata(
     ContextResources* resources, shared_ptr<MemoryTracker> memory_tracker)
     : resources_(resources)
-    , memory_tracker_(verify_tracker(memory_tracker))
+    , memory_tracker_(memory_tracker)
     , tile_offsets_(memory_tracker_->get_resource(MemoryType::TILE_OFFSETS))
     , tile_var_offsets_(memory_tracker_->get_resource(MemoryType::TILE_OFFSETS))
     , tile_var_sizes_(memory_tracker_->get_resource(MemoryType::TILE_OFFSETS))
@@ -113,7 +107,7 @@ FragmentMetadata::FragmentMetadata(
     bool has_timestamps,
     bool has_deletes_meta)
     : resources_(resources)
-    , memory_tracker_(verify_tracker(memory_tracker))
+    , memory_tracker_(memory_tracker)
     , array_schema_(array_schema)
     , dense_(dense)
     , footer_size_(0)

diff --git a/tiledb/sm/query/strategy_base.h b/tiledb/sm/query/strategy_base.h
@@ -94,6 +94,7 @@ class StrategyParams {
   /*                 API               */
   /* ********************************* */
 
+  /** Return the array memory tracker. */
   inline shared_ptr<MemoryTracker> array_memory_tracker() {
     return array_memory_tracker_;
   }
@@ -205,11 +206,6 @@ class StrategyBase {
   /*                 API               */
   /* ********************************* */
 
-  /** Returns `memory_tracker_`. */
-  inline shared_ptr<MemoryTracker> array_memory_tracker() const {
-    return array_memory_tracker_;
-  }
-
   /** Returns `stats_`. */
   inline stats::Stats* stats() const {
     return stats_;

diff --git a/tiledb/sm/query/writers/writer_base.h b/tiledb/sm/query/writers/writer_base.h
@@ -193,6 +193,7 @@ class WriterBase : public StrategyBase, public IQueryStrategy {
   /*         PROTECTED METHODS         */
   /* ********************************* */
 
+  /** Utility function for constructing new FragmentMetadata instances. */
   shared_ptr<FragmentMetadata> create_fragment_metadata();
 
   /** Adss a fragment to `written_fragment_info_`. */