Clean up SkDynamicAnnotations.

include/core/SkOnce.h

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkOnce_DEFINED
 #define SkOnce_DEFINED
 
 // Before trying SkOnce, see if SkLazyPtr or SkLazyFnPtr will work for you.
 // They're smaller and faster, if slightly less versatile.
 
 
 // SkOnce.h defines SK_DECLARE_STATIC_ONCE and SkOnce(), which you can use
 // together to create a threadsafe way to call a function just once.  E.g.
 //
 // static void register_my_stuff(GlobalRegistry* registry) {
 //     registry->register(...);
 // }
 // ...
 // void EnsureRegistered() {
 //     SK_DECLARE_STATIC_ONCE(once);
 //     SkOnce(&once, register_my_stuff, GetGlobalRegistry());
 // }
 //
 // No matter how many times you call EnsureRegistered(), register_my_stuff will be called just once.
 // OnceTest.cpp also should serve as a few other simple examples.
 
-#include "SkDynamicAnnotations.h"
-#include "SkThread.h"
-#include "SkTypes.h"
+#include "SkAtomics.h"
 
 // This must be used in a global scope, not in fuction scope or as a class member.
 #define SK_DECLARE_STATIC_ONCE(name) namespace {} static SkOnceFlag name
 
 class SkOnceFlag;
 
 inline void SkOnce(SkOnceFlag* once, void (*f)());
 
 template <typename Arg>
 inline void SkOnce(SkOnceFlag* once, void (*f)(Arg), Arg arg);
@@ skipping 35 matching lines @@
 // We've pulled a pretty standard double-checked locking implementation apart
 // into its main fast path and a slow path that's called when we suspect the
 // one-time code hasn't run yet.
 
 // This is the guts of the code, called when we suspect the one-time code hasn't been run yet.
 // This should be rarely called, so we separate it from SkOnce and don't mark it as inline.
 // (We don't mind if this is an actual function call, but odds are it'll be inlined anyway.)
 template <typename Lock, typename Arg>
 static void sk_once_slow(bool* done, Lock* lock, void (*f)(Arg), Arg arg) {
     lock->acquire();
     if (!*done) {

[Alexander Potapenko, 2015/03/11 09:09:44]

For the sake of uniformity this needs to be an atomic relaxed load.

[mtklein, 2015/03/11 15:31:20]

Done.

         f(arg);
         // Also known as a store-store/load-store barrier, this makes sure that the writes
         // done before here---in particular, those done by calling f(arg)---are observable
         // before the writes after the line, *done = true.
         //
         // In version control terms this is like saying, "check in the work up
         // to and including f(arg), then check in *done=true as a subsequent change".
         //
         // We'll use this in the fast path to make sure f(arg)'s effects are
         // observable whenever we observe *done == true.
         sk_release_store(done, true);
     }
     lock->release();
 }
 
 // This is our fast path, called all the time.  We do really want it to be inlined.
 template <typename Lock, typename Arg>
 inline void SkOnce(bool* done, Lock* lock, void (*f)(Arg), Arg arg) {
-    if (!SK_ANNOTATE_UNPROTECTED_READ(*done)) {
+    if (!sk_atomic_load(done, sk_memory_order_relaxed)) {
         sk_once_slow(done, lock, f, arg);
     }
     // Also known as a load-load/load-store barrier, this acquire barrier makes
     // sure that anything we read from memory---in particular, memory written by
     // calling f(arg)---is at least as current as the value we read from done.
     //
     // In version control terms, this is a lot like saying "sync up to the
     // commit where we wrote done = true".
     //
     // The release barrier in sk_once_slow guaranteed that done = true
     // happens after f(arg), so by syncing to done = true here we're
     // forcing ourselves to also wait until the effects of f(arg) are readble.
     SkAssertResult(sk_acquire_load(done));

[Alexander Potapenko, 2015/03/11 09:09:44]

|done| is always true at this point, so it looks like you're just doing an extra
memory load for no reason (unless it's optimized away in the release build, in
which case it should be a relaxed load).

I suggest you remove this line and do an acquire load at line 105 - guess it
should be at least not less correct.

[mtklein, 2015/03/11 15:31:20]

Done.  I think we were worried about the case when more than one thread enters
sk_once_slow(), that threads which lost the race to grab the lock might not have
also passed through an acquire barrier before they leave SkOnce.  But clearly
they do pass through an acquire barrier, the lock acquire() operation itself. 
I've updated the comments to explain this.

 }
 
 template <typename Arg>
 inline void SkOnce(SkOnceFlag* once, void (*f)(Arg), Arg arg) {
     return SkOnce(once->mutableDone(), once, f, arg);
 }
 
 // Calls its argument.
 // This lets us use functions that take no arguments with SkOnce methods above.
 // (We pass _this_ as the function and the no-arg function as its argument.  Cute eh?)
 static void sk_once_no_arg_adaptor(void (*f)()) {
     f();
 }
 
 inline void SkOnce(SkOnceFlag* once, void (*func)()) {
     return SkOnce(once, sk_once_no_arg_adaptor, func);
 }
 
 template <typename Lock>
 inline void SkOnce(bool* done, Lock* lock, void (*func)()) {
     return SkOnce(done, lock, sk_once_no_arg_adaptor, func);
 }
 
 #endif  // SkOnce_DEFINED