From 9f51a59570cfbafc5e099b18d694a33c678f3f89 Mon Sep 17 00:00:00 2001 From: Chris Kennelly CA Date: Fri, 18 Sep 2026 09:39:54 -0700 Subject: [PATCH] Put CentralFreeList and miss counters on separate cachelines in TransferCache. TransferCache embeds its CentralFreeList at offset 0x30, so CentralFreeList::lock_ shares the first cacheline with the transfer cache's lock_, slot_info_, hit counters and slots_. Cores hitting in the transfer cache (lock CAS + slot_info_ RMW) and cores holding the CentralFreeList lock on a miss ping-pong that line. The four miss counters likewise share a line with the tail of CentralFreeList::nonempty_, which is mutated under the CentralFreeList lock. GWP: TransferCache::RemoveRange 597k / InsertRange 264k self. New layout: line 0: lock_, low_water_mark_, hit counters, slot_info_, slots_, owner_, max_capacity_ (60 B; owner_/max_capacity_ are immutable) line 1: insert/remove miss counters (48 B) line 2: CentralFreeList, cacheline aligned (its lock_ is at offset 0) A FreeList no larger than a pointer (BackingTransferCache in the sharded cache) has no lock and packs into line 1. CentralFreeList's tail padding absorbs the shift: sizeof(TransferCache) stays 768 B, the sharded TransferCache stays 128 B and TransferCacheManager stays 115,200 B (8K pages). static_asserts in transfer_cache_test pin the layout (offsets and alignment) rather than the raw sizes. Also record misses after, rather than before, the freelist call in InsertRange and RemoveRange. The counter RMW usually misses (the line is written by every missing core); issued ahead of the CentralFreeList lock acquisition, its store had to drain before the lock cmpxchg (a full barrier on x86) could complete, serializing two cross-core transfers on the miss path. Issued afterwards it drains asynchronously from the store buffer. Counts are identical. RemoveRange miss path (-c opt, x86-64) now ends in: lea 0x80(%rbx),%rdi ; call CentralFreeList::RemoveRange incq 0x48(%rbx) ; addq %r14,0x60(%rbx) ; ret PiperOrigin-RevId: 983910780 --- tcmalloc/BUILD | 1 + tcmalloc/CMakeLists.txt | 1 + tcmalloc/transfer_cache_internals.h | 52 +++++++++++++++++++++-------- tcmalloc/transfer_cache_test.cc | 45 +++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/tcmalloc/BUILD b/tcmalloc/BUILD index b7ed6a9d6..02b5f7c82 100644 --- a/tcmalloc/BUILD +++ b/tcmalloc/BUILD @@ -931,6 +931,7 @@ cc_test( "//tcmalloc/internal:percpu", "//tcmalloc/testing:testutil", "//tcmalloc/testing:thread_manager", + "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/strings:str_format", "@com_google_absl//absl/time", "@com_google_absl//absl/types:span", diff --git a/tcmalloc/CMakeLists.txt b/tcmalloc/CMakeLists.txt index 89d54b526..58cd0752c 100644 --- a/tcmalloc/CMakeLists.txt +++ b/tcmalloc/CMakeLists.txt @@ -1091,6 +1091,7 @@ tcmalloc_cc_test( "GTest::gtest_main" "GTest::gmock_main" "GTest::gmock" + "absl::core_headers" "absl::span" "absl::str_format" "absl::time" diff --git a/tcmalloc/transfer_cache_internals.h b/tcmalloc/transfer_cache_internals.h index e906b8553..13e684a93 100644 --- a/tcmalloc/transfer_cache_internals.h +++ b/tcmalloc/transfer_cache_internals.h @@ -98,9 +98,9 @@ class TransferCache { low_water_mark_(0), slot_info_(SizeInfo({0, capacity.capacity})), slots_(nullptr), - freelist_do_not_access_directly_(), owner_(owner), - max_capacity_(capacity.max_capacity) { + max_capacity_(capacity.max_capacity), + freelist_do_not_access_directly_() { freelist().Init(size_class, Parameters::cfl_subbucket_prioritization()); slots_ = max_capacity_ != 0 ? reinterpret_cast(owner_->Alloc( max_capacity_ * sizeof(void*))) @@ -173,10 +173,17 @@ class TransferCache { } } + freelist().InsertRange(batch); + + // Record the miss after the freelist call. The counters live on a + // cacheline that is written by every missing core, so the RMW is likely to + // miss; issued before the freelist's lock acquisition (a full barrier on + // x86), its store would have to drain before the lock CAS could execute. + // Issued afterwards, it drains from the store buffer asynchronously. The + // counters are lossy and nothing on this path reads them, so the count is + // unchanged. insert_misses_.LossyAdd(1); insert_object_misses_.Inc(batch.size()); - - freelist().InsertRange(batch); } // Returns the actual number of fetched elements and stores elements in the @@ -203,9 +210,11 @@ class TransferCache { } } + const int got = freelist().RemoveRange(batch); + // See InsertRange for why the miss is recorded after the freelist call. remove_misses_.LossyAdd(1); remove_object_misses_.Inc(batch.size()); - return freelist().RemoveRange(batch); + return got; } // We record the lowest value of info.used in a low water mark since the last @@ -353,6 +362,8 @@ class TransferCache { int32_t max_capacity() const { return max_capacity_; } private: + friend class TransferCacheTestPeer; + // Returns first object of the i-th slot. void** GetSlot(size_t i) ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { return slots_ + i; @@ -372,8 +383,7 @@ class TransferCache { absl::base_internal::SpinLock lock_; // All the following fields are accessed when holding lock_, so they should - // be collocated with lock_ on the same cacheline. Align insert_hits_ to - // ensure the following fields are on a separate cacheline. + // be collocated with lock_ on the same cacheline. // Lowest value of "slot_info_.used" since last call to TryPlunder. All // elements not used for a full cycle (2 seconds) are unlikely to get used @@ -396,23 +406,37 @@ class TransferCache { // entries. void** slots_ ABSL_GUARDED_BY(lock_); - FreeList freelist_do_not_access_directly_; - + // owner_ and max_capacity_ are immutable after construction and only read on + // slow paths, so they may share the cacheline with lock_. Manager* const owner_; // Maximum size of the cache. const int32_t max_capacity_; - // The following 4 *_misses_ counters - // are frequently updated, so they should reside in a separate cacheline from - // lock_. - + // The following 4 *_misses_ counters are updated on every miss by any core + // that falls through to the freelist, so they reside on their own cacheline, + // apart from both lock_ (hit path) and the freelist's lock (miss path). + // // For these we are deliberately fast-and-loose. Some increments may be lost. - StatsCounter insert_misses_; + alignas(ABSL_CACHELINE_SIZE) StatsCounter insert_misses_; StatsCounter remove_misses_; MissCounts insert_object_misses_; MissCounts remove_object_misses_; + + // CentralFreeList carries its own lock at offset 0 and is mutated by every + // core that misses, while other cores concurrently hit under lock_ above. + // Start it on a fresh cacheline so the two locks do not false share. Kept + // last so its tail padding merges with that of ABSL_CACHELINE_ALIGNED. + // + // A FreeList no larger than a pointer (BackingTransferCache, which holds + // only a size class) is an immutable forwarder with no lock, so it needs no + // isolation and packs into the miss counters' cacheline, keeping the sharded + // TransferCache at two cachelines. + static constexpr size_t kFreeListAlignment = sizeof(FreeList) <= sizeof(void*) + ? alignof(FreeList) + : ABSL_CACHELINE_SIZE; + alignas(kFreeListAlignment) FreeList freelist_do_not_access_directly_; } ABSL_CACHELINE_ALIGNED; template diff --git a/tcmalloc/transfer_cache_test.cc b/tcmalloc/transfer_cache_test.cc index 8a2359199..dad1dc054 100644 --- a/tcmalloc/transfer_cache_test.cc +++ b/tcmalloc/transfer_cache_test.cc @@ -18,12 +18,14 @@ #include #include +#include #include #include #include #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/base/optimization.h" #include "absl/strings/str_format.h" #include "absl/time/clock.h" #include "absl/time/time.h" @@ -42,6 +44,49 @@ namespace tcmalloc { namespace tcmalloc_internal { +namespace internal_transfer_cache { + +class TransferCacheTestPeer { + public: + // Checks that the fields hot on the hit path, the miss counters and the + // freelist do not false share. All checks are compile-time; the function + // exists only so that the static_asserts can name private members. + template + static constexpr bool VerifyLayout() { + // lock_ and the state touched on the hit path share the first cacheline. + static_assert(offsetof(TC, lock_) == 0); + static_assert(offsetof(TC, slots_) + sizeof(void*) <= ABSL_CACHELINE_SIZE); + // The miss counters are written by every core that falls through to the + // freelist; they get a cacheline of their own, apart from lock_. + static_assert(offsetof(TC, insert_misses_) % ABSL_CACHELINE_SIZE == 0); + static_assert(offsetof(TC, remove_object_misses_) + sizeof(MissCounts) <= + offsetof(TC, insert_misses_) + ABSL_CACHELINE_SIZE); + constexpr size_t kFreeListOffset = + offsetof(TC, freelist_do_not_access_directly_); + if constexpr (sizeof(typename TC::FreeList) <= sizeof(void*)) { + // An immutable forwarder packs into the miss counters' cacheline. + static_assert(kFreeListOffset >= offsetof(TC, insert_misses_)); + static_assert(kFreeListOffset + sizeof(typename TC::FreeList) <= + offsetof(TC, insert_misses_) + ABSL_CACHELINE_SIZE); + static_assert(sizeof(TC) == 2 * ABSL_CACHELINE_SIZE); + } else { + // A freelist with its own lock at offset 0 must not share a cacheline + // with lock_ or with the miss counters. + static_assert(kFreeListOffset % ABSL_CACHELINE_SIZE == 0); + static_assert(kFreeListOffset >= + offsetof(TC, insert_misses_) + ABSL_CACHELINE_SIZE); + } + return true; + } +}; + +static_assert(TransferCacheTestPeer::VerifyLayout< + TransferCache>()); +static_assert(TransferCacheTestPeer::VerifyLayout< + TransferCache>()); + +} // namespace internal_transfer_cache + namespace { using ::testing::Return;