On Sep 30, 2013, at 2:23 PM, Benjamin Kramer <[email protected]> wrote:
> > On 30.09.2013, at 19:45, Marshall Clow <[email protected]> wrote: > >> On Sep 25, 2013, at 2:21 PM, Chandler Carruth <[email protected]> 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 <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
