This is an archive of the discontinued LLVM Phabricator instance.

Implement sized deallocation for std::allocator and friends.
ClosedPublic

Authored by EricWF on Oct 10 2018, 5:33 PM.

Details

Summary

C++14 sized deallocation is disabled by default due to ABI concerns. However, when a user manually enables it then libc++ should take advantage of it since sized deallocation can provide a significant performance win depending on the underlying malloc implementation. (Note that libc++'s definitions of sized delete don't do anything special yet, but users are free to provide their own).

This patch updates __libcpp_deallocate to selectively call sized operator delete when it's available. __libcpp_deallocate_unsized should be used when the size of the allocation is unknown.

On Apple this patch makes no attempt to determine if the sized operator delete is unavailable, only that the language feature is enabled. This could cause a compile error when using std::allocator, but the same compile error would occur whenever the user calls new, so I don't think it's a problem.

Diff Detail

Repository
rCXX libc++

Event Timeline

EricWF created this revision.Oct 10 2018, 5:33 PM
ckennelly added inline comments.
include/new
345

Should this be specialized for the "don't know" case, rather than passing the size as 0?

For the common case (std::allocator<T>::deallocate()), we don't need to pay the cost of the branch.

EricWF updated this revision to Diff 169234.Oct 11 2018, 10:11 AM
EricWF marked an inline comment as done.
EricWF edited the summary of this revision. (Show Details)
  • Address inline comments about using 0 as a sentinel value and the branches it requires.
  • Convert <valarray> to use sized deallocation.
rsmith added a subscriber: rsmith.Oct 11 2018, 4:19 PM
rsmith added inline comments.
include/new
298–300

Any reason why one of these is returned but the other is not?

308–310

Likewise

include/valarray
3732–3738

I think __size would be clearer as __capacity here.

EricWF marked 3 inline comments as done.Oct 12 2018, 8:40 AM
EricWF added inline comments.
include/new
298–300

I prefer explicit returns when inside conditional compilation blocks. I'll add return's above as well.

345

Ack. I've fixed this by adding a __libcpp_deallocate_unsized.

EricWF updated this revision to Diff 169412.Oct 12 2018, 8:41 AM
EricWF marked an inline comment as done.

Address review comments.

ldionne requested changes to this revision.Oct 15 2018, 11:29 AM

C++14 sized deallocation is disabled by default due to ABI concerns.

@EricWF Can you expand on those ABI concerns? Do you mean because support for those is not provided in the dylib shipped by Apple?

On Apple this patch makes no attempt to determine if the sized operator delete is unavailable, only that the language feature is enabled.

You mean there's no check like there is for aligned allocation? For example (in __config):

#if defined(__APPLE__)
#  if defined(__MAC_OS_X_VERSION_MIN_REQUIRED)
#    if __MAC_OS_X_VERSION_MIN_REQUIRED < 1060
#      define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
#    endif
#  endif
#endif // defined(__APPLE__)

I think there should be one -- users won't expect calls to std::allocator::deallocate to suddenly start failing after this patch when they target an OS version that does not have sized deallocation. I think Apple started shipping support for sized deallocation in 10.12, so the check should be:

#if defined(__APPLE__)
#  if defined(__MAC_OS_X_VERSION_MIN_REQUIRED)
#    if __MAC_OS_X_VERSION_MIN_REQUIRED < 101200
#      define _LIBCPP_HAS_NO_LIBRARY_SIZED_DEALLOCATION
#    endif
#  endif
#endif // defined(__APPLE__)
include/new
295

I think you need _LIBCPP_INLINE_VISIBILITY here and on other private functions too.

src/experimental/memory_resource.cpp
37

Do you know why there's a FIXME here? (I know it was there before your patch, I'm just curious)

test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
11

typo: relevant

This revision now requires changes to proceed.Oct 15 2018, 11:29 AM

After talking w/ Eric:

  1. Can you please add a test that checks that std::allocator and new end up calling the same allocation/deallocation functions?
  2. I'm fine with not checking for availability on Mac OS. The rationale is that std::allocator should do the same as new: if you decide to turn on support for sized deallocation but your deployment target does not support sized deallocation, you should get a compiler error (regardless of whether you use new directly or use an allocator).
EricWF updated this revision to Diff 170362.Oct 21 2018, 6:10 PM
  • Make the library behavior of std::allocator match the language behavior in the current dialect.
ldionne accepted this revision.Oct 22 2018, 5:39 AM

LGTM

This revision is now accepted and ready to land.Oct 22 2018, 5:39 AM
EricWF updated this revision to Diff 171011.Oct 24 2018, 3:45 PM
  • Misc cleanup.
  • Work around Clang bug in C++03.
This revision was automatically updated to reflect the committed changes.