[libcxx-commits] [PATCH] D152208: [libc++] Introduce __make_uninitialized_buffer and use it instead of get_temporary_buffer

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D152208#4460803 <https://reviews.llvm.org/D152208#4460803>, @davidxl wrote:

> As I mentioned in the previous reply, the performance regression comes from addition runtime cost (allocation) in the context of __inplace_merge call.  A little more debugging shows that in the old version, the allocation can be completely elided when __buf_size is 0 (see get_temporary_buffer code), but make_uninitialized_buffer will try to invoke operator new regardless.
>
> // inplace_merge code
>
>   difference_type __len1 = _IterOps<_AlgPolicy>::distance(__first, __middle);
>   difference_type __len2 = _IterOps<_AlgPolicy>::distance(__middle, __last);
>   difference_type __buf_size = _VSTD::min(__len1, __len2);

Ok, so this whole thing has nothing to do with the fact that we are using aligned allocation vs not using aligned allocation. It has to do with the fact that we're allocating, period.

I'm going to revert this patch for now, because @EricWF pointed out that we actually had an issue with the class itself -- if constructing any of the elements in the buffer throws, the destruction is not going to happen properly (we'll over-destroy). This wasn't a problem with the use case that this was meant for (PSTL), since we `std::terminate()` if the construction of any element would throw. But I agree that is a poor general-purpose API to have. We can take another stab at this next week.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152208/new/

https://reviews.llvm.org/D152208