Allow linker initialization of lazy instance

Allow linker initialization of lazy instance

Using the initializer list construct = {0} allows the object to be linker initialized.
Modify the LazyInstance class design to make it a pod aggregate type that can be linker initialized this way. Also combines the instance and state members, in line with the Singleton<> class design.
Introduces a new LAZY_INSTANCE_INITIALIZER macro specifically for using to init all lazy instances + modify all existing callsites to use it. (Old code would no longer compile)


BUG=94925
TEST=existing tests pass. http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?history=150&header=chrome-si&graph=chrome-si&rev=-1 should step downward.
TBR=jam@chromium.org,rvargas@chromium.org,darin@chromium.org,ben@chromium.org,apatrick@chromium.org,akalin@chromium.org

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

[joth, 2011-11-11 15:17:08]

thoughts on doing this:
http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h ?

it reduces binary size by 14KB but not as much impact on static initializers as
I expected (removing 40), as it turns out not all LazyInstances were resulting
in a constructor, for reasons I don't fully understand.

[willchan no longer on Chromium, 2011-11-11 18:54:22]

On 2011/11/11 15:17:08, joth wrote:
> thoughts on doing this:
> http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h ?
> 
> it reduces binary size by 14KB but not as much impact on static initializers
as
> I expected (removing 40), as it turns out not all LazyInstances were resulting
> in a constructor, for reasons I don't fully understand.

I don't really like the macro, but if that's the required syntax, then I guess
there's no choice. Also, will this work on other compilers (the eliminating
static initializers part)?

[joth, 2011-11-11 19:06:52]

On 11 November 2011 18:54, <willchan@chromium.org> wrote:

> On 2011/11/11 15:17:08, joth wrote:
>
>> thoughts on doing this:
>>
http://codereview.chromium.**org/8491043/diff/2001/base/**lazy_instance.h<htt...
>>
>
>  it reduces binary size by 14KB but not as much impact on static
>> initializers
>>
> as
>
>> I expected (removing 40), as it turns out not all LazyInstances were
>> resulting
>> in a constructor, for reasons I don't fully understand.
>>
>
> I don't really like the macro, but if that's the required syntax, then I
> guess
> there's no choice.


Yeah... and I like the public members even less, but it's an old trick we
used a decade+ ago with gcc so I know it's fairly established.

I lifted the macro directly from
http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutex_init.html
  pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
although now realize it should be LAZY_INSTANCE_INTIALIZER for
consistency..... that's simple to change though.



> Also, will this work on other compilers (the eliminating
> static initializers part)?
>
> right.... lemme summon @thakis a moment as he's been looking at this area
on Mac.
what's the story with LazyInstance on clang? Do you know if they cause
static initializers? Do you know if this  sort of change will improve or
worsen them?

Cheers!

[Nico, 2011-11-11 19:23:13]

I believe mark@ has an opinion on this.

As far as I remember, the current lazyinstance was designed so that it _already_
doesn't require static initialization as is. gcc had a regression from 4.3 –
4.5(?) where it got this wrong, but it's fixed in 4.6(?) (or 4.5 already?). So
this shouldn't be necessary?

[Mark Mentovai, 2011-11-11 19:40:32]

Yeah, if you move to gcc 4.6, the optimizer takes the stupidity away again.

I don’t think this is an unreasonable solution to fix it for stupid compilers,
though.

[joth, 2011-11-11 19:45:52]

On 11 November 2011 19:40, <mark@chromium.org> wrote:

> Yeah, if you move to gcc 4.6, the optimizer takes the stupidity away again.
>
> I don’t think this is an unreasonable solution to fix it for stupid
> compilers,
> though.
>
>
http://codereview.chromium.**org/8491043/<http://codereview.chromium.org/8491...
>

