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

[David Li via Phabricator via libcxx-commits]

davidxl added a comment.

In D152208#4460003 <https://reviews.llvm.org/D152208#4460003>, @ldionne wrote:

> In D152208#4459623 <https://reviews.llvm.org/D152208#4459623>, @ldionne wrote:
>
>> IMO I agree that if we were using aligned allocation and we're not anymore, this is simply something that went unnoticed in this patch and we should fix it.
>>
>> Should `stable_sort` be dominated by an allocation? Probably not, that seems a bit crazy. But nonetheless it seems like we did introduce an unintended difference in this patch and it should be pretty easy to fix. Let's follow up on D152208 <https://reviews.llvm.org/D152208>.
>
> Looks like I read this issue the wrong way around. So we were doing unaligned allocation before, and now we're using aligned allocation (which should be more correct, no?) and it's slower. That's a lot more curious, and I think it's worth investigating why this caused a regression because there might be some sort of perf issue in how your allocator deals with aligned allocation (or maybe on the libc++ side). Anyway, let's follow-up in D152208 <https://reviews.llvm.org/D152208> to avoid forking the discussion too much.

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);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152208