[libcxx] Patch/RFC: Avoid using ::operator new in the default allocator

[Howard Hinnant]

On Sep 30, 2013, at 2:23 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:

> 
> On 30.09.2013, at 19:45, Marshall Clow <mclow.lists at gmail.com> wrote:
> 
>> On Sep 25, 2013, at 2:21 PM, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>>> I also know that regardless of the solution, Marshall has thoughts on the best way to factor this within the library, but those are orthogonal.
>> 
>> Benjamin --
>> 
>> Here's my suggestion.
>> 
>> No matter how this comes out, I think having one place in the library where we allocate memory "in a default" manner is a good thing.
>> So let's do that first.
>> 
>> My suggestion is 
>> 
>> 1) Define two routines (probably in <new>):
>> 
>> _LIBCPP_ALWAYS_INLINE void *__allocate     ( std::size_t sz ) { return ::operator new ( sz ); }
>> _LIBCPP_ALWAYS_INLINE void *__deallocate ( void * p )         { return ::operator delete ( p ); }
>> 
>> [ MIght need different names, since hash_table has a __deallocate and dynarray has an __allocate and __deallocate ]
>> 
>> and call them from the places in <memory> that you noted.
>> 
>> 2) Make sure that works, then switch them (__allocate/__deallocate) to use new char []/delete [] and make sure that your optimization still works.
>> Then, switch them back while we figure out the correct way to solve this.
>> 
>> 3) Next, find all the places in libc++ where we call ::operator new/delete, and switch them over to calling __allocate/__deallocate.
>> That way, when we figure out the best way to solve this, we'll reap the benefits library-wide.
>> 
>> How does that sound to you?
> 
> It sounds like a great first step! Howard probably has an opinion on where's the best place for the new functions.

This sounds fine with me.

<new> looks like a good place to put these.  I'm ok with the names.  I don't think they will conflict with <__hash_table> or <dynarray> as those are member functions.  I like that these are _LIBCPP_ALWAYS_INLINE.  We should also prepend inline, just in case someone overrides _LIBCPP_ALWAYS_INLINE to do nothing (these still need to be weak).

The places we would need to change are:

<__sso_allocator>
<dynarray>
<memory> 3 places
<valarray> 17 places
thread.cpp

I still think we have a major conformance issue with subbing in new char[].  At the very least such a move would need committee buy-in.  And with my committee-hat on, I'm concerned about backwards compatibility.  I would be less concerned if the committee could agree to relax:

On Sep 25, 2013, at 3:46 PM, Chandler Carruth <chandlerc at google.com> wrote:

> This only applies to calls made as part of a new expression -- an explicit call cannot be transformed, it is allowed to have observed side effects according to as-if.

It seems illogical to me to allow one transformation but not the other because of backwards compatibility concerns.  And this seems to indicate that the committee *does* have backwards compatibility concerns.

I see a few ways forward, none of them easy, nor actionable by us alone.

1.  Get the LWG to change the places where they specify calls to ::operator new.  This will entail a backwards compatibility hit.  Not sure if it is acceptably small or not.

2.  Get the CWG to further relax where these types of optimizations can be done.  This will entail a backwards compatibility hit.  Not sure if it is acceptably small or not.

3.  There's a new polymorphic allocator on the horizon.  Make the spec for that sufficiently relaxed for these optimizations, and get it standardized.

Perhaps I'm missing another possibility?

At this point I have no objection to refactoring all calls to ::operator new/delete into __allocate/__deallocate as proposed by Marshall.  I also see no large motivation to do so either.  I'm neutral on it.  I don't see that it buys us anything at this time.  It is easy to do a search to see everywhere we are using operator new/delete, and it is not that many places.  So we could easily do this now, later or never.  With _LIBCPP_ALWAYS_INLINE as proposed, it will have no ABI impact.

Howard