From 27d31c6fd3add9ed33f18d5536fb6028df4a9942 Mon Sep 17 00:00:00 2001 From: Chris Kennelly Date: Fri, 28 Aug 2026 22:41:03 -0700 Subject: [PATCH] Fix flakiness in ProfileTest.EventTraceTruncation. In EventTraceTruncation, allocations were performed after an initial SleepFor(20ms), allowing background allocations during the sleep to consume the small trace memory buffer (kEventTraceMemoryLimit = 2048, capacity for only 1 pair) before the early allocation was recorded, causing intermittent truncation of the expected early event. PiperOrigin-RevId: 972970666 --- tcmalloc/BUILD | 3 -- tcmalloc/central_freelist.h | 27 ++++------ tcmalloc/central_freelist_test.cc | 24 ++++----- tcmalloc/huge_region_fuzz.cc | 66 +++-------------------- tcmalloc/internal/percpu_tcmalloc_fuzz.cc | 9 +--- tcmalloc/span_fuzz.cc | 38 +------------ tcmalloc/static_vars.cc | 2 +- tcmalloc/testing/profile_test.cc | 13 ++--- tcmalloc/testing/tcmalloc_benchmark.cc | 33 ------------ 9 files changed, 34 insertions(+), 181 deletions(-) diff --git a/tcmalloc/BUILD b/tcmalloc/BUILD index cd6b4b520..d0001775c 100644 --- a/tcmalloc/BUILD +++ b/tcmalloc/BUILD @@ -787,9 +787,6 @@ cc_test( "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/log:check", - "@com_google_absl//absl/strings", - "@com_google_absl//absl/strings:str_format", - "@com_google_absl//absl/strings:string_view", "@com_google_absl//absl/types:span", "@com_google_fuzztest//fuzztest", "@com_google_fuzztest//fuzztest:fuzztest_gtest_main", diff --git a/tcmalloc/central_freelist.h b/tcmalloc/central_freelist.h index 4a465748c..d533dbb02 100644 --- a/tcmalloc/central_freelist.h +++ b/tcmalloc/central_freelist.h @@ -142,9 +142,6 @@ class CentralFreeList { public: using Forwarder = ForwarderT; - static constexpr size_t kSameSpanBucketCapacity = - absl::bit_width(kMaxObjectsToMove); - constexpr CentralFreeList() : lock_(absl::base_internal::SCHEDULE_KERNEL_ONLY), size_class_(0), @@ -347,12 +344,9 @@ class CentralFreeList { // Records histogram of how many consecutive objects fell on the same span for // batches. // - // Index in this array corresponds to absl::bit_width(same_span), yielding - // 8 buckets total because same_span has range [0, 127] (assuming - // kMaxObjectsToMove is 128). - // - // TODO(b/527641380): Delete this after wrapping up optimizations. - StatsCounter num_same_spans_[kSameSpanBucketCapacity]; + // Note: This goes to kMaxObjectsToMove and not kMaxObjectsToMove+1, since we + // ignore the very first object. + StatsCounter num_same_spans_[kMaxObjectsToMove]; #endif // TCMALLOC_INTERNAL_LEGACY_LOCKING // Num free objects in cache entry @@ -635,9 +629,7 @@ inline void CentralFreeList::InsertRange(absl::Span batch) { #ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING const int same_span = batch.size() - runs; - TC_ASSERT_GE(same_span, 0); - num_same_spans_[absl::bit_width(static_cast(same_span))] - .LossyAdd(1); + num_same_spans_[same_span].LossyAdd(1); #endif RecordMultiSpansDeallocated(free_count); @@ -844,7 +836,8 @@ template inline void CentralFreeList::PrintSameSpanStats(Printer& out) { #ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING out.printf("class %3d [ %8zu bytes ] :", size_class_, object_size_); - for (int i = 0; i < kSameSpanBucketCapacity; ++i) { + // num_same_spans_ is exclusive with num_to_move_, not inclusive. + for (int i = 0; i < num_to_move_; ++i) { out.printf(" %6zu", num_same_spans_[i].value()); } out.printf("\n"); @@ -855,16 +848,14 @@ template inline void CentralFreeList::PrintSameSpanStatsInPbtxt( PbtxtRegion& region) { #ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING - for (int i = 0; i < kSameSpanBucketCapacity; ++i) { + // num_same_spans_ is exclusive with num_to_move_, not inclusive. + for (int i = 0; i < num_to_move_; ++i) { auto value = num_same_spans_[i].value(); if (value == 0) { continue; } PbtxtRegion histogram = region.CreateSubRegion("same_span_stats"); - int lower_bound = i == 0 ? 0 : (1 << (i - 1)); - int upper_bound = i == 0 ? 0 : ((1 << i) - 1); - histogram.PrintI64("lower_bound", lower_bound); - histogram.PrintI64("upper_bound", upper_bound); + histogram.PrintI64("lower_bound", i); histogram.PrintI64("value", value); } #endif // TCMALLOC_INTERNAL_LEGACY_LOCKING diff --git a/tcmalloc/central_freelist_test.cc b/tcmalloc/central_freelist_test.cc index dbd5584d5..4be7966a9 100644 --- a/tcmalloc/central_freelist_test.cc +++ b/tcmalloc/central_freelist_test.cc @@ -383,7 +383,7 @@ class CentralFreeListTestPeer { static size_t num_same_spans(const CentralFreeList& cfl, size_t index) { #ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING - return cfl.num_same_spans_[absl::bit_width(index)].value(); + return cfl.num_same_spans_[index].value(); #else return 0; #endif @@ -1114,10 +1114,9 @@ TEST_P(CentralFreeListTest, SameSpans) { { std::string expected_stats = absl::StrFormat( "class %3d [ %8zu bytes ] :", e.kSizeClass, GetParam().size); - for (int i = 0; i < CentralFreeList::kSameSpanBucketCapacity; ++i) { - const bool first_batch = e.objects_per_span() > 1 && - i == absl::bit_width(static_cast( - got - pseudo_spans.size())); + for (int i = 0; i < num_to_move; ++i) { + const bool first_batch = + e.objects_per_span() > 1 && i == got - pseudo_spans.size(); const int count = first_batch ? 1 : 0; absl::StrAppendFormat(&expected_stats, " %6d", count); } @@ -1129,16 +1128,11 @@ TEST_P(CentralFreeListTest, SameSpans) { EXPECT_EQ(buffer, expected_stats) << got; } { - std::string expected_pbtxt = ""; - if (e.objects_per_span() > 1) { - int same_span_val = got - pseudo_spans.size(); - int bucket = absl::bit_width(static_cast(same_span_val)); - int lower_bound = bucket == 0 ? 0 : (1 << (bucket - 1)); - int upper_bound = bucket == 0 ? 0 : ((1 << bucket) - 1); - expected_pbtxt = absl::StrFormat( - " same_span_stats { lower_bound: %d upper_bound: %d value: 1}", - lower_bound, upper_bound); - } + std::string expected_pbtxt = + e.objects_per_span() > 1 + ? absl::StrFormat(" same_span_stats { lower_bound: %d value: 1}", + got - pseudo_spans.size()) + : ""; std::string buffer_pbtxt = PrintToString(1024 * 1024, [&](PbtxtRegion& region) { diff --git a/tcmalloc/huge_region_fuzz.cc b/tcmalloc/huge_region_fuzz.cc index 5aa0782ff..3df5c78a8 100644 --- a/tcmalloc/huge_region_fuzz.cc +++ b/tcmalloc/huge_region_fuzz.cc @@ -27,9 +27,6 @@ #include "absl/base/attributes.h" #include "absl/container/flat_hash_set.h" #include "absl/log/check.h" -#include "absl/strings/str_format.h" -#include "absl/strings/str_join.h" -#include "absl/strings/string_view.h" #include "absl/types/span.h" #include "tcmalloc/huge_cache.h" #include "tcmalloc/huge_pages.h" @@ -131,17 +128,6 @@ struct Toggle { void Perform(State& state) const; }; -struct SetUnbackSuccess { - bool success; - - template - friend void AbslStringify(Sink& sink, const SetUnbackSuccess& s) { - absl::Format(&sink, "SetUnbackSuccess{.success=%v}", s.success); - } - - void Perform(State& state) const; -}; - struct Reentrant; struct GatherStatsPbtxt { @@ -162,9 +148,8 @@ struct PrintStats { void Perform(State& state) const; }; -using Instruction = - std::variant; +using Instruction = std::variant; struct Reentrant { std::vector subprogram; @@ -229,30 +214,22 @@ struct State { region.Put(alloc, false); } allocs.clear(); - EXPECT_EQ(region.used_pages(), Length(0)); CheckInvariants(); } void Execute(absl::Span instructions) { for (const auto& inst : instructions) { std::visit([&](const auto& arg) { arg.Perform(*this); }, inst); - CheckInvariants(); } } void CheckInvariants() { + TC_CHECK_EQ(region.used_pages(), Length(0)); SmallSpanStats small; LargeSpanStats large; region.AddSpanStats(&small, &large); - ASSERT_LE(region.free_backed(), region.backed()); - ASSERT_LE(region.backed(), region.size()); - BackingStats stats = region.stats(); - EXPECT_EQ(stats.system_bytes, HugeRegion::size().in_bytes()); - EXPECT_EQ(stats.free_bytes, region.free_pages().in_bytes()); - EXPECT_EQ(stats.unmapped_bytes, region.unmapped_pages().in_bytes()); - EXPECT_EQ( - region.used_pages() + region.free_pages() + region.unmapped_pages(), - HugeRegion::size().in_pages()); + TC_CHECK_LE(region.free_backed(), region.backed()); + TC_CHECK_LE(region.backed(), region.size()); } }; @@ -263,8 +240,6 @@ void Allocate::Perform(State& state) const { if (!state.region.MaybeGet(n, &p, &from_released)) { return; } - EXPECT_TRUE(state.region.contains(p)); - EXPECT_TRUE(state.region.contains(p + n - Length(1))); state.allocs.emplace_back(p, n); if (!from_released) { return; @@ -301,7 +276,6 @@ void Release::Perform(State& state) const { TC_CHECK_EQ(actual, NHugePages(0)); return; } - if (max_expected > NHugePages(0) && len > Length(0)) { TC_CHECK_GT(actual, NHugePages(0)); } @@ -309,40 +283,16 @@ void Release::Perform(State& state) const { } void Stats::Perform(State& state) const { + state.region.stats(); SmallSpanStats small; LargeSpanStats large; state.region.AddSpanStats(&small, &large); - - Length small_normal_pages; - Length small_returned_pages; - for (size_t i = 0; i < kMaxPages.raw_num(); ++i) { - small_normal_pages += Length(i * small.normal_length[i]); - small_returned_pages += Length(i * small.returned_length[i]); - } - - EXPECT_EQ(small_normal_pages + large.normal_pages, state.region.free_pages()); - EXPECT_EQ(small_returned_pages + large.returned_pages, - state.region.unmapped_pages()); - - BackingStats stats = state.region.stats(); - EXPECT_EQ(stats.system_bytes, HugeRegion::size().in_bytes()); - EXPECT_EQ(stats.free_bytes, state.region.free_pages().in_bytes()); - EXPECT_EQ(stats.unmapped_bytes, state.region.unmapped_pages().in_bytes()); - EXPECT_EQ(state.region.used_pages() + state.region.free_pages() + - state.region.unmapped_pages(), - HugeRegion::size().in_pages()); - EXPECT_LE(state.region.free_backed(), state.region.backed()); - EXPECT_LE(state.region.backed(), state.region.size()); } void Toggle::Perform(State& state) const { state.unback.unback_success_ = !state.unback.unback_success_; } -void SetUnbackSuccess::Perform(State& state) const { - state.unback.unback_success_ = success; -} - void Reentrant::Perform(State& state) const { state.reentrant_stack.push_back(subprogram); } @@ -359,7 +309,6 @@ void GatherStatsPbtxt::Perform(State& state) const { void PrintStats::Perform(State& state) const { Printer p(&state.output[0], state.output.size()); state.region.Print(p); - ASSERT_LE(p.SpaceRequired(), state.output.size()); } void FuzzRegion(const std::vector& instructions, @@ -382,9 +331,6 @@ auto GetFlatInstructionDomain() { fuzztest::Arbitrary()), fuzztest::Map([](Toggle t) -> Instruction { return Instruction{t}; }, fuzztest::Arbitrary()), - fuzztest::Map( - [](SetUnbackSuccess s) -> Instruction { return Instruction{s}; }, - fuzztest::Arbitrary()), fuzztest::Map( [](GatherStatsPbtxt g) -> Instruction { return Instruction{g}; }, fuzztest::Arbitrary()), diff --git a/tcmalloc/internal/percpu_tcmalloc_fuzz.cc b/tcmalloc/internal/percpu_tcmalloc_fuzz.cc index fce33e84d..a71416c17 100644 --- a/tcmalloc/internal/percpu_tcmalloc_fuzz.cc +++ b/tcmalloc/internal/percpu_tcmalloc_fuzz.cc @@ -389,7 +389,7 @@ struct ShrinkOtherCache { template friend void AbslStringify(Sink& sink, const ShrinkOtherCache& s) { absl::Format(&sink, - "ShrinkOtherCache{.cpu_index=%v, .size_class=%v, .len=%v}", + "ShrinkOtherCache{.size_class=%v, .cpu_index=%v, .len=%v}", s.cpu_index, s.size_class, s.len); } @@ -711,13 +711,6 @@ TEST(PercpuTcmallocTest, FuzzPercpuTcmallocRegression) { StopCpu{.cpu_index = 1}, StartCpu{.cpu_index = 1}}); } -TEST(PercpuTcmallocTest, ShrinkOtherCacheStringify) { - EXPECT_EQ( - absl::StrFormat( - "%v", ShrinkOtherCache{.cpu_index = 1, .size_class = 2, .len = 3}), - "ShrinkOtherCache{.cpu_index=1, .size_class=2, .len=3}"); -} - FUZZ_TEST(PercpuTcmallocTest, FuzzPercpuTcmalloc) .WithDomains(fuzztest::Arbitrary>()); diff --git a/tcmalloc/span_fuzz.cc b/tcmalloc/span_fuzz.cc index 10e6476ad..aabc08941 100644 --- a/tcmalloc/span_fuzz.cc +++ b/tcmalloc/span_fuzz.cc @@ -63,8 +63,6 @@ struct State { std::vector live_ptrs; std::vector batch; std::mt19937 rng; - bool donated = false; - uint8_t nonempty_index = 0; State(size_t object_size, Length pages, size_t num_to_move) : object_size(object_size), @@ -190,41 +188,7 @@ struct DeallocIndex { } }; -struct SetBitpackedAttributes { - uint8_t nonempty_index; - bool donated; - - template - friend void AbslStringify(Sink& sink, const SetBitpackedAttributes& s) { - absl::Format(&sink, - "SetBitpackedAttributes{.nonempty_index=%v, .donated=%v}", - s.nonempty_index, s.donated); - } - - void Perform(State& state) const { - EXPECT_EQ(state.nonempty_index, state.span->nonempty_index()); - state.nonempty_index = nonempty_index % (1 << Span::kNonemptyIndexBits); - state.span->set_nonempty_index(state.nonempty_index); - EXPECT_EQ(state.span->nonempty_index(), state.nonempty_index); - - EXPECT_EQ(state.donated, state.span->donated()); - state.donated = donated; - state.span->set_donated(donated); - EXPECT_EQ(state.span->donated(), donated); - } -}; - -struct Prefetch { - template - friend void AbslStringify(Sink& sink, const Prefetch& p) { - absl::Format(&sink, "Prefetch{}"); - } - - void Perform(State& state) const { state.span->Prefetch(); } -}; - -using Instruction = std::variant; +using Instruction = std::variant; template void AbslStringify(Sink& sink, const Instruction& i) { diff --git a/tcmalloc/static_vars.cc b/tcmalloc/static_vars.cc index b1e5ededc..1b92aa63f 100644 --- a/tcmalloc/static_vars.cc +++ b/tcmalloc/static_vars.cc @@ -136,7 +136,7 @@ size_t Static::metadata_bytes() { sizeof(pageheap_lock) + sizeof(arena_) + sizeof(sizemap_) + sizeof(sharded_transfer_cache_) + sizeof(transfer_cache_) + sizeof(cpu_cache_) + sizeof(sampledallocation_allocator_) + - sizeof(span_allocator_) + sizeof(threadcache_allocator_) + + sizeof(span_allocator_) + +sizeof(threadcache_allocator_) + sizeof(sampled_allocation_recorder_) + sizeof(linked_sample_allocator_) + sizeof(inited_) + sizeof(cpu_cache_active_) + sizeof(page_allocator_) + sizeof(pagemap_) + sizeof(sampled_objects_size_) + diff --git a/tcmalloc/testing/profile_test.cc b/tcmalloc/testing/profile_test.cc index 473e0ff6c..0bacc7537 100644 --- a/tcmalloc/testing/profile_test.cc +++ b/tcmalloc/testing/profile_test.cc @@ -538,8 +538,10 @@ TEST(ProfileTest, EventTraceTruncation) { // Set a small memory limit to force truncation. // Note: A single matched allocation produces 2 records (alloc + dealloc), - // each ~600B, requiring at least ~1.3kB to *admit* the first pair. - constexpr int64_t kEventTraceMemoryLimit = 2048; + // each ~600B. A limit of 8192 allows initial allocations (including any + // background activity) to be recorded while ensuring late allocations are + // truncated by filler allocations. + constexpr int64_t kEventTraceMemoryLimit = 8192; constexpr size_t kApproximateDeallocationSampleRecordSize = 600; constexpr int kExpectedSampleCount = kEventTraceMemoryLimit / kApproximateDeallocationSampleRecordSize; @@ -554,13 +556,13 @@ TEST(ProfileTest, EventTraceTruncation) { const absl::Time test_start = absl::Now(); auto token = MallocExtension::StartEventTracing(); - // Sleep slightly to guarantee a non-zero, measurable duration. - absl::SleepFor(absl::Milliseconds(20)); - // Early allocations (should be captured in the trace). void* early_ptr = ::operator new(kEarlySize); ::operator delete(early_ptr); + // Sleep slightly to guarantee a non-zero, measurable duration. + absl::SleepFor(absl::Milliseconds(20)); + // Trigger enough allocations to exceed the memory limit. constexpr int kNumFillerAllocs = 50; for (int i = 0; i < kNumFillerAllocs; ++i) { @@ -608,7 +610,6 @@ TEST(ProfileTest, EventTraceTruncation) { } } EXPECT_TRUE(requested_size_id.has_value()); - int sample_count = 0; bool contains_early = false; bool contains_late = false; diff --git a/tcmalloc/testing/tcmalloc_benchmark.cc b/tcmalloc/testing/tcmalloc_benchmark.cc index c9b93e03f..64c198114 100644 --- a/tcmalloc/testing/tcmalloc_benchmark.cc +++ b/tcmalloc/testing/tcmalloc_benchmark.cc @@ -25,12 +25,6 @@ #include #include -#ifdef __x86_64__ -#include -#elif defined(__aarch64__) -#include -#endif - #include "absl/base/attributes.h" #include "absl/log/check.h" #include "absl/random/random.h" @@ -87,33 +81,6 @@ static void BM_new_sized_delete(benchmark::State& state) { } BENCHMARK(BM_new_sized_delete)->Range(1, 1 << 20); -#if defined(__x86_64__) || defined(__aarch64__) -// Make each call independent from each other (by an explicit fence), so that -// the CPU cannot extract ILP from running multiple allocations in parallel, -// and we measure latency instead of throughput. -static void BM_new_sized_delete_fence(benchmark::State& state) { - const int arg = state.range(0); - - CHECK_EQ(tcmalloc_internal::new_hooks_.size(), 0); - CHECK_EQ(tcmalloc_internal::delete_hooks_.size(), 0); - for (auto s : state) { -#if defined(__x86_64__) - // LFENCE is blessed after-the-fact to be a full speculation fence. - // https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html - // “The LFENCE instruction and the serializing instructions all ensure that - // no later instruction will execute, even speculatively, until all prior - // instructions have completed locally” - _mm_lfence(); -#else - __builtin_arm_isb(0xf); -#endif - void* ptr = ::operator new(arg); - ::operator delete(ptr, arg); - } -} -BENCHMARK(BM_new_sized_delete_fence)->Range(1, 1 << 20); -#endif - static void BM_new_sized_delete_cold(benchmark::State& state) { const int arg = state.range(0); const int hot_cold = state.range(1);