From 80acb3bcf313cab89ca9d88cb13ae9f7c36f040a Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Wed, 6 Sep 2023 07:50:35 -0700 Subject: [PATCH] tcmalloc: remove some unnecessary thunk functions Both invoke_delete_hooks_and_free versions are used with only one target function. Using templates in this only over-abstracts and complicates things. Untemplate them. do_free_pages is only called via invoke_delete_hooks_and_free, so fold them to remove one level of indirection. FreePages is a no-op wrapper around invoke_delete_hooks_and_free, remove this unnecessary indirection. PiperOrigin-RevId: 563103299 Change-Id: I7ab294326bb024a4b9bc2d599f675f075650fa21 --- tcmalloc/tcmalloc.cc | 38 +++++++++----------------- tcmalloc/testing/memory_errors_test.cc | 4 +-- 2 files changed, 15 insertions(+), 27 deletions(-) diff --git a/tcmalloc/tcmalloc.cc b/tcmalloc/tcmalloc.cc index 2f8794efd..41875ace2 100644 --- a/tcmalloc/tcmalloc.cc +++ b/tcmalloc/tcmalloc.cc @@ -543,20 +543,10 @@ inline size_t GetSize(const void* ptr) { // function that both performs delete hooks calls and does free. This is done so // that free fast-path only does tail calls, which allow compiler to avoid // generating costly prologue/epilogue for fast-path. -template -static ABSL_ATTRIBUTE_SECTION(google_malloc) void invoke_delete_hooks_and_free( - void* ptr, size_t t) { +static void InvokeHooksAndFreeSmall(void* ptr, size_t size_class) { // Refresh the fast path state. GetThreadSampler()->UpdateFastPathState(); - return F(ptr, t); -} - -template -static ABSL_ATTRIBUTE_SECTION(google_malloc) void invoke_delete_hooks_and_free( - void* ptr, PageId p) { - // Refresh the fast path state. - GetThreadSampler()->UpdateFastPathState(); - return F(ptr, p); + FreeSmallSlow(ptr, size_class); } // Helper for do_free_with_size_class @@ -569,7 +559,7 @@ static inline ABSL_ATTRIBUTE_ALWAYS_INLINE void FreeSmall(void* ptr, } if (ABSL_PREDICT_FALSE(!GetThreadSampler()->IsOnFastPath())) { // Take the slow path. - invoke_delete_hooks_and_free(ptr, size_class); + InvokeHooksAndFreeSmall(ptr, size_class); return; } @@ -679,7 +669,11 @@ inline sized_ptr_t ABSL_ATTRIBUTE_ALWAYS_INLINE AllocSmall(Policy policy, // keep it out of fast-path. This helps avoid expensive // prologue/epilogue for fast-path freeing functions. ABSL_ATTRIBUTE_NOINLINE -static void do_free_pages(void* ptr, const PageId p) { +static void InvokeHooksAndFreePages(void* ptr) { + // Refresh the fast path state. + GetThreadSampler()->UpdateFastPathState(); + const PageId p = PageIdContaining(ptr); + Span* span = tc_globals.pagemap().GetExistingDescriptor(p); CHECK_CONDITION(span != nullptr && "Possible double free detected"); // Prefetch now to avoid a stall accessing *span while under the lock. @@ -742,8 +736,6 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size_class( // !have_size_class -> size_class == 0 ASSERT(have_size_class || size_class == 0); - const PageId p = PageIdContaining(ptr); - // if we have_size_class, then we've excluded ptr == nullptr case. See // comment in do_free_with_size. Thus we only bother testing nullptr // in non-sized case. @@ -758,6 +750,7 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size_class( // therefore static initialization must have already occurred. ASSERT(tc_globals.IsInited()); + const PageId p = PageIdContaining(ptr); if (!have_size_class) { size_class = tc_globals.pagemap().sizeclass(p); } @@ -767,7 +760,7 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size_class( ASSERT(!tc_globals.pagemap().GetExistingDescriptor(p)->sampled()); FreeSmall(ptr, size_class); } else { - invoke_delete_hooks_and_free(ptr, p); + InvokeHooksAndFreePages(ptr); } } @@ -780,11 +773,6 @@ bool CorrectSize(void* ptr, size_t size, AlignPolicy align); bool CorrectAlignment(void* ptr, std::align_val_t alignment); -inline ABSL_ATTRIBUTE_ALWAYS_INLINE void FreePages(void* ptr) { - const PageId p = PageIdContaining(ptr); - invoke_delete_hooks_and_free(ptr, p); -} - template inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size(void* ptr, size_t size, @@ -804,7 +792,7 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size(void* ptr, if (!IsColdMemory(ptr)) { // we don't know true class size of the ptr if (ptr == nullptr) return; - return FreePages(ptr); + return InvokeHooksAndFreePages(ptr); } else { // This code is redundant with the outer !IsSampledMemory path below, but // this avoids putting the IsSampledMemory&&IsColdMemory check on the @@ -822,7 +810,7 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size(void* ptr, ASSERT(size > kMaxSize || align.align() > alignof(std::max_align_t)); static_assert(kMaxSize >= kPageSize, "kMaxSize must be at least kPageSize"); - return FreePages(ptr); + return InvokeHooksAndFreePages(ptr); } return do_free_with_size_class(ptr, size_class); @@ -843,7 +831,7 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size(void* ptr, // We couldn't calculate the size class, which means size > kMaxSize. ASSERT(size > kMaxSize || align.align() > alignof(std::max_align_t)); static_assert(kMaxSize >= kPageSize, "kMaxSize must be at least kPageSize"); - return FreePages(ptr); + return InvokeHooksAndFreePages(ptr); } return do_free_with_size_class(ptr, size_class); diff --git a/tcmalloc/testing/memory_errors_test.cc b/tcmalloc/testing/memory_errors_test.cc index 9bbfd0788..9ea4cc4b9 100644 --- a/tcmalloc/testing/memory_errors_test.cc +++ b/tcmalloc/testing/memory_errors_test.cc @@ -354,8 +354,8 @@ TEST(AlwaysSamplingTest, DoubleFree) { }; EXPECT_DEATH(DoubleFree(), "span != " - "nullptr|Span::Unsample\\(\\)|Span::IN_USE|invoke_delete_hooks_" - "and_free<>\\(\\)"); + "nullptr|Span::Unsample\\(\\)|Span::IN_USE|" + "InvokeHooksAndFreePages\\(\\)"); } TEST(AlwaysSamplingTest, ReallocLarger) {