+digit
So far we've found 4.6 hasn't fixed (on platform we're interested in), and
even if it had it's not clear how quick we'd get all platforms able to move
to 4.6.

It would be nice to get something that works now, and is less likely to
randomly break again in future. My impression given pthreads uses this
pattern plus my prior experience on mobile platforms where static
initializers were out-right disallowed by the toolchain, was this is the
most likely approach to work on any given toolchain.

sounds like you're agreeable to it, if digit can confirms 4.6 hasn't fixed
them then I'll look to move this one forward. LAZY_INSTANCE_INITIALIZR ?

Cheers

[willchan no longer on Chromium, 2011-11-11 19:50:56]

I'm agreeable to this. I'll do a more detailed review shortly.

On Fri, Nov 11, 2011 at 11:45 AM, Jonathan Dixon <joth@chromium.org> wrote:

>
>
> On 11 November 2011 19:40, <mark@chromium.org> wrote:
>
>> Yeah, if you move to gcc 4.6, the optimizer takes the stupidity away
>> again.
>>
>> I don’t think this is an unreasonable solution to fix it for stupid
>> compilers,
>> though.
>>
>>
http://codereview.chromium.**org/8491043/<http://codereview.chromium.org/8491...
>>
>
> +digit
> So far we've found 4.6 hasn't fixed (on platform we're interested in), and
> even if it had it's not clear how quick we'd get all platforms able to move
> to 4.6.
>
> It would be nice to get something that works now, and is less likely to
> randomly break again in future. My impression given pthreads uses this
> pattern plus my prior experience on mobile platforms where static
> initializers were out-right disallowed by the toolchain, was this is the
> most likely approach to work on any given toolchain.
>
> sounds like you're agreeable to it, if digit can confirms 4.6 hasn't fixed
> them then I'll look to move this one forward. LAZY_INSTANCE_INITIALIZR ?
>
> Cheers
>
>

[Mark Mentovai, 2011-11-11 19:51:55]

Jonathan Dixon wrote:
> +digit
> So far we've found 4.6 hasn't fixed (on platform we're interested in), and
> even if it had it's not clear how quick we'd get all platforms able to move
> to 4.6.
> It would be nice to get something that works now, and is less likely to
> randomly break again in future. My impression given pthreads uses this
> pattern plus my prior experience on mobile platforms where static
> initializers were out-right disallowed by the toolchain, was this is the
> most likely approach to work on any given toolchain.
> sounds like you're agreeable to it, if digit can confirms 4.6 hasn't fixed
> them then I'll look to move this one forward. LAZY_INSTANCE_INITIALIZR ?

Yes, I’m fine with this. But please spell “initializer” properly. :)

[Nico, 2011-11-11 20:21:56]

Another nice side effect is that clang's -Wglobal-constructor warning stops
warning about lazy_instance with this change. With this patch, almost everything
by that warning is signal. Beforehand, it was mostly just noise about
lazyinstance.

I'll let willchan do the actual review, below just two minor comments I happened
to notice.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode27
base/lazy_instance.h:27: //   static LazyInstance<MyClass> my_instance =
LINKER_ZERO_INITIALIZED;
Is this needed? Aren't statics implicitly zero initialized? (See the definition
of LINKER_INITIALIZED for why that's there)

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode171
base/lazy_instance.h:171: // Effectively private: member data is only public to
allow the linker to
Sentence cut off

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode173
base/lazy_instance.h:173: // |private_buf_| on 64 bit arcchitectures.
spello arcchitectures

[joth, 2011-11-11 20:43:34]

cool. I'll follow up on specific comments separately, but to this question..

On 11 November 2011 20:21, <thakis@chromium.org> wrote:

> Another nice side effect is that clang's -Wglobal-constructor warning stops
> warning about lazy_instance with this change. With this patch, almost
> everything
> by that warning is signal. Beforehand, it was mostly just noise about
> lazyinstance.
>
> I'll let willchan do the actual review, below just two minor comments I
> happened
> to notice.
>
>
>
http://codereview.chromium.**org/8491043/diff/2001/base/**lazy_instance.h<htt...
> File base/lazy_instance.h (right):
>
> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.h#newcode27<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode27>
> base/lazy_instance.h:27: //   static LazyInstance<MyClass> my_instance =
> LINKER_ZERO_INITIALIZED;
> Is this needed? Aren't statics implicitly zero initialized? (See the
> definition of LINKER_INITIALIZED for why that's there)
>
> AFAIK I think it's mostly useful as a double-check:
- it ensures no implicit zero argument work-doing constructor maybe called.
e.g. if someone declares the wrong type as a static such as a
ThreadLocal<LazyInstanc<bool> > when they meant the opposite.
- if someone declares one as e.g. a class member variable (non-static)
 this annotation will break the compile.

But both these checks relying on them still remembering the =
BLAH_INITIALIZER though. hopefully the code-reviewer will be on the look
out for it at least :)




> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.h#newcode171<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode171>
> base/lazy_instance.h:171: // Effectively private: member data is only
> public to allow the linker to
> Sentence cut off
>
> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.h#newcode173<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode173>
> base/lazy_instance.h:173: // |private_buf_| on 64 bit arcchitectures.
> spello arcchitectures
>
>
http://codereview.chromium.**org/8491043/<http://codereview.chromium.org/8491...
>

[willchan no longer on Chromium, 2011-11-11 21:12:47]

I skimmed it and things look fine. I'm going to wait for your rename before
I do a thorough review.

On Fri, Nov 11, 2011 at 12:43 PM, Jonathan Dixon <joth@chromium.org> wrote:

> cool. I'll follow up on specific comments separately, but to this
> question..
>
> On 11 November 2011 20:21, <thakis@chromium.org> wrote:
>
>> Another nice side effect is that clang's -Wglobal-constructor warning
>> stops
>> warning about lazy_instance with this change. With this patch, almost
>> everything
>> by that warning is signal. Beforehand, it was mostly just noise about
>> lazyinstance.
>>
>> I'll let willchan do the actual review, below just two minor comments I
>> happened
>> to notice.
>>
>>
>>
http://codereview.chromium.**org/8491043/diff/2001/base/**lazy_instance.h<htt...
>> File base/lazy_instance.h (right):
>>
>> http://codereview.chromium.**org/8491043/diff/2001/base/**
>>
lazy_instance.h#newcode27<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode27>
>> base/lazy_instance.h:27: //   static LazyInstance<MyClass> my_instance =
>> LINKER_ZERO_INITIALIZED;
>> Is this needed? Aren't statics implicitly zero initialized? (See the
>> definition of LINKER_INITIALIZED for why that's there)
>>
>> AFAIK I think it's mostly useful as a double-check:
> - it ensures no implicit zero argument work-doing constructor maybe
> called. e.g. if someone declares the wrong type as a static such as a
> ThreadLocal<LazyInstanc<bool> > when they meant the opposite.
> - if someone declares one as e.g. a class member variable (non-static)
>  this annotation will break the compile.
>
> But both these checks relying on them still remembering the =
> BLAH_INITIALIZER though. hopefully the code-reviewer will be on the look
> out for it at least :)
>
>
>
>
>> http://codereview.chromium.**org/8491043/diff/2001/base/**
>>
lazy_instance.h#newcode171<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode171>
>> base/lazy_instance.h:171: // Effectively private: member data is only
>> public to allow the linker to
>> Sentence cut off
>>
>> http://codereview.chromium.**org/8491043/diff/2001/base/**
>>
lazy_instance.h#newcode173<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode173>
>> base/lazy_instance.h:173: // |private_buf_| on 64 bit arcchitectures.
>> spello arcchitectures
>>
>>
http://codereview.chromium.**org/8491043/<http://codereview.chromium.org/8491...
>>
>
>

