Create a LazyInstance abstraction.

Create a LazyInstance abstraction for avoiding static constructors by lazily creating an instance of an object on first access.  This is like Singleton, but without the Singleton property of sharing instances.  This also preallocates space for the object to avoid the heap to try to help fragmentation and creation performance.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=1837

[Dean McNamee, 2008-09-07 14:51:49]

I think in a lot of situations we're abusing Singleton, when really we don't
want Singleton sharing, we just wanted a lazily constructed instance.  I think
this abstraction is much clearer, and should replace some of our uses of
Singleton (but not all).  For example, any Singleton using a DifferentiatingType
should probably instead be a LazyInstance.

Darin had concerns about Singleton increasing the likely hood of heap
fragmentation, by having lately allocated permanent objects that make be
creating "stakes" in the heap.  I preallocated the space for the instance, which
should void this concern, and also lead to faster construction of the instance.

[darin (slow to review), 2008-09-07 18:48:19]

LGTM

http://codereview.chromium.org/1608/diff/1/2
File base/lazy_instance.h (right):

http://codereview.chromium.org/1608/diff/1/2#newcode12
Line 12: // LazyInstance is completely thread safe, assuming that you create it
safetly.
nit: safetly -> safely

http://codereview.chromium.org/1608/diff/1/2#newcode49
Line 49: // We can do anything in the destructor so we can avoid a static
initializer.
nit: I'm having trouble parsing this sentence.  Did you mean "We can't do
anything..." instead?

