[PATCH] D64358: [scudo][standalone] Merge Spin & Blocking mutex into a Hybrid one

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: lib/scudo/standalone/common.h:122
+  // Bionic uses a hardcoded value.
+  if (SCUDO_ANDROID)
+    return 4096U;
----------------
Could we potentially add an android-specific test to make sure this assumption doesn't change?

Also, could this potentially be a compile-time resolution? getPageSizeCached() is called for every allocation to the secondary allocator and seems like an unneccessary branch that can be eliminated.


================
Comment at: lib/scudo/standalone/mutex.h:21
 public:
-  void init() { atomic_store_relaxed(&State, 0); }
-
-  void lock() {
+  void init() { memset(this, 0, sizeof(*this)); }
+  bool tryLock();
----------------
Instead of memset-init here, can we simply have a `constexpr` constructor to guarantee zero-initialisation? Would potentially prevent forgotten `init()` in future.


================
Comment at: lib/scudo/standalone/mutex.h:27
+#ifdef __clang__
+#pragma nounroll
+#endif
----------------
Why?


================
Comment at: lib/scudo/standalone/mutex.h:29
+#endif
+    for (u8 I = 0U; I < 10U; I++) {
+      yieldProcessor(10U);
----------------
Could we have some sort of class-level constexpr here instead of just hardcoding the number `10`?


================
Comment at: lib/scudo/standalone/mutex.h:30
+    for (u8 I = 0U; I < 10U; I++) {
+      yieldProcessor(10U);
+      if (tryLock())
----------------
Same as above.


================
Comment at: lib/scudo/standalone/mutex.h:40
+  enum State { Unlocked = 0, Locked = 1, Sleeping = 2 };
+  uptr Storage[1];
+
----------------
Why is this a `uptr[1]`? It looks like the underlying `atomic_compare_exchange_strong` supports any atomic type, including char.

>From my reading of the spec, the underlying type for an unscoped enum is implementation defined. If you want a concrete type, you could always use `enum State : char { ... }`. If you're worried about alignment, you could always use the uptr as the backing type or `alignas`. I feel like `State Storage;` would be much more easy to understand here. You could always also use `enum class` to get a guaranteed integer-backed enum.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D64358