[joth, 2011-11-11 21:15:36]

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode171
base/lazy_instance.h:171: // Effectively private: member data is only public to
allow the linker to
On 2011/11/11 20:21:56, Nico wrote:
> Sentence cut off

Done.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode173
base/lazy_instance.h:173: // |private_buf_| on 64 bit arcchitectures.
On 2011/11/11 20:21:56, Nico wrote:
> spello arcchitectures

Done.

[joth, 2011-11-11 21:18:35]

I've added in a CC: list. (I'm afraid git cl upload crashes with the cc list the
patch generates so I had to make this one up, hopefully it's near enough)

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

Only nits, I'll defer formal approval to willchan but I'm fine with the code.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.cc
File base/lazy_instance.cc (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.cc#newcode16
base/lazy_instance.cc:16: // TODO(joth): This function could be shared with
Singlton, in place of its
Singleton

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.cc#newcode61
base/lazy_instance.cc:61: 
remove too

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode48
base/lazy_instance.h:48: // LazyInstance uses it's on sruct initializer-list
style static initialization,
struct

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

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode48
base/lazy_instance.h:48: // LazyInstance uses it's on sruct initializer-list
style static initialization,
Marc-Antoine Ruel wrote:
> struct

and its
and own
…

[willchan no longer on Chromium, 2011-11-11 22:18:21]

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.cc
File base/lazy_instance.cc (right):

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.cc#newcode25
base/lazy_instance.cc:25: state, 0, internal::kLazyInstanceStateCreating) == 0)
Already within internal namespace, no need for internal::.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode134
base/lazy_instance.h:134: // Since a thread sees private_instance_ == 0 or
Creating at most once,
You mean kLazyInstanceStateCreating instead of just Creating, right?

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode140
base/lazy_instance.h:140: base::subtle::AtomicWord value =
No need to use base:: from within the base namespace.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode142
base/lazy_instance.h:142: if ((value == 0 || value ==
internal::kLazyInstanceStateCreating) &&
I'm not sure I understand why we changed away from using the state machine.
We're doing two compares here now (0 and kLazyInstanceStateCreating) rather than
a single != STATE_CREATED. Why? Are you making a space/time tradeoff here,
combining the state_ and instance_ variables into a single private_instance_
variable?

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

drive-by.  you have my LGTM either way.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode51
base/lazy_instance.h:51: #define LAZY_INSTANCE_INITIALIZER {0}
Would changing {0} to {} allow you to make the data members of LazyInstance
private again? The end result will still be the same.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode87
base/lazy_instance.h:87: // We pull out some of the functionality into a
non-templated functions, so we
"into non-templated functions"

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode88
base/lazy_instance.h:88: // we can implement the more complicated pieces out of
line in the .cc file.
"we we" -> "we"

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode95
base/lazy_instance.h:95: // Declaring a destructor (even if it's empty) will
cause MSVC to register a
This seems out of place.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode115
base/lazy_instance.h:115: // Declaring a destructor (even if it's empty) will
cause MSVC to register a
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-12 17:44:14]

