From 114ea9b1eb7d7ba8671a7785baf865433d83775c Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 18 Oct 2020 13:51:59 +0300 Subject: [PATCH] Fix accounting for new/delete from different threads for VariableContext::Thread MemoryTracker assumes that for VariableContext::Thread new/delete may be called from different threads, hence the amount of memory can go negative. However the MemoryTracker is nested, so even if the negative amount is allowed for VariableContext::Thread it does not allowed for anything upper, and hence the MemoryTracking will not be decremented properly. Fix this, by passing initial size to the parent free. This should fix memory drift for HTTP queries. --- src/Common/MemoryTracker.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Common/MemoryTracker.cpp b/src/Common/MemoryTracker.cpp index 6d0b17270f9..380fcb1b2b6 100644 --- a/src/Common/MemoryTracker.cpp +++ b/src/Common/MemoryTracker.cpp @@ -210,14 +210,15 @@ void MemoryTracker::free(Int64 size) DB::TraceCollector::collect(DB::TraceType::MemorySample, StackTrace(), -size); } + Int64 accounted_size = size; if (level == VariableContext::Thread) { /// Could become negative if memory allocated in this thread is freed in another one - amount.fetch_sub(size, std::memory_order_relaxed); + amount.fetch_sub(accounted_size, std::memory_order_relaxed); } else { - Int64 new_amount = amount.fetch_sub(size, std::memory_order_relaxed) - size; + Int64 new_amount = amount.fetch_sub(accounted_size, std::memory_order_relaxed) - accounted_size; /** Sometimes, query could free some data, that was allocated outside of query context. * Example: cache eviction. @@ -228,7 +229,7 @@ void MemoryTracker::free(Int64 size) if (unlikely(new_amount < 0)) { amount.fetch_sub(new_amount); - size += new_amount; + accounted_size += new_amount; } } @@ -236,7 +237,7 @@ void MemoryTracker::free(Int64 size) loaded_next->free(size); if (metric != CurrentMetrics::end()) - CurrentMetrics::sub(metric, size); + CurrentMetrics::sub(metric, accounted_size); }