Simplify implementation of SkOnce to not need so many comments.

I think this version reads more clearly, and the key invariants are
expressed in code rather than comments:
   - race losers always go through an acquire
   - we never exit the function unless fState is Done

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1951013004

Review-Url: https://codereview.chromium.org/1951013004

1 files changed, 10 insertions, 32 deletions

diff --git a/include/private/SkOnce.h b/include/private/SkOnce.h
index 507dcb5b8c..65334e3a26 100644
--- a/include/private/SkOnce.h
+++ b/include/private/SkOnce.h
@@ -29,44 +29,22 @@ public:
             return;
         }
 
-        if (state == NotStarted) {
-            // Try to claim the job of calling fn() by swapping from NotStarted to Calling.
-            // See [1] below for why we use std::memory_order_acquire instead of relaxed.
-            if (fState.compare_exchange_strong(state, Calling, std::memory_order_acquire)) {
-                // Claimed!  Call fn(), then mark this SkOnce as Done.
-                fn(std::forward<Args>(args)...);
-                return fState.store(Done, std::memory_order_release);
-            }
+        // If it looks like no one has started calling fn(), try to claim that job.
+        if (state == NotStarted && fState.compare_exchange_strong(state, Claimed,
+                                                                  std::memory_order_relaxed)) {
+            // Great!  We'll run fn() then notify the other threads by releasing Done into fState.
+            fn(std::forward<Args>(args)...);
+            return fState.store(Done, std::memory_order_release);
         }
 
-        while (state == Calling) {
-            // Some other thread is calling fn().  Wait for them to finish.
-            state = fState.load(std::memory_order_acquire);
-        }
-        SkASSERT(state == Done);
+        // Some other thread is calling fn().
+        // We'll just spin here acquiring until it releases Done into fState.
+        while (fState.load(std::memory_order_acquire) != Done) { /*spin*/ }
     }
 
 private:
-    enum State : uint8_t { NotStarted, Calling, Done};
+    enum State : uint8_t { NotStarted, Claimed, Done};
     std::atomic<uint8_t> 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 Calling or Done state.
- * Again, if we're in Calling state, relaxed would have been fine: the spin loop will
- * acquire up to the Calling 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