diff --git a/changelogs/current/minor_behavior_changes/stats__lock-free-non-final-ref-count-releases.rst b/changelogs/current/minor_behavior_changes/stats__lock-free-non-final-ref-count-releases.rst new file mode 100644 index 0000000000000..2a39a9a94874d --- /dev/null +++ b/changelogs/current/minor_behavior_changes/stats__lock-free-non-final-ref-count-releases.rst @@ -0,0 +1,7 @@ +Reduced the cost of releasing references to stats: non-final reference-count decrements no +longer take the allocator's lock (for counters, gauges and text readouts) or the store's +histogram lock (for histograms), and stat reference-count operations use standard +reference-counting memory orderings instead of sequentially-consistent ones. Processes with +very large numbers of stats should see substantially lower and more consistent flush times +(roughly 30-40% in the stats flush benchmark at millions of stats). There are no visible +behavioral changes. diff --git a/envoy/stats/refcount_ptr.h b/envoy/stats/refcount_ptr.h index 664780b648a1f..4ecfda8daa655 100644 --- a/envoy/stats/refcount_ptr.h +++ b/envoy/stats/refcount_ptr.h @@ -151,25 +151,68 @@ class RefcountInterface { // Delegation helper for RefcountPtr. This can be instantiated in a class, but // explicit delegation will be needed for each of the three methods. struct RefcountHelper { - // Implements the RefcountInterface API. - void incRefCount() { - // Note: The ++ref_count_ here and --ref_count_ below have implicit memory - // orderings of sequentially consistent. Relaxed on addition and - // acquire/release on subtraction is the typical use for reference - // counting. On x86, the difference in instruction count is minimal, but the - // savings are greater on other platforms. - // - // https://www.boost.org/doc/libs/1_69_0/doc/html/atomic/usage_examples.html#boost_atomic.usage_examples.example_reference_counters - ++ref_count_; - } + // Implements the RefcountInterface API, using the memory orderings from the + // canonical reference-counting example: + // https://www.boost.org/doc/libs/1_69_0/doc/html/atomic/usage_examples.html#boost_atomic.usage_examples.example_reference_counters + // + // Incrementing needs atomicity but no ordering: no action is taken based on + // the result, and a new reference can only be created by a thread that + // already holds a valid one. Decrementing must ensure that all of this + // thread's accesses to the object happen-before the object's destruction, + // so each decrement releases, and the final decrement (the one that takes + // the count to zero and triggers destruction) also acquires. We use an + // acq_rel read-modify-write rather than the fence-based formulation from + // the Boost example because TSAN models RMW orderings precisely, while + // standalone fences are not as well supported. On x86 the difference from + // the previous sequentially-consistent operations is minimal, but the + // savings are real on weakly-ordered platforms such as ARM. + void incRefCount() { ref_count_.fetch_add(1, std::memory_order_relaxed); } bool decRefCount() { ASSERT(ref_count_ >= 1); - return --ref_count_ == 0; + return ref_count_.fetch_sub(1, std::memory_order_acq_rel) == 1; } - uint32_t use_count() const { return ref_count_; } + uint32_t use_count() const { return ref_count_.load(std::memory_order_relaxed); } std::atomic ref_count_{0}; }; +// Lock-free release fast path shared by stat implementations whose final +// reference-count decrement must happen under a lock so that the transition +// to zero is atomic with removal from a by-name map (see +// StatsSharedImpl::decRefCount in allocator.cc and +// ParentHistogramImpl::decRefCount in thread_local_store.cc, and the +// discussion introduced in #45821). +// +// Takes a snapshot of the current ref_count. If the snapshot is 1, a +// decrement could free memory, so we return false and the caller must +// decrement under its lock. If the snapshot is > 1, thread interleavings +// resolve as follows: +// +// First case: ref_count is unchanged (ref_count == snapshot). +// compare_exchange_weak succeeds having dropped a non-final reference, and +// we return true. +// +// Second case: ref_count changed and is still > 1. compare_exchange_weak +// fails and reloads the snapshot; the loop restarts. +// +// Third case: ref_count changed and is now <= 1. compare_exchange_weak fails +// and reloads the snapshot; the loop exits and we return false (a decrement +// could free memory; see above). +// +// @return true if a non-final reference was dropped (the caller is done), or +// false if the caller may hold the last reference and must perform the +// decrement under its lock. +inline bool tryDecRefCountFastPath(std::atomic& ref_count) { + ASSERT(ref_count >= 1); + uint32_t ref_count_snapshot = ref_count.load(std::memory_order_relaxed); + while (ref_count_snapshot > 1) { + if (ref_count.compare_exchange_weak(ref_count_snapshot, ref_count_snapshot - 1, + std::memory_order_acq_rel)) { + return true; + } + } + return false; +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/allocator.cc b/source/common/stats/allocator.cc index 4673fe3d5c9a0..186800a85ac74 100644 --- a/source/common/stats/allocator.cc +++ b/source/common/stats/allocator.cc @@ -90,37 +90,12 @@ template class StatsSharedImpl : public MetricImpl bool hidden() const override { return flags_ & Metric::Flags::Hidden; } // RefcountInterface - void incRefCount() override { ++ref_count_; } + void incRefCount() override { ref_count_.fetch_add(1, std::memory_order_relaxed); } bool decRefCount() override { - ASSERT(ref_count_ >= 1); - // Takes a snapshot of the current ref_count_ - uint32_t ref_count_snapshot = ref_count_.load(std::memory_order_relaxed); - - // Checks if ref_count_snapshot is 1. If it is 1, then a decrement would free memory, and we - // would need to acquire the allocator mutex. - // - // If ref_count_snapshot is > 1, then there are some important cases where thread - // interruptions might change what happens: - // - // First case: ref_count_ is unchanged (ref_count_ == ref_count_snapshot) - // Zero or more threads touch ref_count_ before compare_exchange_weak is called. - // compare_exchange_weak returns true, and decRefCount returns false. - // - // Second case: ref_count_ is changed (ref_count_ != ref_count_snapshot && ref_count_ > 1) - // One or more threads touch ref_count_ before compare_exchange_weak is called. - // compare_exchange_weak returns false, ref_count_snapshot is set to be ref_count_. Loop - // restarts. - // - // Third case: ref_count_ is changed (ref_count_ != ref_count_snapshot && ref_count <= 1) - // One or more threads touch ref_count_ before compare_exchange_weak is called. - // compare_exchange_weak returns false. ref_count_snapshot is set to be ref_count_. Loop - // restarts, but exits out of the while loop because conditional is false (a decrement would - // free memory; check start of comment). - while (ref_count_snapshot > 1) { - if (ref_count_.compare_exchange_weak(ref_count_snapshot, ref_count_snapshot - 1, - std::memory_order_acq_rel)) { - return false; - } + // See tryDecRefCountFastPath() in refcount_ptr.h for the interleaving + // analysis of the lock-free fast path. + if (tryDecRefCountFastPath(ref_count_)) { + return false; } // Another thread may call incRefCount at this point. The lock path still does the right thing // because the stat is not freed if ref_count_ is not 0. @@ -132,7 +107,7 @@ template class StatsSharedImpl : public MetricImpl } return false; } - uint32_t use_count() const override { return ref_count_; } + uint32_t use_count() const override { return ref_count_.load(std::memory_order_relaxed); } /** * We must atomically remove the counter/gauges from the allocator's sets when @@ -150,9 +125,10 @@ template class StatsSharedImpl : public MetricImpl // but these are always in transition to ref-count 2 or higher, and thus // cannot race with a decrement to zero. // - // However, we must hold alloc_.mutex_ when decrementing ref_count_ so that - // when it hits zero we can atomically remove it from alloc_.counters_ or - // alloc_.gauges_. We leave it atomic to avoid taking the lock on increment. + // Non-final decrements also avoid the lock, using a CAS loop that can never + // reach zero. However, we must hold alloc_.mutex_ for a decrement that may + // hit zero, so that we can atomically remove the stat from alloc_.counters_ + // or alloc_.gauges_; see decRefCount(). std::atomic ref_count_{0}; std::atomic flags_{0}; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 2eda1cf05ac41..4d2e1b1935dec 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -1094,10 +1094,9 @@ ParentHistogramImpl::~ParentHistogramImpl() { hist_free(cumulative_histogram_); } -void ParentHistogramImpl::incRefCount() { ++ref_count_; } +void ParentHistogramImpl::incRefCount() { ref_count_.fetch_add(1, std::memory_order_relaxed); } bool ParentHistogramImpl::decRefCount() { - bool ret; if (shutting_down_) { // When shutting down, we cannot reference thread_local_store_, as // histograms can outlive the store. So we decrement the ref-count without @@ -1105,19 +1104,24 @@ bool ParentHistogramImpl::decRefCount() { // histogram map in this scenario, as the set was cleared during shutdown, // and will not be repopulated in histogramFromStatNameWithTags after // initiating shutdown. - ret = --ref_count_ == 0; - } else { - // We delegate to the Store object to decrement the ref-count so it can hold - // the lock to the map. If we don't hold a lock, another thread may - // simultaneously try to allocate the same name'd histogram after we - // decrement it, and we'll wind up with a dtor/update race. To avoid this we - // must hold the lock until the histogram is removed from the map. - // - // See also StatsSharedImpl::decRefCount() in allocator.cc, which has - // the same issue. - ret = thread_local_store_.decHistogramRefCount(*this, ref_count_); + return ref_count_.fetch_sub(1, std::memory_order_acq_rel) == 1; } - return ret; + + // Fast path: when this is not the last reference, drop it without taking + // the store's histogram lock; see tryDecRefCountFastPath() in + // refcount_ptr.h for the interleaving analysis, with the store's histogram + // lock playing the role of the allocator's mutex. + if (tryDecRefCountFastPath(ref_count_)) { + return false; + } + + // Slow path: we may hold the last reference, so we delegate to the Store + // object to decrement the ref-count under the lock to the map. If we don't + // hold a lock, another thread may simultaneously try to allocate the same + // name'd histogram after we decrement it, and we'll wind up with a + // dtor/update race. To avoid this we must hold the lock until the histogram + // is removed from the map. + return thread_local_store_.decHistogramRefCount(*this, ref_count_); } bool ThreadLocalStoreImpl::decHistogramRefCount(ParentHistogramImpl& hist, diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 2b772c284618d..19c40ecfd93b9 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -128,7 +128,7 @@ class ParentHistogramImpl : public MetricImpl { // RefcountInterface void incRefCount() override; bool decRefCount() override; - uint32_t use_count() const override { return ref_count_; } + uint32_t use_count() const override { return ref_count_.load(std::memory_order_relaxed); } // Indicates that the ThreadLocalStore is shutting down, so no need to clear its histogram_set_. void setShuttingDown(bool shutting_down) { shutting_down_ = shutting_down; } diff --git a/source/docs/stats.md b/source/docs/stats.md index b39b5f252cf87..8b78535361fa4 100644 --- a/source/docs/stats.md +++ b/source/docs/stats.md @@ -83,8 +83,10 @@ Pictorially this looks like: `ParentHistogram`s are held weakly a set in ThreadLocalStore. Like other stats, they keep an embedded reference count and are removed from the set and destroyed -when the last strong reference disappears. Consequently, we must hold a lock for -the set when decrementing histogram reference counts. A similar process occurs for +when the last strong reference disappears. Consequently, a decrement that may +drop the last reference must hold a lock for the set, so that removal from the +set is atomic with the final decrement; non-final decrements take a lock-free +fast path. A similar process occurs for other types of stats, but in those cases it is taken care of in `AllocatorImpl`. There are strong references to `ParentHistograms` in TlsCacheEntry::parent_histograms_. diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index f72df037338f1..c45f10c33248a 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -178,7 +178,10 @@ envoy_cc_test( name = "refcount_ptr_test", srcs = ["refcount_ptr_test.cc"], rbe_pool = "6gig", - deps = ["//envoy/stats:refcount_ptr_interface"], + deps = [ + "//envoy/stats:refcount_ptr_interface", + "//test/test_common:thread_factory_for_test_lib", + ], ) envoy_cc_test( diff --git a/test/common/stats/allocator_test.cc b/test/common/stats/allocator_test.cc index 70b0ea2d2f702..010f3efae371f 100644 --- a/test/common/stats/allocator_test.cc +++ b/test/common/stats/allocator_test.cc @@ -137,6 +137,87 @@ TEST_F(AllocatorTest, RefCountDecAllocRaceSynchronized) { EXPECT_FALSE(alloc_.isMutexLockedForTest()); } +// Hammers non-final ref-count releases, which drop their reference without +// taking the allocator's lock: half the threads copy and drop a reference +// that the main thread holds throughout (so all of their releases are +// non-final), while the other half re-acquire the same name'd counter through +// the allocator, which takes the lock. Verifies the ref-count, the stat's +// value, and that the final release still destroys the stat. The case where +// the final release itself races the fast-path releases across threads is +// covered by RefCountCrossThreadFinalRelease below. +TEST_F(AllocatorTest, RefCountNonFinalReleaseChurn) { + StatName counter_name = makeStat("counter.name"); + Thread::ThreadFactory& thread_factory = Thread::threadFactoryForTest(); + + CounterSharedPtr counter = alloc_.makeCounter(counter_name, StatName(), {}); + + const uint32_t num_threads = 12; + const uint32_t iters = 10000; + std::vector threads; + absl::Notification go; + for (uint32_t i = 0; i < num_threads; ++i) { + threads.push_back(thread_factory.createThread([&, i]() { + go.WaitForNotification(); + for (uint32_t iter = 0; iter < iters; ++iter) { + if (i % 2 == 0) { + CounterSharedPtr copy(counter); + copy->inc(); + } else { + alloc_.makeCounter(counter_name, StatName(), {})->inc(); + } + } + })); + } + go.Notify(); + for (uint32_t i = 0; i < num_threads; ++i) { + threads[i]->join(); + } + + EXPECT_EQ(1, counter->use_count()); + EXPECT_EQ(num_threads * iters, counter->value()); + + // Dropping the last reference destroys the stat; allocating the same name + // again must yield a fresh counter. + counter.reset(); + CounterSharedPtr fresh = alloc_.makeCounter(counter_name, StatName(), {}); + EXPECT_EQ(1, fresh->use_count()); + EXPECT_EQ(0, fresh->value()); +} + +// The final release races the fast-path releases across worker threads: the +// main thread drops its reference before releasing the workers, so whichever +// worker drops last performs the locked final release and destroys the stat +// while the other workers are releasing through the lock-free fast path. +// Under TSAN this validates that every worker's accesses to the stat +// happen-before its destruction. +TEST_F(AllocatorTest, RefCountCrossThreadFinalRelease) { + StatName counter_name = makeStat("counter.name"); + Thread::ThreadFactory& thread_factory = Thread::threadFactoryForTest(); + + const uint32_t num_threads = 12; + const uint32_t iters = 200; + for (uint32_t iter = 0; iter < iters; ++iter) { + CounterSharedPtr counter = alloc_.makeCounter(counter_name, StatName(), {}); + absl::Notification go; + std::vector threads; + for (uint32_t i = 0; i < num_threads; ++i) { + threads.push_back(thread_factory.createThread([copy = counter, &go]() mutable { + go.WaitForNotification(); + copy->inc(); + copy.reset(); // One of these releases is the final one. + })); + } + counter.reset(); // The main thread no longer holds a reference. + go.Notify(); + for (uint32_t i = 0; i < num_threads; ++i) { + threads[i]->join(); + } + // The stat was destroyed by whichever worker released last; the same name + // must now yield a fresh counter. + EXPECT_EQ(0, alloc_.makeCounter(counter_name, StatName(), {})->value()); + } +} + TEST_F(AllocatorTest, HiddenGauge) { GaugeSharedPtr uninitialized_gauge = alloc_.makeGauge(makeStat("uninitialized"), StatName(), {}, Gauge::ImportMode::Uninitialized); diff --git a/test/common/stats/refcount_ptr_test.cc b/test/common/stats/refcount_ptr_test.cc index 448bd2fbc66d3..288a9cc65cf94 100644 --- a/test/common/stats/refcount_ptr_test.cc +++ b/test/common/stats/refcount_ptr_test.cc @@ -1,7 +1,11 @@ #include +#include #include "envoy/stats/refcount_ptr.h" +#include "test/test_common/thread_factory_for_test.h" + +#include "absl/synchronization/notification.h" #include "gtest/gtest.h" namespace Envoy { @@ -69,5 +73,34 @@ TEST(RefcountPtr, Operators) { EXPECT_EQ(1, shared3.use_count()); } +// Reads through references from many threads while the last of them to drop +// its reference deletes the object; every read must happen-before the delete. +// The copies are made sequentially on the main thread (capture-by-value at +// thread creation), so only reads, releases, and the deletion race with each +// other. Meaningful primarily under TSAN, which verifies the release/acquire +// pairing on RefcountHelper's reference count. +TEST(RefcountPtr, ThreadedCopyDropChurn) { + Thread::ThreadFactory& thread_factory = Thread::threadFactoryForTest(); + const uint32_t num_threads = 8; + const uint32_t iters = 500; + for (uint32_t iter = 0; iter < iters; ++iter) { + SharedString shared(new RefcountedString("Hello")); + absl::Notification go; + std::vector threads; + for (uint32_t i = 0; i < num_threads; ++i) { + threads.push_back(thread_factory.createThread([copy = SharedString(shared), &go]() mutable { + go.WaitForNotification(); + EXPECT_EQ(5, copy->size()); + copy.reset(); // Possibly the final release, which deletes the string. + })); + } + shared.reset(); // Drop the main thread's reference before releasing the readers. + go.Notify(); + for (uint32_t i = 0; i < num_threads; ++i) { + threads[i]->join(); + } + } +} + } // namespace Stats } // namespace Envoy diff --git a/test/server/server_stats_flush_benchmark_test.cc b/test/server/server_stats_flush_benchmark_test.cc index 8e789409378a0..b223dd7a3af77 100644 --- a/test/server/server_stats_flush_benchmark_test.cc +++ b/test/server/server_stats_flush_benchmark_test.cc @@ -119,7 +119,14 @@ static void bmFlushToSinksWithPredicatesSet(::benchmark::State& state) { speed_test.test(state); } -BENCHMARK(bmFlushToSinks)->Unit(::benchmark::kMillisecond)->RangeMultiplier(10)->Range(10, 1000000); +// The 2500000 case exercises 10M metric objects per snapshot (2.5M each of +// counters, gauges, histograms and text readouts), the regime where ref-count +// overhead during flush was reported in #43836. +BENCHMARK(bmFlushToSinks) + ->Unit(::benchmark::kMillisecond) + ->RangeMultiplier(10) + ->Range(10, 1000000) + ->Arg(2500000); BENCHMARK(bmFlushToSinksWithPredicatesSet) ->Unit(::benchmark::kMillisecond) ->RangeMultiplier(10)