diff --git a/libcxx/docs/DesignDocs/InternalThreadSynchronization.rst b/libcxx/docs/DesignDocs/InternalThreadSynchronization.rst --- a/libcxx/docs/DesignDocs/InternalThreadSynchronization.rst +++ b/libcxx/docs/DesignDocs/InternalThreadSynchronization.rst @@ -80,16 +80,12 @@ facet in the ``locale::__imp::facets_`` vector. For reasons described `here `_, ``locale::id::__id_`` is 0 initialized at first, and only truly intialized when the corresponding facet is first added to a locale (the -initialization occurs via the first call to ``locale::id::__get()``, which invokes -``locale::id::__init()``). +initialization occurs via the first call to ``locale::id::__get()``). This initialization must happen once for each ``locale::id``, but can occur on any thread. Since there is more than one ``locale::id``, and each one needs to independently perform this -initialization, a static local variable is insufficient. Instead, to prevent a race condition -between multiple threads trying to initialize the same facet's id, libcxx uses ``std::call_once`` -with each ``locale::id`` having it's own ``once_flag``, and to prevent a race condition on -the increment of ``locale::id::__next_id`` between threads trying to initialize different facets' -ids, it uses an atomic add. +initialization, a static local variable is insufficient. Instead, libcxx uses a mutex common to all +``locale::id``\s to guard both ``locale::id::__id_`` and ``locale::id::__next_id``. Static local variables @@ -220,8 +216,7 @@ Aside from exceptions_, our only concern in the remaining seven situations described above is preventing concurrent use of a resource which is not thread-safe: -* For locales the thread-unsafe resource is the ``once_flag`` used in ``std::call_once`` (which is - ensuring we only increment ``locale::id::__next_id`` once per facet) +* For locales the thread-unsafe resource is ``locale::id::__id_`` and ``locale::id::__next_id`` * For static local variables the thread-unsafe resource is the init byte * For the shared pointer overloads of ``std::atomic_...`` the thread-unsafe resource is the ``shared_ptr`` itself @@ -230,8 +225,7 @@ * For fallback malloc the thread-unsafe resource is the byte array global variable In all of these situations, the critical section(s) of code that use the resource which we're -trying to protect don't spawn threads, and don't invoke user code (while ``std::call_once`` can -invoke user code, that happens between two separate critical sections). Thus, if there is only one +trying to protect don't spawn threads, and don't invoke user code. Thus, if there is only one thread in existence when we would normally need to acquire the mutex (as would happen if the threading library is unavailable), there will only be one thread for the entirety of the critical section, meaning that it is safe to skip acquisition of the mutex. We can use this to avoid relying diff --git a/libcxx/include/__locale b/libcxx/include/__locale --- a/libcxx/include/__locale +++ b/libcxx/include/__locale @@ -203,7 +203,8 @@ class _LIBCPP_TYPE_VIS locale::id { - once_flag __flag_; + // Reserve space for a once_flag to preserve ABI compatibility + once_flag __reserved; int32_t __id_; static int32_t __next_id; @@ -212,8 +213,6 @@ void operator=(const id&) = delete; id(const id&) = delete; -private: - void __init(); public: // only needed for tests long __get(); diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp --- a/libcxx/src/locale.cpp +++ b/libcxx/src/locale.cpp @@ -12,6 +12,7 @@ #define _LCONV_C99 #endif +#include "__threading_support" #include "algorithm" #include "clocale" #include "codecvt" @@ -707,36 +708,29 @@ int32_t locale::id::__next_id = 0; -namespace -{ +#ifndef _LIBCPP_HAS_NO_THREADS +_LIBCPP_SAFE_STATIC static __libcpp_mutex_t __id_mut = _LIBCPP_MUTEX_INITIALIZER; +#endif -class __fake_bind -{ - locale::id* id_; - void (locale::id::* pmf_)(); -public: - __fake_bind(void (locale::id::* pmf)(), locale::id* id) - : id_(id), pmf_(pmf) {} +long locale::id::__get() { + // 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_); +#endif + id_copy = __id_; + if (id_copy == 0) { + id_copy = ++__next_id; + __libcpp_relaxed_store(&__id_, id_copy); + } -void -locale::id::__init() -{ - __id_ = __libcpp_atomic_add(&__next_id, 1); +#ifndef _LIBCPP_HAS_NO_THREADS + __libcpp_mutex_unlock(&__m_); +#endif + return id_copy - 1; } // template <> class collate_byname