Allow linker initialization of lazy instance

base/lazy_instance.h

Index: base/lazy_instance.h
diff --git a/base/lazy_instance.h b/base/lazy_instance.h
index ac94a009111cf230ef70984add4b22034f53fdac..f6842dc534232072734caf8ea5b6e1cc3272f927 100644
--- a/base/lazy_instance.h
+++ b/base/lazy_instance.h
@@ -14,7 +14,7 @@
 // LazyInstance is completely thread safe, assuming that you create it safely.
 // The class was designed to be POD initialized, so it shouldn't require a
 // static constructor.  It really only makes sense to declare a LazyInstance as
-// a global variable using the base::LinkerInitialized constructor.
+// a global variable using the LAZY_INSTANCE_INITIALIZER initializer.
 //
 // LazyInstance is similar to Singleton, except it does not have the singleton
 // property.  You can have multiple LazyInstance's of the same type, and each
@@ -24,7 +24,7 @@
 // requires that Type be a complete type so we can determine the size.
 //
 // Example usage:
-//   static LazyInstance<MyClass> my_instance(base::LINKER_INITIALIZED);
+//   static LazyInstance<MyClass> my_instance = LAZY_INSTANCE_INITIALIZER;
 //   void SomeMethod() {
 //     my_instance.Get().SomeMethod();  // MyClass::SomeMethod()
 //
@@ -45,6 +45,11 @@
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/thread_restrictions.h"
 
+// LazyInstance uses it's on sruct initializer-list style static initialization,

[M-A Ruel, 2011/11/11 21:34:06]

struct

[Mark Mentovai, 2011/11/11 21:38:08]

Marc-Antoine Ruel wrote:
and its
and own
…

[joth, 2011/11/14 16:58:23]

Done.

[joth, 2011/11/14 16:58:23]

Done.
// LazyInstance uses its own struct initializer-list style static
// initialization, as base's LINKER_INITIALIZED requires a constructor and on
// some compilers (noteably gcc 4.4) this still ends up needing runtime
// initialization.

(note 4.6 -> 4.4)

+// as base's LINKER_INITIALIZED requires a constructor and on some compilers
+// (noteably gcc 4.6) this still ends up needing runtime initialization.
+#define LAZY_INSTANCE_INITIALIZER {0}

[grt (UTC plus 2), 2011/11/12 02:45:17]

Would changing {0} to {} allow you to make the data members of LazyInstance
private again? The end result will still be the same.

[joth, 2011/11/14 16:58:23]

No, all data members must still be public, and AIUI "= {};" is a gcc extension
and isn't portable.

Going in the other direction, I could say "{0, {0}}" which would help ensure
this macro is only used with this class.

[grt (UTC plus 2), 2011/11/14 17:54:25]

Ah, I see my mistake: if the class has non-public members, it isn't an
aggregate.  Since it is an aggregate with the public members, "{}" (which is
portable in this case) can be used just as "{0}" to the same effect (C++-03
8.5.1.8).
Or any other aggregate with similar structure.  Since there are unlikely to be
many, this makes sense as a defensive move.

+
 namespace base {
 
 template <typename Type>
@@ -79,53 +84,42 @@ struct LeakyLazyInstanceTraits {
   }
 };
 
