[libcxx-commits] [PATCH] D140944: [libc++] Optimize _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY

Tue Jan 3 20:22:22 PST 2023

philnik added a comment.

In D140944#4024901 <https://reviews.llvm.org/D140944#4024901>, @vitalybuka wrote:

> In D140944#4024899 <https://reviews.llvm.org/D140944#4024899>, @vitalybuka wrote:
>
>> In D140944#4024885 <https://reviews.llvm.org/D140944#4024885>, @philnik wrote:
>>
>>> Could you provide some numbers? This doesn't look great, but I don't think we care that much about binary size with sanitizers. It's not great anyways, so it would have to be quite a big difference to make it worth adding `_LIBCPP_ALWAYS_INLINE`. Also, please add a comment why it is added. We don't do that everywhere we use the attribute currently, but we should. So let's not make it worse.
>>
>> Actually at google we care about size with sanitizers.
>> server apps often close to 2GB of linker memory model limit
>> mobile always care about size
>>
>> With asan+ubsan combo _LIBCPP_ENABLE_ASSERTIONS=1 costs us 1.6%
>> with this patch it's just 0.2%
>
> actually it's 
> _LIBCPP_ENABLE_ASSERTIONS=1 _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY=1
> and _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY is there exactly to save size

`_LIBCPP_HAS_NO_VERBOSE_ABORTIN_LIBRARY` exists to make deploying to a target that doesn't have `__libcpp_verbose_abort` in the dylib possible, not to save size. You can use a custom verbose abort handler for that. I expect that we'll remove the inline version at some point (Not that this will happen soon).
Enabling sanitizers probably has a much larger size impact on it's own. While 1.6% vs. 0.2% is a large improvement, I'm not sure it's enough to justify adding `_LIBCPP_ALWAYS_INLINE`. Do you know why this doesn't get inlined? I would expect clang to be able to see through this, even with sanitizers. Fixing this in clang would probably have a much larger impact on overall binary size.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140944