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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.
Herald added a project: All.

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.

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



================
Comment at: libcxx/src/locale.cpp:717
+  // 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)
----------------
Where does `__id_` get initialized?


================
Comment at: libcxx/src/locale.cpp:718
+  auto id_copy = __libcpp_atomic_load(&__id_, _AO_Relaxed);
+  if (id_copy != 0)
+    return id_copy - 1;
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117372