Thanks, will look at the specific changes Monday.

On 11 November 2011 22:18, <willchan@chromium.org> wrote:

>
>
http://codereview.chromium.**org/8491043/diff/2001/base/**lazy_instance.cc<ht...
> File base/lazy_instance.cc (right):
>
> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.cc#newcode25<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.cc#newcode25>
> base/lazy_instance.cc:25: state, 0,
> internal::**kLazyInstanceStateCreating) == 0)
> Already within internal namespace, no need for internal::.
>
>
>
http://codereview.chromium.**org/8491043/diff/2001/base/**lazy_instance.h<htt...
> File base/lazy_instance.h (right):
>
> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.h#newcode134<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode134>
> base/lazy_instance.h:134: // Since a thread sees private_instance_ == 0
> or Creating at most once,
> You mean kLazyInstanceStateCreating instead of just Creating, right?
>
> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.h#newcode140<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode140>
> base/lazy_instance.h:140: base::subtle::AtomicWord value =
> No need to use base:: from within the base namespace.
>
> http://codereview.chromium.**org/8491043/diff/2001/base/**
>
lazy_instance.h#newcode142<http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode142>
> base/lazy_instance.h:142: if ((value == 0 || value ==
> internal::**kLazyInstanceStateCreating) &&
> I'm not sure I understand why we changed away from using the state
> machine. We're doing two compares here now (0 and
> kLazyInstanceStateCreating) rather than a single != STATE_CREATED. Why?
> Are you making a space/time tradeoff here, combining the state_ and
> instance_ variables into a single private_instance_ variable?
>
>
Yeah... the aim was to simplify things by sharing the same design with
Singleton,and also it reduces the overhead from two words to 1. I could
still do this with a single compare by doing "if (value >> 1)" or "if
(value & ~kLazyInstanceStateCreating)" but that is rather obscure (and
relies on looking the creating state to value 1 -- not a big deal)
Or I can switch back to the old state + instance members. This is not a
critical part of the change.

