[libcxx-commits] [PATCH] D116334: [libc++] Remove std::basic_string's base class in ABIv2

Mon Jan 24 10:26:53 PST 2022

ldionne added a comment.

Can we please change the name to `_LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS`?

================
Comment at: libcxx/include/string:1724
     void __throw_out_of_range() const {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        __basic_string_common<true>::__throw_out_of_range();
-#else
-        _VSTD::abort();
-#endif
+        _VSTD::__throw_out_of_range("basic_string");
     }
----------------
philnik wrote:
> ldionne wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > Quuxplusone wrote:
> > > > > (1) My understanding is that the `_LIBCPP_NO_EXCEPTIONS` branch needs to remain. Someone who compiles with `-D_LIBCPP_NO_EXCEPTIONS` must not see any calls to `__throw_out_of_range` popping up at link time.
> > > > > 
> > > > > (2) I have the (more nebulous) impression that we call a dedicated library function `__basic_string_common<true>::__throw_out_of_range()` instead of `__throw_out_of_range("basic_string")` on purpose, because `std::string` is used so heavily, and we don't want the user to pay for the 13-byte string literal `"basic_string"` in every single one of their TUs. (This rationale does not apply in lesser-used places such as `deque`.) However, off the top of my head I don't know that we have ever benchmarked this or that anyone really cares.
> > > > (1) No that branch can be removed. I had the same initial feeling but `include/stdexcept` has the branch.
> > > > 
> > > > (2) That may be an issue indeed. I wonder whether the linker is/can be smart enough to remove the duplicates.
> > > > 
> > > I ran a few tests now. My general conclusion would be to put the throws directly into the header. The TU size doesn't really matter I think, because even changing the build location can have a bigger impact on the build directory size. (At least with CMake)
> > > 
> > > I used `export LINK_FLAGS="-nostdlib++ -L$LLVM/default/build/lib -Wl,-rpath,$LLVM/default/build/lib -lc++"; CC=clang CXX=clang++ cmake -G Ninja -DLLVM_ENABLE_PROJECTS="llvm;clang" -DCMAKE_BUILD_TYPE=Release ../../../llvm/ -DCMAKE_CXX_FLAGS="-nostdinc++ -nostdlib++ -isystem $LLVM/default/build/include/c++/v1" -DCMAKE_EXE_LINKER_FLAGS="$LINK_FLAGS" -DCMAKE_SHARED_LINKER_FLAGS="$LINK_FLAGS" -DLLVM_USE_LLD=On -DLLVM_ENABLE_EH=On -DLLVM_ENABLE_RTTI=On (-DLLVM_ENABLE_LTO=Thin)` to generate the build.
> > > I got these results:
> > > 
> > > 
> > > | throw in header | ThinLTO | clang-14 binary size (bytes)  |
> > > | yes | no | 163250384 |
> > > | no | no | 163250400 |
> > > | yes | yes | 182829280 |
> > > | no | yes | 182839312 |
> > > 
> > > I guess that especially with ThinLTO the functions can now be inlined or eliminated.
> > > I also timed the first tests but there was no significant difference between the two.
> > If you read the description in D111173, I explain that one of the ideas behind hiding the exception-throwing-business behind a function call is this:
> > 
> > > Encapsulate the code to throw an exception (which is non-trivial) in an externally-defined function so that the important code paths that call it (e.g. `vector::at`) are free from that code. Basically, the intent is for the "hot" code path to contain a single conditional jump (based on checking the error condition) to an externally-defined function, which handles all the exception-throwing business.
> > 
> > If you look at the code generation for `std::vector::at` before and after your change, you should see that it's more complicated after the change, because there's code to throw the exception. Before, it was a conditional jump to the function defined in the dylib, which then contained that business. I remember looking at the codegen and coming to that conclusion.
> > 
> > This patch shouldn't regress that.
> I don't know what you have looked at exactly, but I can't reproduce your problem. It seems to be that clang actually goes out of its way to not include the call in the hot path:
> 
> With `llvm-objdump --disassemble-symbols=_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE2atEm libc++.so.1 --x86-asm-syntax=intel | c++filt` I get:
> 
> ```
> 
> libc++.so.1:	file format elf64-x86-64
> 
> Disassembly of section .text:
> 
> 0000000000054ec0 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)>:
>    54ec0: 50                           	push	rax
>    54ec1: 0f b6 07                     	movzx	eax, byte ptr [rdi]
>    54ec4: a8 01                        	test	al, 1
>    54ec6: 74 06                        	je	0x54ece <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0xe>
>    54ec8: 48 8b 4f 08                  	mov	rcx, qword ptr [rdi + 8]
>    54ecc: eb 06                        	jmp	0x54ed4 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0x14>
>    54ece: 48 89 c1                     	mov	rcx, rax
>    54ed1: 48 d1 e9                     	shr	rcx
>    54ed4: 48 39 f1                     	cmp	rcx, rsi
>    54ed7: 76 1c                        	jbe	0x54ef5 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0x35>
>    54ed9: a8 01                        	test	al, 1
>    54edb: 74 0c                        	je	0x54ee9 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::at(unsigned long)+0x29>
>    54edd: 48 8b 7f 10                  	mov	rdi, qword ptr [rdi + 16]
>    54ee1: 48 01 f7                     	add	rdi, rsi
>    54ee4: 48 89 f8                     	mov	rax, rdi
>    54ee7: 59                           	pop	rcx
>    54ee8: c3                           	ret
>    54ee9: 48 83 c7 01                  	add	rdi, 1
>    54eed: 48 01 f7                     	add	rdi, rsi
>    54ef0: 48 89 f8                     	mov	rax, rdi
>    54ef3: 59                           	pop	rcx
>    54ef4: c3                           	ret
>    54ef5: e8 f6 e3 ff ff               	call	0x532f0 <std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__throw_out_of_range() const>
>    54efa: 66 0f 1f 44 00 00            	nop	word ptr [rax + rax]
> 
> ```
> Note the extra lines 54ef5 and 54efa. Clang went out of its way to even just have a jump (54ecc) in the hot path.
So I looked at this again, and indeed I am unable to reproduce what I was seeing, although I do clearly remember seeing *something*, otherwise it would have been a no-brainer to just scratch all the code and just call `_VSTD::__throw_foo()` back in D111173. But hey, I won't block this improvement if I can't find a downside!

Godbolt does show me they are the same: https://godbolt.org/z/rjshvGxoG

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116334