[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#4453925 <https://reviews.llvm.org/D152208#4453925>, @philnik wrote:

> In D152208#4452378 <https://reviews.llvm.org/D152208#4452378>, @hans wrote:
>
>> In D152208#4448532 <https://reviews.llvm.org/D152208#4448532>, @hans wrote:
>>
>>> We're hitting CFI errors in Chromium after this, see https://github.com/llvm/llvm-project/issues/63523
>>
>> Friendly ping to make sure this doesn't get lost in the other discussion.
>
> I'm working on adding a CFI run to our CI right now. It looks like I just have to add `_LIBCPP_NO_CFI` to `__make_uninitialized_buffer`, given that `get_temporary_buffer` also has that attribute. I hope to get the patch completed by tomorrow.
>
> In D152208#4453582 <https://reviews.llvm.org/D152208#4453582>, @davidxl wrote:
>
>> Should we partially revert this change? Instead of making use of the new interface for stable_sort, the old  get_temporary_buffer can be used (as an internal API), until the performance parity is reached?
>
> I don't see why. If the allocation is actually so costly that it dominates `stable_sort`, you should either look into whether you actually need `stable_sort` for your workload

Forgot to answer the common on the use of stable_sort:

The stable sort is called by another common library not controlled directly by the user. Besides the behavior depends on the input. The problem will manifest when the array to be sorted is small.

> or look into getting a better allocator (TCMalloc is probably not bad, so I suspect that's not the problem). Sorting doesn't seem like a task where a single allocation should ever dominate. I'd also like to see the actual use case where a single branch is so problematic that it warrants a revert before doing so. I don't even know how much of an impact this patch actually is in your specific use-case.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152208