http://codereview.chromium.org/1608/diff/1/2#newcode56
Line 56: Type* Pointer() {
Q: any thoughts on how to factor this method into portions that depend on Type
and portions that do not?  it semes like there is enough code here that it would
be nice to move some of it into a shared non-template helper function if
possible.

perhaps you could have a non-template LazyInstance_Helper class that you extend
from.  that class could have the state_ variable and enumeration.  that would
then allow you to write a non-template method that does everything here except
call "new (instance) Type()", which could probably be safely deferred until
after you call RegisterCallback.

anyways, just an idea... feel free to punt on this kind of code saving
optimization if you think it'll be too complicated.

[darin (slow to review), 2008-09-07 18:48:50]

btw... couldn't Singleton<T> be re-implemented in terms of LazyInstance<T>? 
Perhaps a Singleton<T> could just have a static member of type LazyInstance<T>?

[cpu_(ooo_6.6-7.5), 2008-09-07 20:05:32]

http://codereview.chromium.org/1608/diff/1/2
File base/lazy_instance.h (right):

http://codereview.chromium.org/1608/diff/1/2#newcode20
Line 20: // to avoid allocating the Type instance on the heap.  This may help
with the
I am curious to know how does it cope when instanciated with incomplete types.

http://codereview.chromium.org/1608/diff/1/2#newcode46
Line 46: LazyInstance() : state_(0) { }
nit STATE_EMPTY is better than zero

http://codereview.chromium.org/1608/diff/1/2#newcode47
Line 47: explicit LazyInstance(LinkerInitialized x) { /* state_ is 0 */ }
How about const LinkerInitialized& x  instead?

http://codereview.chromium.org/1608/diff/1/2#newcode80
Line 80: while (true) {
You are duplicating what Lock does better, is that because you need a trivial
ctor? I got lost here

[Mark Mentovai, 2008-09-07 23:26:27]

Don't let the comments scare you, this looks good.

http://codereview.chromium.org/1608/diff/1/2
File base/lazy_instance.h (right):

http://codereview.chromium.org/1608/diff/1/2#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Nit: legal sez: 2008 on brand-new files.

http://codereview.chromium.org/1608/diff/1/2#newcode17
Line 17: // LazyInstance is similar to Singleton, except it does not have the
Singleton
nit: lowercase s on the second Singleton, since you don't mean to use it by
proper name there.

http://codereview.chromium.org/1608/diff/1/2#newcode18
Line 18: // property.  You can have multiple LazyInstance's of the same type,
and each
nit: one other difference from Singleton is that this doesn't have the traits. 
You might want to mention that too.

Or not.

http://codereview.chromium.org/1608/diff/1/2#newcode20
Line 20: // to avoid allocating the Type instance on the heap.  This may help
with the
cpu wrote:
> I am curious to know how does it cope when instanciated with incomplete types.

It doesn't!  Maybe worth a comment.

http://codereview.chromium.org/1608/diff/1/2#newcode21
Line 21: // performance of creating the instance, and reducing heap
fragmentation.
Can you also document how destruction is handled somewhere up here?

http://codereview.chromium.org/1608/diff/1/2#newcode24
Line 24: //   // Safe to declare as a global variable.  NOT safe as a
block-level static.
Say: "Not safe as a block-level static, use the no-argument constructor
instead."

http://codereview.chromium.org/1608/diff/1/2#newcode25
Line 25: //   static LazyInstance<MyClass> my_class(base::LINKER_INITALIZED);
Clearer if the demo object is named my_object instead of my_class.

http://codereview.chromium.org/1608/diff/1/2#newcode31
Line 31: //   }
Give an example where LazyInstance is declared in some function scope.  It's
important to point out that it must be declared as static LazyInstance<T>.  It
will be completely screwed up if someone tries to use a LazyInstance<T> as a
simple local variable.  It might not seem like people would actually try that,
but someone familiar with Singleton, without reading LazyInstance, might just
write (no static) LazyInstance<T> and get an unshared instance with all the
race-prevention overhead.

If you do this, you should probably also say that a nonstatic LazyInstance can
make sense as a member variable.

http://codereview.chromium.org/1608/diff/1/2#newcode47
Line 47: explicit LazyInstance(LinkerInitialized x) { /* state_ is 0 */ }
cpu wrote:
> How about const LinkerInitialized& x  instead?

Please don't do that, just go with the snippet advertised by the comment in
basictypes.h.  The code snippet and its attached comment in basictypes.h comes
from upstream, we should just do what it advises unless we want to push a const&
change upstream.

Leaving this as enum-by-value can never be worse, and can only be faster in some
cases, such as LP64 and LLP64.

http://codereview.chromium.org/1608/diff/1/2#newcode49
Line 49: // We can do anything in the destructor so we can avoid a static
initializer.
Darin wrote:
> nit: I'm having trouble parsing this sentence.  Did you mean "We can't do
> anything..." instead?

Sentence better as something like "LinkerInitialized requires zero-fill instead
of static initialization code, which will only be true if the destructor doesn't
touch the object's storage."  That's more accurate.

http://codereview.chromium.org/1608/diff/1/2#newcode68
Line 68: new (instance) Type();
Awesome, nice job.

http://codereview.chromium.org/1608/diff/1/2#newcode80
Line 80: while (true) {
cpu wrote:
> You are duplicating what Lock does better, is that because you need a trivial
> ctor? I got lost here

I understand what's happening here, and the same thing happens in Singleton.

Doing our own spin is kind of sucky, can we wait on a condition variable?

http://codereview.chromium.org/1608/diff/1/2#newcode98
Line 98: enum {
nit: enums before methods

http://codereview.chromium.org/1608/diff/1/2#newcode99
Line 99: STATE_EMPTY    = 0,
nit: bros before hos

(sorry)

http://codereview.chromium.org/1608/diff/1/2#newcode106
Line 106: DISALLOW_COPY_AND_ASSIGN(LazyInstance<Type>);
This can just be LazyInstance, not LazyInstance<Type>, right?

http://codereview.chromium.org/1608/diff/1/6
File base/lazy_instance_unittest.cc (right):

http://codereview.chromium.org/1608/diff/1/6#newcode23
Line 23: destructed = true;
nit: destroyed

http://codereview.chromium.org/1608/diff/1/6#newcode26
Line 26: static bool constructed;
nit: _-suffix.  Same with SlowConstructor::constructed.

http://codereview.chromium.org/1608/diff/1/6#newcode28
Line 28: static void ClearState() {
nit: functions before variables

http://codereview.chromium.org/1608/diff/1/6#newcode43
Line 43: int some_int() { return some_int_; }
nit: const.  I don't really care in this test code, but it's a good habit.

http://codereview.chromium.org/1608/diff/1/6#newcode62
Line 62: int SlowConstructor::constructed = 0;
This should be right after the SlowConstructor class.

http://codereview.chromium.org/1608/diff/1/6#newcode71
Line 71: base::LazyInstance<ConstructAndDestructLogger> logger;
Like I mentioned in the other file, a function-scope LazyInstance<> that's not
static is kind of a stupid use case.  Since people will look to the test code as
an example, you should make this static.

http://codereview.chromium.org/1608/diff/1/6#newcode80
Line 80: EXPECT_TRUE(ConstructAndDestructLogger::constructed);
I'm not sure that this is the best test because these ConstructAndDestructLogger
data members are static.  You've got no idea at this point whether the call to
Pointer gave you back the same instance that Get did.  You can check for that
explicitly or you can just make these members nonstatic.  I'd prefer the latter.
 To check for proper destruction, I'd recommend having the
ConstructAndDestructLogger constructor and destructor do
AtomicIncrement/AtomicDecrement of a static "live object count", which you can
test == 0 before lazy construction and after AtExitManager cleanup.

http://codereview.chromium.org/1608/diff/1/6#newcode85
Line 85: 
I am a blank line for no reason.

http://codereview.chromium.org/1608/diff/1/6#newcode92
Line 92: base::LazyInstance<SlowConstructor> lazy;
static as before.

http://codereview.chromium.org/1608/diff/1/6#newcode104
Line 104: }
Can you add a test for a linker-initialized LazyInstance<> too?  You should be
able to test for both construction and destruction with a
ShadowingAtExitManager.

To cover all the bases if you do that, maybe also add a test with a nonstatic
member LazyInstance<> owned by some object.

It should be fine to reuse ConstructAndDestructLogger for these.  Maybe even
just make them be extra scopes in the Basic test.

[M-A Ruel, 2008-09-08 01:01:46]

Not much left to say with Darin and Mark comments. :)

lgtm

http://codereview.chromium.org/1608/diff/1/3
File base/SConscript (right):

http://codereview.chromium.org/1608/diff/1/3#newcode43
Line 43: 'lazy_instance.cc',
lazy_instance.cc doesn't exist.

http://codereview.chromium.org/1608/diff/1/2
File base/lazy_instance.h (right):

http://codereview.chromium.org/1608/diff/1/2#newcode80
Line 80: while (true) {
CV implies a constructor. Dean, keep it as is.
And am I alone to prefer for (;;) ? I guess so. :)

[M-A Ruel, 2008-09-08 13:56:35]

still lgtm

http://codereview.chromium.org/1608/diff/272/38
File base/lazy_instance.h (right):

http://codereview.chromium.org/1608/diff/272/38#newcode98
Line 98: int8 buf_[sizeof(Type)];  // Preallocate the space for the Type
instance.
I'm just sad about objects requiring 8 bytes alignment.

[Mark Mentovai, 2008-09-08 13:56:50]

I'm satisfied now, LGTM

[darin (slow to review), 2008-09-08 16:13:00]

LGTM