-// We pull out some of the functionality into a non-templated base, so that we
-// can implement the more complicated pieces out of line in the .cc file.
-class BASE_EXPORT LazyInstanceHelper {
- protected:
-  enum {
-    STATE_EMPTY    = 0,
-    STATE_CREATING = 1,
-    STATE_CREATED  = 2
-  };
+// We pull out some of the functionality into a non-templated functions, so we

[grt (UTC plus 2), 2011/11/12 02:45:17]

"into non-templated functions"

[joth, 2011/11/14 16:58:23]

Done.

+// we can implement the more complicated pieces out of line in the .cc file.

[grt (UTC plus 2), 2011/11/12 02:45:17]

"we we" -> "we"

[joth, 2011/11/14 16:58:23]

Done.

+namespace internal {
 
-  explicit LazyInstanceHelper(LinkerInitialized /*unused*/) {/* state_ is 0 */}
+// Our AtomicWord doubles as a spinlock, where a value of
+// kBeingCreatedMarker means the spinlock is being held for creation.
+static const subtle::AtomicWord kLazyInstanceStateCreating = 1;
 
-  // Declaring a destructor (even if it's empty) will cause MSVC to register a
-  // static initializer to register the empty destructor with atexit().
+// Declaring a destructor (even if it's empty) will cause MSVC to register a

[grt (UTC plus 2), 2011/11/12 02:45:17]

This seems out of place.

[joth, 2011/11/14 16:58:23]

Done.

+// static initializer to register the empty destructor with atexit().
 
-  // A destructor is intentionally not defined.  If we were to say
-  // ~LazyInstanceHelper() { }
-  // Even though it's empty, a destructor will still be generated.
-  // In order for the constructor to be called for static variables,
-  // it will be registered as a callback at runtime with AtExit().
-  // We don't want this, so we don't declare a destructor at all,
-  // effectively keeping the type POD (at least in terms of
-  // initialization and destruction).
-
-  // Check if instance needs to be created. If so return true otherwise
-  // if another thread has beat us, wait for instance to be created and
-  // return false.
-  bool NeedsInstance();
+// Check if instance needs to be created. If so return true otherwise
+// if another thread has beat us, wait for instance to be created and
+// return false.
+BASE_EXPORT bool NeedsLazyInstance(base::subtle::AtomicWord* state);
 
-  // After creating an instance, call this to register the dtor to be called
-  // at program exit and to update the state to STATE_CREATED.
-  void CompleteInstance(void* instance, void (*dtor)(void*));
+// After creating an instance, call this to register the dtor to be called
+// at program exit and to update the atomic state to hold the |new_instance|
+BASE_EXPORT void CompleteLazyInstance(base::subtle::AtomicWord* state,
+                                      base::subtle::AtomicWord new_instance,
+                                      void* lazy_instance,
+                                      void (*dtor)(void*));
 
-  base::subtle::Atomic32 state_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(LazyInstanceHelper);
-};
+}  // namespace internal
 
 template <typename Type, typename Traits = DefaultLazyInstanceTraits<Type> >