Cheers

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

Thanks, PTAL.
I've addressed all open comments except the state & instance pointer being
combined into a single shared AtomicWord - let me know if you'd rather I left
them as separate entities.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.cc
File base/lazy_instance.cc (right):

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.cc#newcode25
base/lazy_instance.cc:25: state, 0, internal::kLazyInstanceStateCreating) == 0)
On 2011/11/11 22:18:21, willchan wrote:
> Already within internal namespace, no need for internal::.

Done.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode27
base/lazy_instance.h:27: //   static LazyInstance<MyClass> my_instance =
LINKER_ZERO_INITIALIZED;
On 2011/11/11 20:21:56, Nico wrote:
> Is this needed? Aren't statics implicitly zero initialized? (See the
definition
> of LINKER_INITIALIZED for why that's there)

Just needed as a double check to prove to the caller the instance really is POD
initializer-list initializable

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode134
base/lazy_instance.h:134: // Since a thread sees private_instance_ == 0 or
Creating at most once,
On 2011/11/11 22:18:21, willchan wrote:
> You mean kLazyInstanceStateCreating instead of just Creating, right?

Done.

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode140
base/lazy_instance.h:140: base::subtle::AtomicWord value =
On 2011/11/11 22:18:21, willchan wrote:
> No need to use base:: from within the base namespace.

Done. here & many other places in .h and .cc

