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;