[libcxx-commits] [PATCH] D113069: [libcxx][SystemZ][z/OS] Update libcxx/src/random_shuffle.cpp to accommodate POSIX(OFF)

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM marked 3 inline comments as done.
DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxx/src/random_shuffle.cpp:28
 #ifndef _LIBCPP_HAS_NO_THREADS
+  if (__libcpp_are_threads_enabled())
     __libcpp_mutex_lock(&__rs_mut);
----------------
Quuxplusone wrote:
> DanielMcIntosh-IBM wrote:
> > Quuxplusone wrote:
> > > jroelofs wrote:
> > > > should this guard go inside `__libcpp_mutex_{,un}lock` instead?
> > > (This function comes from parent revision D110349)
> > > FWLIW, I think yes it should.
> > I had considered doing that, but don't think it's a good idea because 
> > 1. it would result in a small performance hit in a lot of places
> > 2. since `cxa_guard` (which manages initialization of static local variables) uses `__libcpp_mutex_{,un}lock`, it would cause infinite recursion (`__libcpp_mutex_{,un}lock` -> `__libcpp_is_threading_api_enabled()` -> `cxa_guard` -> `__libcpp_mutex_{,un}lock`). This loop could be broken by getting rid of the static local variable in `__libcpp_is_threading_api_enabled()`, but I think that is a significantly worse option. Also, getting rid of the static local variable makes the performance hit in 1) a lot bigger
> Again FWLIW: Makes sense to me. :)
> I briefly wondered if `constinit` could help with the initialization in D110349, but no, it can't. Although... maybe you could use `atomic`? or is that a case of "now you have two problems"? ;) https://godbolt.org/z/objbvvdx3 In the thread-safe-static-initialization case, notice that declaring `__gtca()` as `noexcept` improves codegen, because then Clang doesn't have to codegen a "failure" path for the initialization.
The way `__gtca()` is currently defined/declared in `__gfunc.h`, Clang won't codegen the failure path anyways.

As for `atomic`, while it might work, I'm not convinced it would be any better performance wise than a static local variable, and it's definitely not easier to read.

However, using `atomic` in `__libcpp_ceeedb_flags()` might provide a significant performance benefit to `might_have_multiple_threads`. `CEECAA`, `CEECAAEDB`, and `CEEEDBFLAG1` won't change over the lifetime of the process, but since cxa_guard uses `might_have_multiple_threads` (see D110351), none of them can be static local variables. If using `atomic` there does provide a performance benefit, then it might be worth considering, but that can happen at a later date.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113069