http://codereview.chromium.org/8491043/diff/2001/base/lazy_instance.h#newcode142
base/lazy_instance.h:142: if ((value == 0 || value ==
internal::kLazyInstanceStateCreating) &&
On 2011/11/11 22:18:21, willchan wrote:
> I'm not sure I understand why we changed away from using the state machine.
> We're doing two compares here now (0 and kLazyInstanceStateCreating) rather
than
> a single != STATE_CREATED. Why? Are you making a space/time tradeoff here,
> combining the state_ and instance_ variables into a single private_instance_
> variable?

Yep memory reduction plus consistency with Singleton.
but I remove the double comparison (by adding a bitwise and).

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.cc
File base/lazy_instance.cc (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.cc#newcode16
base/lazy_instance.cc:16: // TODO(joth): This function could be shared with
Singlton, in place of its
On 2011/11/11 21:34:06, Marc-Antoine Ruel wrote:
> Singleton

Done.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.cc#newcode61
base/lazy_instance.cc:61: 
On 2011/11/11 21:34:06, Marc-Antoine Ruel wrote:
> remove too

Done.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode48
base/lazy_instance.h:48: // LazyInstance uses it's on sruct initializer-list
style static initialization,
On 2011/11/11 21:34:06, Marc-Antoine Ruel wrote:
> struct

Done.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode48
base/lazy_instance.h:48: // LazyInstance uses it's on sruct initializer-list
style static initialization,
On 2011/11/11 21:38:08, Mark Mentovai wrote:
> Marc-Antoine Ruel wrote:
> > struct
> 
> and its
> and own
> …

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)

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode51
base/lazy_instance.h:51: #define LAZY_INSTANCE_INITIALIZER {0}
On 2011/11/12 02:45:17, grt wrote:
> Would changing {0} to {} allow you to make the data members of LazyInstance
> private again? The end result will still be the same.

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.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode87
base/lazy_instance.h:87: // We pull out some of the functionality into a
non-templated functions, so we
On 2011/11/12 02:45:17, grt wrote:
> "into non-templated functions"

Done.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode88
base/lazy_instance.h:88: // we can implement the more complicated pieces out of
line in the .cc file.
On 2011/11/12 02:45:17, grt wrote:
> "we we" -> "we"

Done.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode95
base/lazy_instance.h:95: // Declaring a destructor (even if it's empty) will
cause MSVC to register a
On 2011/11/12 02:45:17, grt wrote:
> This seems out of place.

Done.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode115
base/lazy_instance.h:115: // Declaring a destructor (even if it's empty) will
cause MSVC to register a
On 2011/11/12 02:45:17, grt wrote:
> 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() {}

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() {}

[willchan no longer on Chromium, 2011-11-14 17:29:06]

I'm fine with your explanation of your design choices. Please update the
changelist description though. LGTM.

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.cc
File base/lazy_instance.cc (right):

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.cc#newcode33
base/lazy_instance.cc:33: // CompleteInstance().
s/CompleteInstance/CompleteLazyInstance/g

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode50
base/lazy_instance.h:50: // some compilers (noteably gcc 4.4) this still ends up
needing runtime
notably

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode98
base/lazy_instance.h:98: static const subtle::AtomicWord
kLazyInstanceCreatedMask =
This probably can be scoped to the function, right?

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode142
base/lazy_instance.h:142: // CompleteInstance().
s/CompleteInstance/CompleteLazyInstance/

[grt (UTC plus 2), 2011-11-14 17:54:24]

still LGTM on my part.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode51
base/lazy_instance.h:51: #define LAZY_INSTANCE_INITIALIZER {0}
On 2011/11/14 16:58:23, joth wrote:
> On 2011/11/12 02:45:17, grt wrote:
> > Would changing {0} to {} allow you to make the data members of LazyInstance
> > private again? The end result will still be the same.
> 
> No, all data members must still be public, and AIUI "= {};" is a gcc extension
> and isn't portable.

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).

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

Or any other aggregate with similar structure.  Since there are unlikely to be
many, this makes sense as a defensive move.

http://codereview.chromium.org/8491043/diff/4003/base/lazy_instance.h#newcode115
base/lazy_instance.h:115: // Declaring a destructor (even if it's empty) will
cause MSVC to register a
On 2011/11/14 16:58:23, joth wrote:
> On 2011/11/12 02:45:17, grt wrote:
> > 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() {}
> 
> 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() {}

SGTM

[joth, 2011-11-15 11:40:40]

+OWNERS