-class LazyInstance : public LazyInstanceHelper {
+class LazyInstance {
  public:
-  explicit LazyInstance(LinkerInitialized x) : LazyInstanceHelper(x) { }
-
   // Declaring a destructor (even if it's empty) will cause MSVC to register a

[grt (UTC plus 2), 2011/11/12 02:45:17]

I realize you didn't add this comment, but it bugs me so I'll pipe up.  The
comment at the top of the file says "The class was designed to be POD
initialized".  Defining a destructor automatically makes it not a POD, so it
must not have a user-defined destructor if it's designed to be a POD-struct.
MSVC is irrelevant. Would it offend anyone to change this whole comment
(including the paragraph beginning on line 117) to the much simpler:

  // Do not define a destructor, as doing so makes LazyInstance a
non-POD-struct.
  // ~LazyInstance() {}

[joth, 2011/11/14 16:58:23]

Done-ish. I wrote a compromise version, LMKWYT:

  // Do not define a destructor, as doing so makes LazyInstance a
  // non-POD-struct. We don't want that because then a static initializer will
  // be created to register the (empty) destructor with atexit() under MSVC, for
  // example. We handle destruction of the contained Type class explicitly via
  // the OnExit member function, where needed.
  // ~LazyInstance() {}

[grt (UTC plus 2), 2011/11/14 17:54:25]

SGTM

   // static initializer to register the empty destructor with atexit().
-  // Refer to the destructor-related comment in LazyInstanceHelper.
+  // Even though it's empty, a destructor will still be generated.
+  // In order for the constructor to be called for static variables,
+  // it will be registered as a callback at runtime with AtExit().
+  // We don't want this, so we don't declare a destructor at all,
+  // effectively keeping the type POD (at least in terms of
+  // initialization and destruction).
   // ~LazyInstance() {}
 
   Type& Get() {
@@ -140,57 +134,64 @@ class LazyInstance : public LazyInstanceHelper {
 #endif
 
     // We will hopefully have fast access when the instance is already created.
-    // Since a thread sees state_ != STATE_CREATED at most once,
+    // Since a thread sees private_instance_ == 0 or Creating at most once,
     // the load is taken out of NeedsInstance() as a fast-path.
     // The load has acquire memory ordering as a thread which sees
-    // state_ == STATE_CREATED needs to acquire visibility over
-    // the associated data (buf_). Pairing Release_Store is in
+    // private_instance_ > creating needs to acquire visibility over
+    // the associated data (private_buf_). Pairing Release_Store is in
     // CompleteInstance().
-    if ((base::subtle::Acquire_Load(&state_) != STATE_CREATED) &&
-        NeedsInstance()) {
-      // Create the instance in the space provided by |buf_|.
-      instance_ = Traits::New(buf_);
-      CompleteInstance(this, Traits::kRegisterOnExit ? OnExit : NULL);
+    base::subtle::AtomicWord value =
+        base::subtle::Acquire_Load(&private_instance_);
+    if ((value == 0 || value == internal::kLazyInstanceStateCreating) &&
+        internal::NeedsLazyInstance(&private_instance_)) {
+      // Create the instance in the space provided by |private_buf_|.
+      value = reinterpret_cast<base::subtle::AtomicWord>(
+          Traits::New(private_buf_));
+      internal::CompleteLazyInstance(&private_instance_, value, this,
+                                     Traits::kRegisterOnExit ? OnExit : NULL);
     }
 
     // This annotation helps race detectors recognize correct lock-less
     // synchronization between different threads calling Pointer().
     // We suggest dynamic race detection tool that "Traits::New" above
-    // and CompleteInstance(...) happens before "return instance_" below.
+    // and CompleteInstance(...) happens before "return instance()" below.
     // See the corresponding HAPPENS_BEFORE in CompleteInstance(...).
-    ANNOTATE_HAPPENS_AFTER(&state_);
-    return instance_;
+    ANNOTATE_HAPPENS_AFTER(&private_instance_);
+    return instance();
   }
 
   bool operator==(Type* p) {
-    switch (base::subtle::NoBarrier_Load(&state_)) {
-      case STATE_EMPTY:
+    switch (base::subtle::NoBarrier_Load(&private_instance_)) {
+      case 0:
         return p == NULL;
-      case STATE_CREATING:
-        return static_cast<int8*>(static_cast<void*>(p)) == buf_;
-      case STATE_CREATED:
-        return p == instance_;
+      case internal::kLazyInstanceStateCreating:
+        return static_cast<int8*>(static_cast<void*>(p)) == private_buf_;
       default:
-        return false;
+        return p == instance();
     }
   }
 
+  // Effectively private: member data is only public to allow the linker to
+  // statically initialize it. DO NOT USE FROM OUTSIDE THIS CLASS.
+
+  // Note this must use AtomicWord, not Atomic32, to ensure correct alignment
+  // of |private_buf_| on 64 bit architectures. (This member must be first to
+  // allow the syntax used in LAZY_INSTANCE_INITIALIZER to work correctly.)
+  base::subtle::AtomicWord private_instance_;
+  int8 private_buf_[sizeof(Type)];  // Preallocated space for the Type instance.
+
  private:
+  Type* instance() { return reinterpret_cast<Type*>(private_instance_); }
+
   // Adapter function for use with AtExit.  This should be called single
   // threaded, so don't synchronize across threads.
   // Calling OnExit while the instance is in use by other threads is a mistake.
   static void OnExit(void* lazy_instance) {
     LazyInstance<Type, Traits>* me =
         reinterpret_cast<LazyInstance<Type, Traits>*>(lazy_instance);
-    Traits::Delete(me->instance_);
-    me->instance_ = NULL;
-    base::subtle::Release_Store(&me->state_, STATE_EMPTY);
+    Traits::Delete(me->instance());
+    base::subtle::Release_Store(&me->private_instance_, 0);
   }
-
-  Type *instance_;
-  int8 buf_[sizeof(Type)];  // Preallocate the space for the Type instance.
-
-  DISALLOW_COPY_AND_ASSIGN(LazyInstance);
 };
 
 }  // namespace base