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

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxx/include/__locale:206-207
 {
-    once_flag      __flag_;
+    // Reserve space for a once_flag to preserve ABI compatibility
+    once_flag      __reserved;
     int32_t        __id_;
----------------
Kind of ugly that we still need to `#include "mutex"` just for this. Maybe I should change this to
```
#if defined(_LIBCPP_ABI_MICROSOFT)
   uintptr_t __reserved;
#else
   unsigned long __reserved;
#endif
```
And add a `static_assert(sizeof(locale::id.__reserved) == sizeof(once_flag))` somewhere. Not clear where I'd add it though.


================
Comment at: libcxx/include/__locale:215-216
 
-private:
-    void __init();
 public:  // only needed for tests
----------------
I think technically this removes a symbol from the ABI, but it's one that really shouldn't ever get used by anybody.
Does this mean I need to update something in `libcxx/lib`?


================
Comment at: libcxx/src/locale.cpp:716-732
+  // Before we do anything as expensive as acquire a mutex, check if __id_ has already been set
+  auto id_copy = __libcpp_atomic_load(&__id_, _AO_Relaxed);
+  if (id_copy != 0)
+    return id_copy - 1;
 
-    void operator()() const
-    {
-        (id_->*pmf_)();
-    }
-};
-
-}
-
-long
-locale::id::__get()
-{
-    call_once(__flag_, __fake_bind(&locale::id::__init, this));
-    return __id_ - 1;
-}
+#ifndef _LIBCPP_HAS_NO_THREADS
+  __libcpp_mutex_lock(&__m_);
----------------
I'm not 100% certain the atomic load/store is necessary or that I got the memory ordering right.

Looking at `std::call_once` as a reference, perhaps this should be
```
if(__libcpp_atomic_load(&__id_, _AO_Acquire) != 0)
```
and on line 726
```
__libcpp_atomic_store(&__id_, ++__next_id, _AO_Release);
```
However, I think the reason `std::call_once` uses acquire/release memory ordering is to make sure ["all concurrent calls to call_once are guaranteed to observe any side-effects made by the active call, with no additional synchronization"](https://en.cppreference.com/w/cpp/thread/call_once), which doesn't really apply here.


================
Comment at: libcxx/src/locale.cpp:732
-{
-    call_once(__flag_, __fake_bind(&locale::id::__init, this));
-    return __id_ - 1;
----------------
I could have also addressed this by pulling `__call_once` out of mutex.cpp into a separate file, and had that use `internal_threading_support.h`. That would have made `std::call_once` usable when the base threading support library (e.g. pthread) is unavailable at runtime. However, since `std::call_once` is part of the C++11 Thread Support Library, I figured it's better to change things here so we're consistent about what we do and don't support when the thread support library is unavailable (and we don't needlessly affect the performance of `std::call_once`).

Assuming this and D117373 are the only reason we support including `<mutex>` when `_LIBCPP_HAS_NO_THREADS` is defined (instead of giving an `#error` like we do with almost every other header that is part of the C++11 Thread support library), and both of these changes are accepted, it may make sense to stop supporting `<mutex>` with `_LIBCPP_HAS_NO_THREADS`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117372