Thanks will, I plan to submit this once the try servers are green with TBR on
the OWNERS (I'm replacing (base::LINKER_INTITIALIZED) with "=
LAZY_INSTANCE_INITIALIZER" on your lazy instances.)

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.cc
File base/lazy_instance.cc (right):

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.cc#newcode33
base/lazy_instance.cc:33: // CompleteInstance().
On 2011/11/14 17:29:06, willchan wrote:
> s/CompleteInstance/CompleteLazyInstance/g

Done.

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode50
base/lazy_instance.h:50: // some compilers (noteably gcc 4.4) this still ends up
needing runtime
On 2011/11/14 17:29:06, willchan wrote:
> notably

Done.

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode98
base/lazy_instance.h:98: static const subtle::AtomicWord
kLazyInstanceCreatedMask =
On 2011/11/14 17:29:06, willchan wrote:
> This probably can be scoped to the function, right?

Done.

http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode142
base/lazy_instance.h:142: // CompleteInstance().
On 2011/11/14 17:29:06, willchan wrote:
> s/CompleteInstance/CompleteLazyInstance/

Done.

[joth, 2011-11-15 14:04:15]

+ kerz. I just landed, and early signs are promising:
http://build.chromium.org/p/chromium/builders/Linux/builds/17188

PERF_IMPROVE: chrome-si/initializers (40.74%)

http://build.chromium.org/p/chromium/builders/Linux/builds/17187/steps/sizes/...
http://build.chromium.org/p/chromium/builders/Linux/builds/17188/steps/sizes/...

from 282 down to 176 = 106 fewer static initializers.



On 15 November 2011 11:40, <joth@chromium.org> wrote:

> +OWNERS
>
> Thanks will, I plan to submit this once the try servers are green with TBR
> on
> the OWNERS (I'm replacing (base::LINKER_INTITIALIZED) with "=
> LAZY_INSTANCE_INITIALIZER" on your lazy instances.)
>
>
>
>
>
http://codereview.chromium.**org/8491043/diff/9001/base/**lazy_instance.cc<ht...
> File base/lazy_instance.cc (right):
>
> http://codereview.chromium.**org/8491043/diff/9001/base/**
>
lazy_instance.cc#newcode33<http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.cc#newcode33>
> base/lazy_instance.cc:33: // CompleteInstance().
> On 2011/11/14 17:29:06, willchan wrote:
>
>> s/CompleteInstance/**CompleteLazyInstance/g
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/8491043/diff/9001/base/**lazy_instance.h<htt...
> File base/lazy_instance.h (right):
>
> http://codereview.chromium.**org/8491043/diff/9001/base/**
>
lazy_instance.h#newcode50<http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode50>
> base/lazy_instance.h:50: // some compilers (noteably gcc 4.4) this still
> ends up needing runtime
> On 2011/11/14 17:29:06, willchan wrote:
>
>> notably
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8491043/diff/9001/base/**
>
lazy_instance.h#newcode98<http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode98>
> base/lazy_instance.h:98: static const subtle::AtomicWord
> kLazyInstanceCreatedMask =
> On 2011/11/14 17:29:06, willchan wrote:
>
>> This probably can be scoped to the function, right?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8491043/diff/9001/base/**
>
lazy_instance.h#newcode142<http://codereview.chromium.org/8491043/diff/9001/base/lazy_instance.h#newcode142>
> base/lazy_instance.h:142: // CompleteInstance().
> On 2011/11/14 17:29:06, willchan wrote:
>
>> s/CompleteInstance/**CompleteLazyInstance/
>>
>
> Done.
>
>
http://codereview.chromium.**org/8491043/<http://codereview.chromium.org/8491...
>

[Mark Mentovai, 2011-11-15 14:26:06]

LGTM for base OWNERS review

(except I hate when people write “we” or ”our” in comments.)

[M-A Ruel, 2011-11-15 14:31:34]

On 2011/11/15 14:26:06, Mark Mentovai wrote:
> (except I hate when people write “we” or ”our” in comments.)

I also hate "I" in comments but "I" do it all the time, it's compulsive. :/

[Mark Mentovai, 2011-11-15 14:32:40]

maruel@chromium.org wrote:
> I also hate "I" in comments but "I" do it all the time, it's compulsive. :/

If you find a way to break the habit, I’ll find a way to commend you. :)

[joth, 2011-11-15 14:41:27]

On 15 November 2011 14:26, <mark@chromium.org> wrote:

> LGTM for base OWNERS review
>
> Thanks! (I'd also taken willchan's as covering base, sorry I didn't call
areas out specifically)


> (except I hate when people write “we” or ”our” in comments.)
>
> Yes, we do generally try and avoid that when we write original comments --
in our defense, "Our AtomicWord doubles as a spinlock" etc were lifted from
Singleton.