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

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

On 03/06/2019 12:50 AM, Jonathan Wakely wrote:
> On 06/03/19 02:43 +0000, Pádraig Brady wrote:
>>
>>
>> On 02/26/2019 04:23 PM, Padraig Brady wrote:
>>>
>>>> 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.
>>
>> Actually the jemalloc issue will only be fixed with the release of 5.2
>> (a few weeks away).
>> I've updated the commit message in the attached accordingly.
>
> Hmm, I'm a bit nervous about making a change that will cause crashes
> unless using an unreleased version (I know it will be released by the
> time GCC 9.1 is released, but some people might upgrade GCC without
> upgrading jemalloc).
Yes it's not ideal. It does make it a lot less risky that one has to
rebuild programs to get the new functionality, so existing programs
will be unaffected. Also -fsized-deallocation is only enabled by
default on gcc with -std >= c++14.
>
> On the other hand, zero sized allocations should be rare in practice.
Yes they were rare in testing here

So programs _rebuilt_ against the following would need to update
to jemalloc 5.2:

  zero sized allocs, jemalloc<5.2, c++>=14, GCC>=9.1

Hopefully that's a small enough set.

thanks,
Pádraig