[libcxx-commits] [PATCH] D117372: [libcxx] switch locale from using std::call_once to __libcpp_mutex_t

Thu Mar 10 17:20:41 PST 2022

DanielMcIntosh-IBM added a comment.

In D117372#3374008 <https://reviews.llvm.org/D117372#3374008>, @EricWF wrote:

> Do we have tests that fire up a bunch of threads and construct various `locale::id`s concurrently? If not we should add some. That will allow TSAN to diagnose bugs.

None that I can find. Should that be part of a separate patch?

> Overall, I think the current approach is still the best approach. `call_once` is always available in libc++, so you can use it unconditionally.

I would argue that `call_once` shouldn't always be available in libc++. Specifically, it shouldn't be available when `_LIBCPP_HAS_NO_THREADS` is defined. Nearly every header that is part of the C++11 Thread Support Library <https://en.cppreference.com/w/cpp/thread> (i.e. `<thread>`, `<mutex>`, `<shared_mutex>`, `<condition_variable>`, `<semaphore>`, `<latch>` and `<future>`) has the following right near at the top:

  #ifdef _LIBCPP_HAS_NO_THREADS
  # error <latch> is not supported on this single threaded system
  #endif

The only exceptions are `<mutex>` and `<condition_variable>`. However, all of `<condition_variable>` is wrapped in a `#ifndef _LIBCPP_HAS_NO_THREADS`, so including it does nothing when `_LIBCPP_HAS_NO_THREADS` is defined (aside from the `#include` of `<__config>`, `<__mutex_base>`, `<memory>` and `<version>` at the very start, but `<__mutex_base>` is similarly wrapped in a `#ifndef _LIBCPP_HAS_NO_THREADS`). `<mutex>` does the exact same thing except it's careful to still provide all the definitions and forward declarations needed for `std::call_once` so that we can use it here in `libcxx/src/locale.cpp`.

This seems like a very poor design to me, and if I had to guess the only reason it's like this is probably because `_LIBCPP_HAS_NO_THREADS` didn't exist when `locale::id::__get` was written, and we didn't bother to change the design when we added it. I would actually argue that we should have forbidden the use of `<mutex>` and `<condition_variable>` with `_LIBCPP_HAS_NO_THREADS`, because there's nothing in there that should realistically be used by a single-threaded application. As a user, if you're on a platform without threads, using `std::call_once` for anything is frankly ridiculous - replacing the `std::once_flag` with a boolean, and using a simple if statement to check the boolean makes WAYYYYY more sense. However, at this point, it's maybe a little late to be doing that, since we've already opened the door to such bizarre uses of libc++. We could maybe still add something like this to `<mutex>` and `<condition_variable>` (though `#warning` is non-standard):

  #ifdef _LIBCPP_HAS_NO_THREADS
  # warning <mutex> included on a single threaded system
  #endif

Any changes to `<mutex>` and `<condition_variable>` aside, when we're on a single-threaded system, libc++ itself certainly (in my mind) shouldn't be relying on anything from `<mutex>` or `<condition_variable>` (including `std::call_once`).

================
Comment at: libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist:1707
-{'is_defined': True, 'name': '__ZNSt3__16locale2id6__initEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__16locale2id9__next_idE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__16locale3allE', 'size': 0, 'type': 'OBJECT'}
----------------
EricWF wrote:
> The fact this function went away, means the ABI is potentially broken.
I was under the impression this wouldn't be the first time we've removed a function from the ABI?
E.g. In libcxx/lib/abi/CHANGELOG.TXT there's this at line 816:

> This change also marks __start_std_streams as hidden -- this variable is only required to initialize the streams, and nobody should depend on it from outside the dylib.

This function is in a similar situation: It should only be getting called from `libcxx/src/locale.cpp`, which is a compiled libc++ source file and thus part of the dylib.

================
Comment at: libcxx/src/locale.cpp:718
+  auto id_copy = __libcpp_atomic_load(&__id_, _AO_Relaxed);
+  if (id_copy != 0)
+    return id_copy - 1;
----------------
EricWF wrote:
> Before this change, there was no synchronization  between different instances of `locale::id` objects because each object had its own `__flag`. There was an atomic increment, but that was non-blocking.
> 
> Now, every construction of a new `locale::id` potentially blocks every construction of `locale::id`. This could significantly effect the behavior of existing multithreaded programs that use a lot of locales and a lot of threads.
> 
> Also, couldn't this just be written
> 
> ```
> static int& __id_ref = (__id_ = __cxx_atomic_add(__next_id, 1));
> return __id; 
> ```
> 
> Which is just a fancier way of writing the current `call_once` version. (Which is available in all dialects and with/without threads).
There was still synchronization, it was just buried in `std::call_once` (specifically inside `std::__call_once` at libcxx/src/mutex.cpp:231).

If anything, this version is less restrictive since the memory ordering here is `_AO_Relaxed` instead of `_AO_Acquire` like in `std::call_once`. This could improve performance and reduce the amount of time spent blocked/waiting since it allows more re-ordering of instructions than before. However, as I mentioned in a comment a little further down I'm not 100% sure I actually got this memory ordering right, since I'm not very familiar with them.

As for your proposed alternative, I'm not super familiar with how static local references work, so I might be missing something, but won't that result in only 1 call to `__cxx_atomic_add` and thus only one increment operation on `__next_id`? The current implementation gets around this by using a different `__flag_` per instance of `locale::id` (recall that `__flag_` is a non-static member of `locale::id`). I can't see any way of replicating this behaviour using just static locals.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117372