SkOnce: 2 bytes -> 1 byte

include/private/SkOnce.h

Index: include/private/SkOnce.h
diff --git a/include/private/SkOnce.h b/include/private/SkOnce.h
index d83b63f2837382f0179a0c987a67b8a2f979f3b7..f19112da8a607212e95aeb16f58e421c1a5a16f8 100644
--- a/include/private/SkOnce.h
+++ b/include/private/SkOnce.h
@@ -8,9 +8,9 @@
 #ifndef SkOnce_DEFINED
 #define SkOnce_DEFINED
 
-#include "../private/SkSpinlock.h"
 #include <atomic>
 #include <utility>
+#include "SkTypes.h"
 
 // SkOnce provides call-once guarantees for Skia, much like std::once_flag/std::call_once().
 //
@@ -21,20 +21,46 @@ class SkOnce {
 public:
     template <typename Fn, typename... Args>
     void operator()(Fn&& fn, Args&&... args) {
-        // Vanilla double-checked locking.
-        if (!fDone.load(std::memory_order_acquire)) {
-            fLock.acquire();
-            if (!fDone.load(std::memory_order_relaxed)) {
+        auto state = fState.load(std::memory_order_acquire);
+
+        if (state == NotStarted) {

[herb_g, 2016/04/20 16:38:37]

Instead of having:
if (state == NotStarted) {
...
}
while (state == Active) {
...
}

could you wrap the whole thing in while.

while (state != Done) {
  if (state == NotStarted) {
    ...
  }
}

Making this change will reduce this code to using a single branch in the common
case, and while (state != Done) is a clearer invariant of the state needed to
proceed.

[mtklein_C, 2016/04/20 17:44:37]

Done.

+            // Try to claim the job of calling fn() by swapping from NotStarted to Active.
+            // See [1] below for why we use std::memory_order_acquire instead of relaxed.
+            if (fState.compare_exchange_strong(state, Active, std::memory_order_acquire)) {
+                // Claimed!  Call fn(), then mark this SkOnce as Done.
                 fn(std::forward<Args>(args)...);
-                fDone.store(true, std::memory_order_release);
+                return fState.store(Done, std::memory_order_release);
             }
-            fLock.release();
         }
+
+        while (state == Active) {
+            // Some other thread is calling fn().  Wait for them to finish.
+            state = fState.load(std::memory_order_acquire);
+        }
+        SkASSERT(state == Done);
     }
 
 private:
-    std::atomic<bool> fDone{false};
-    SkSpinlock        fLock;
+    enum State : uint8_t { NotStarted, Active, Done};

[herb_g, 2016/04/20 16:38:37]

To me, calling the state Initializing is clearer than Active. 

[mtklein_C, 2016/04/20 17:44:37]

I've changed it to Calling.

+    std::atomic<State> fState{NotStarted};
 };
 
+/* [1]  Why do we compare_exchange_strong() with std::memory_order_acquire instead of relaxed?
+ *
+ * If we succeed, we really only need a relaxed compare_exchange_strong()... we're the ones
+ * who are about to do a release store, so there's certainly nothing yet for an acquire to
+ * synchronize with.
+ *
+ * If that compare_exchange_strong() fails, we're either in Active or Done state.
+ * Again, if we're in Active state, relaxed would have been fine: the spin loop will
+ * acquire up to the active thread's release store.
+ *
+ * But if that compare_exchange_strong() fails and we find ourselves in the Done state,
+ * we've never done an acquire load to sync up to the store of that Done state.
+ *
+ * So on failure we need an acquire load.  Generally the failure memory order cannot be
+ * stronger than the success memory order, so we need acquire on success too.  The single
+ * memory order version of compare_exchange_strong() uses the same acquire order for both.
+ */
+
 #endif  // SkOnce_DEFINED