[libcxx-commits] [libcxx] [libc++] Optimize `std::exception_ptr` (PR #162773)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> Thanks for your review!
> 
> **Splitting the PR**
> 
> > Can we split this up [...]?
> 
> Happy to split it up 🙂
> 
> I would suggest a slightly different splitting across the commits, though:
> 
>     1. introduce benchmark
> 
>     2. change `src/exception_pointer_glibcxx.ipp` to use `_M_{add,release}ref` (still leaving the implementation in the library)
> 
>     3. expose the selected ABI via `__config_site.in` and change existing code to use it
> 
>     4. introduce move ctor/assign and possibly swap (the swap may or may not actually be necessary, we'll have to check)
> 
>     5. inline functions into the header and add the `if (ptr_)` fast paths to the inlined functions

I'm not convinced 2 and 3 are a good idea so far, so I'd rather do them later. I'm happy with (1) though and I think (4) can be implemented separately from moving any code (you seem to have come to the same conclusion below?)

> 
> Are we aligned at least for steps (1)-(4)? If so I will go ahead and open the corresponding PRs for those steps right away
> 
> ~However, I am not sure if splitting (4) and (5) into separate steps is actually a good idea. Step (4) would introduce new symbols to the library which would not be needed after step (5) anymore. I guess we should still split it into two separate PRs, but make sure to ship them immediately after each other, so we don't have new symbols in the library? Or maybe we should swap (4) and (5) around?~
> 
> Scratch that - I thought about this incorrectly. I will just mark the move ctor/assign/swap as inline from the get-go, and I thereby won't run into that problem

Yes, exactly.

> **Attributes for improving codegen**
> 
> > consider whether we can add any attributes to allow improved code gen
> 
> I am not quite sure what that step would actually entail. I am not currently aware of any specific attributes which would improve codegen. Do you have any specific attributes in mind?

No, nothing specific. It just looks to me like the kind of interface where slapping some attributes on would do the trick. Actually, looking at it again, I'm not sure my assessment was correct.

> **Inlining of `rethrow_exception` & `current_exception`**
> 
> > do the inlining of the rest of the functions you want, assuming you still think it's worth the cost.
> 
> Inlining the destructor, move assignment etc. will be necessary for performance. Inlining `rethrow_exception` and `current_exception` will not be worth it performance-wise.
> 
> I inlined `rethrow_exception` and `current_exception` primarily because
> 
>     1. it kept the per-ABI code more local / coherent. Instead of having two files per exception ABI (one in `include` and one in `src`), I preferred having a single file per ABI (in `include`)
> 
>     2. it needed less C++ boilerplate. I only needed the ABI dispatching `#ifdef`s in the `include` directory once instead of twice (once in `include`, once in `src`).
> 
> 
> WDYT about those points? Does it make sense to inline `rethrow_exception` and `current_exception` for code simplicity, even if it is not necessary for performance? ("Let's see later, as soon as we reach step (5)" is also a perfectly fine answer for me)

Yeah, let's see once we get there. I think we can be a bit more targeted here, so I'm not sure the argumentation will be relevant in the end.

https://github.com/llvm/llvm-project/pull/162773