diff --git a/velox/common/base/Counters.cpp b/velox/common/base/Counters.cpp index 00a503b8a5e2..1c6018cb71a2 100644 --- a/velox/common/base/Counters.cpp +++ b/velox/common/base/Counters.cpp @@ -146,7 +146,6 @@ void registerVeloxMetrics() { // The distribution of a root memory pool's initial capacity in range of [0, // 256MB] with 32 buckets. It is configured to report the capacity at P50, // P90, P99, and P100 percentiles. - DEFINE_HISTOGRAM_METRIC( kMetricMemoryPoolInitialCapacityBytes, 8L << 20, @@ -163,6 +162,12 @@ void registerVeloxMetrics() { DEFINE_HISTOGRAM_METRIC( kMetricMemoryPoolCapacityGrowCount, 8, 0, 256, 50, 90, 99, 100); + // Tracks the count of double frees in memory allocator, indicating the + // possibility of buffer ownership issues when a buffer is freed more than + // once. + DEFINE_METRIC( + kMetricMemoryAllocatorDoubleFreeCount, facebook::velox::StatType::COUNT); + /// ================== Spill related Counters ================= // The number of bytes in memory to spill. diff --git a/velox/common/base/Counters.h b/velox/common/base/Counters.h index c78ea5e2afe6..6c7a876af2d2 100644 --- a/velox/common/base/Counters.h +++ b/velox/common/base/Counters.h @@ -67,6 +67,9 @@ constexpr folly::StringPiece kMetricMemoryPoolUsageLeakBytes{ constexpr folly::StringPiece kMetricMemoryPoolReservationLeakBytes{ "velox.memory_pool_reservation_leak_bytes"}; +constexpr folly::StringPiece kMetricMemoryAllocatorDoubleFreeCount{ + "velox.memory_allocator_double_free_count"}; + constexpr folly::StringPiece kMetricArbitratorRequestsCount{ "velox.arbitrator_requests_count"}; diff --git a/velox/common/memory/MmapAllocator.cpp b/velox/common/memory/MmapAllocator.cpp index 2383b33698ea..65d5abe7aa5d 100644 --- a/velox/common/memory/MmapAllocator.cpp +++ b/velox/common/memory/MmapAllocator.cpp @@ -18,7 +18,9 @@ #include +#include "velox/common/base/Counters.h" #include "velox/common/base/Portability.h" +#include "velox/common/base/StatsReporter.h" #include "velox/common/memory/Memory.h" namespace facebook::velox::memory { @@ -877,10 +879,10 @@ MachinePageCount MmapAllocator::SizeClass::free(Allocation& allocation) { const int firstBit = (runAddress - address_) / (AllocationTraits::kPageSize * unitSize_); for (auto page = firstBit; page < firstBit + numPages; ++page) { - if (!bits::isBitSet(pageAllocated_.data(), page)) { - // TODO: change this to a velox failure to catch the bug. + if (FOLLY_UNLIKELY(!bits::isBitSet(pageAllocated_.data(), page))) { VELOX_MEM_LOG(ERROR) << "Double free: page = " << page << " sizeclass = " << unitSize_; + RECORD_METRIC_VALUE(kMetricMemoryAllocatorDoubleFreeCount); continue; } if (bits::isBitSet(pageMapped_.data(), page)) { diff --git a/velox/docs/monitoring/metrics.rst b/velox/docs/monitoring/metrics.rst index c3bf283f8863..8ca90e878c65 100644 --- a/velox/docs/monitoring/metrics.rst +++ b/velox/docs/monitoring/metrics.rst @@ -175,6 +175,11 @@ Memory Management * - memory_pool_capacity_leak_bytes - Sum - The root memory pool reservation leak in bytes. + * - memory_allocator_double_free_count + - Count + - Tracks the count of double frees in memory allocator, indicating the + possibility of buffer ownership issues when a buffer is freed more + than once. Spilling --------