Re: [PATCH] improve performance of std::allocator::deallocate

[Pádraig Brady <pbrady at fb dot com>]

On 02/26/2019 05:50 AM, Jonathan Wakely wrote:
> On 23/02/19 02:04 +0000, Pádraig Brady wrote:
>> Attached is a simple patch which has been extensively tested within
>> Facebook,
>> and is enabled by default in our code base.
>>
>> Passing the size to the allocator allows it to optimize deallocation,
>> and this was seen to significantly reduce the work required in jemalloc,
>> with about 40% reduction in CPU cycles in the free path.
>
> Thanks, the patch looks good.
>
> I would prefer to only change the function body, as otherwise LTO
> sometimes produces link-time warnings about ODR violations, because
> the same function is defined on two different lines. The ODR detection
> heuristics aren't smart enough to complain when the function
> declarator is always defined on the same line, but with two different
> function bodies.

We've not noticed issues here with LTO, though that could be
due to global enablement of -fsized-deallocation.
Anyway your suggestion is neater. Updated patch attached.
> Note jemalloc >= 5.1 is required to fix a bug with 0 sizes.
>
> How serious is the bug? What are the symptoms?
>
I've updated the commit summary to say it's a crash.
Arguably that's better than mem corruption.

> It looks like 5.1.0 is less than a year old, so older versions are
> presumably still in wide use.
>
> We could potentially workaround it by making
> new_allocator::allocate(0) call ::operator new(1) when
> __cpp_sized_deallocation is defined, and deallocate(p, 0) call
> ::operator delete(p, 1). Obviously I'd prefer not to do that, because
> the default operator new already has an equivalent check, and only
> programs linking to jemalloc require the workaround.
>
Right the jemalloc fix was released May 2018.
It would be great to avoid the extra workarounds.
Given this would be released about a year after the jemalloc fix was
released,
and that this would only manifest upon program rebuild,
I would be inclined to not add any workarounds.
For reference tcmalloc and glibc malloc were seen to work fine with this.

thanks!
Pádraig.

From 54b69ee9ac7a188fa938108b4eac46a66416d0ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <pbrady@fb.com>
Date: Fri, 22 Feb 2019 17:21:57 -0800
Subject: [PATCH] std::allocator::deallocate support sized-deallocation

Pass the size to the allocator so that it may optimize deallocation.
This was seen to significantly reduce the work required in jemalloc,
with about 40% reduction in CPU cycles in the free path.
Note jemalloc >= 5.1 is required to fix a crash with 0 sizes.

* libstdc++-v3/include/ext/new_allocator.h (deallocate): Pass the size
to the deallocator with -fsized-deallocation.
---
 libstdc++-v3/include/ext/new_allocator.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/libstdc++-v3/include/ext/new_allocator.h b/libstdc++-v3/include/ext/new_allocator.h
index e245391..f1ff7da 100644
--- a/libstdc++-v3/include/ext/new_allocator.h
+++ b/libstdc++-v3/include/ext/new_allocator.h
@@ -116,16 +116,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       // __p is not permitted to be a null pointer.
       void
-      deallocate(pointer __p, size_type)
+      deallocate(pointer __p, size_type __t)
       {
 #if __cpp_aligned_new
 	if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
 	  {
-	    ::operator delete(__p, std::align_val_t(alignof(_Tp)));
+	    ::operator delete(__p,
+# if __cpp_sized_deallocation
+			      __t * sizeof(_Tp),
+# endif
+			      std::align_val_t(alignof(_Tp)));
 	    return;
 	  }
 #endif
-	::operator delete(__p);
+	::operator delete(__p
+#if __cpp_sized_deallocation
+			  , __t * sizeof(_Tp)
+#endif
+			 );
       }
 
       size_type
-- 
2.5.5