Skip to content

Commit

Permalink
tcmalloc: remove some unnecessary thunk functions
Browse files Browse the repository at this point in the history
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<do_free_pages>,
remove this unnecessary indirection.

PiperOrigin-RevId: 563103299
Change-Id: I7ab294326bb024a4b9bc2d599f675f075650fa21
  • Loading branch information
dvyukov authored and copybara-github committed Sep 6, 2023
1 parent 6b6d5b1 commit 80acb3b
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 27 deletions.
38 changes: 13 additions & 25 deletions tcmalloc/tcmalloc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 <void F(void*, size_t)>
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 <void F(void*, PageId)>
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
Expand All @@ -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<FreeSmallSlow>(ptr, size_class);
InvokeHooksAndFreeSmall(ptr, size_class);
return;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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);
}
Expand All @@ -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<do_free_pages>(ptr, p);
InvokeHooksAndFreePages(ptr);
}
}

Expand All @@ -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<do_free_pages>(ptr, p);
}

template <typename AlignPolicy>
inline ABSL_ATTRIBUTE_ALWAYS_INLINE void do_free_with_size(void* ptr,
size_t size,
Expand All @@ -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
Expand All @@ -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<true>(ptr, size_class);
Expand All @@ -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<true>(ptr, size_class);
Expand Down
4 changes: 2 additions & 2 deletions tcmalloc/testing/memory_errors_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 80acb3b

Please sign in to comment.