felipecrv commented on code in PR #40647:
URL: https://github.com/apache/arrow/pull/40647#discussion_r1530904516
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -36,43 +37,58 @@ namespace internal {
// Helper tracking memory statistics
class MemoryPoolStats {
- public:
- MemoryPoolStats() : bytes_allocated_(0), max_memory_(0) {}
-
- int64_t max_memory() const { return max_memory_.load(); }
-
- int64_t bytes_allocated() const { return bytes_allocated_.load(); }
-
- int64_t total_bytes_allocated() const { return
total_allocated_bytes_.load(); }
+ private:
+ // All atomics are updated according to Acquire-Release ordering.
+ //
https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering
+ //
+ // max_memory_, total_allocated_bytes_, and num_allocs_ only go up (they are
+ // monotonically increasing) which can allow some optimizations.
+ std::atomic<int64_t> bytes_allocated_{0};
Review Comment:
One of the things I want to try. Still thinking how to do it without growing
the size of the `MemoryPool` structs that embedd this struct in them.
##########
cpp/src/arrow/memory_pool.h:
##########
@@ -36,43 +37,58 @@ namespace internal {
// Helper tracking memory statistics
class MemoryPoolStats {
- public:
- MemoryPoolStats() : bytes_allocated_(0), max_memory_(0) {}
-
- int64_t max_memory() const { return max_memory_.load(); }
-
- int64_t bytes_allocated() const { return bytes_allocated_.load(); }
-
- int64_t total_bytes_allocated() const { return
total_allocated_bytes_.load(); }
+ private:
+ // All atomics are updated according to Acquire-Release ordering.
+ //
https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering
+ //
+ // max_memory_, total_allocated_bytes_, and num_allocs_ only go up (they are
+ // monotonically increasing) which can allow some optimizations.
+ std::atomic<int64_t> bytes_allocated_{0};
+ std::atomic<int64_t> max_memory_{0};
+ std::atomic<int64_t> total_allocated_bytes_{0};
+ std::atomic<int64_t> num_allocs_{0};
- int64_t num_allocations() const { return num_allocs_.load(); }
+ public:
+ int64_t max_memory() const { return
max_memory_.load(std::memory_order_acquire); }
Review Comment:
Cost is high on the store side. It's usually the case that multiple threads
are read-modify-update'ing the values and no thread is actually doing the
loads, so there is no need to compromise on the correctness of the loads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
