Add CallOnce() and simple LazyInstance implementation.

Add CallOnce() and simple LazyInstance implementation.

The next CL that will remove static initializers depends on this change.

A few notes:
- callback.h comes from Protocol Buffers.
- This implementation of LazyInstance does not handle global destructors (i.e. the lazy instances are never deleted). Let me know if this is an issue.
- A CL adding a similar implementation of CallOnce() (based on a patch sent by Digit) will be sent for review to the Protobuf team very soon.



BUG=1859

[dvyukov, 2012-02-22 14:07:35]

http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h
File src/lazy-instance.h (right):

http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h#newcode95
src/lazy-instance.h:95: inline T* MutableInstance() const {
inline's are excessive here and below

http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h#newcode123
src/lazy-instance.h:123: mutable char instance_storage[sizeof(T)];
This will break on ARM. The type must be uintptr_t at the very least, uint64_t
is better, max_align_t is even better.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc
File src/once.cc (right):

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode55
src/once.cc:55: // main memory at this point (especially the assignment of
'already_done').
That is incorrect. Unlock() do provide the barrier, but it is in the wrong
place, you need the barrier *before* store to already_done, not *after*.
What currently prevents the code be reordered as:
    once->already_done = true;
    closure->Run();
?
You need to use Release_Store() to set the flag.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode71
src/once.cc:71: STATE_DESTROYED = 3
remove, it is not intended to be destroyed at all

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode75
src/once.cc:75: Atomic32 state = Acquire_Load(&state_);
The implementation can be simplified considerably. You implement lazy
initialization and then use that lazy initialization to implement another lazy
initialization. You may just move this LazyMutex::CreateOnce() code to 
LockAndExecuteClosureOnce() and eliminate mutexes and associated problems
entirely.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode88
src/once.cc:88: state =
InterlockedCompareExchange(reinterpret_cast<LONG*>(&state_),
Is there the Barrier_CompareAndSwap()? It would be more consistent.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode95
src/once.cc:95: state = LazyMutex::STATE_CREATED;
remove

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode99
src/once.cc:99: // TODO(pliard): Use an event to wait.
Most likely it will never be required. The wait is one-time and short. I would
remove the TODO.

http://codereview.appspot.com/5687064/diff/4004/src/once.h
File src/once.h (right):

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode92
src/once.h:92: uint32_t storage[8];
s/uint32_t/uintptr_t/
alignment

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode100
src/once.h:100: #define V8_ONCE_INIT /* Nothing (Zeros are fine) */
I think it's better to be "{}" and then V8_DECLARE_ONCE can be
platform-independent.

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode106
src/once.h:106: #if !defined(PTHREAD_MUTEX_INITIALIZER) &&
defined(PTHREAD_MUTEX_INITIALIZER_NP)
What is it required for? PTHREAD_MUTEX_INITIALIZER is POSIX.

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode121
src/once.h:121: if (!Acquire_Load(&once->already_done)) {
Acquire_Load() w/o a pairing Release_Store() is always senseless.

[pliard1, 2012-02-22 18:17:36]

Hi Dmitry,

Thanks for reviewing this CL!

The CallOnce() implementation was significantly simplified thanks to your
comments.

About LazyInstance<>, I had to make it a bit more flexible (i.e. complicated) to
cover all the needs of the v8 codebase.
One of these needs is the lazy initialization of this kind of global variable:

Mutex* my_mutex = OS::Mutex();

In that case, we don't want LazyInstance to statically allocate an instance of
Mutex (which is abstract anyway) for us.

http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h
File src/lazy-instance.h (right):

http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h#newcode95
src/lazy-instance.h:95: inline T* MutableInstance() const {
On 2012/02/22 14:07:35, dvyukov wrote:
> inline's are excessive here and below

Done.

http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h#newcode123
src/lazy-instance.h:123: mutable char instance_storage[sizeof(T)];
On 2012/02/22 14:07:35, dvyukov wrote:
> This will break on ARM. The type must be uintptr_t at the very least, uint64_t
> is better, max_align_t is even better.

I rely now on the previous field (OnceType => AtomicWord) to make sure that the
field is correctly aligned. Chromium's lazy_instance.h relies on that as well.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc
File src/once.cc (right):

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode55
src/once.cc:55: // main memory at this point (especially the assignment of
'already_done').
On 2012/02/22 14:07:35, dvyukov wrote:
> That is incorrect. Unlock() do provide the barrier, but it is in the wrong
> place, you need the barrier *before* store to already_done, not *after*.
> What currently prevents the code be reordered as:
>     once->already_done = true;
>     closure->Run();
> ?
> You need to use Release_Store() to set the flag.

Done.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode71
src/once.cc:71: STATE_DESTROYED = 3
On 2012/02/22 14:07:35, dvyukov wrote:
> remove, it is not intended to be destroyed at all

Done.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode75
src/once.cc:75: Atomic32 state = Acquire_Load(&state_);
On 2012/02/22 14:07:35, dvyukov wrote:
> The implementation can be simplified considerably. You implement lazy
> initialization and then use that lazy initialization to implement another lazy
> initialization. You may just move this LazyMutex::CreateOnce() code to 
> LockAndExecuteClosureOnce() and eliminate mutexes and associated problems
> entirely.

Indeed!

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode88
src/once.cc:88: state =
InterlockedCompareExchange(reinterpret_cast<LONG*>(&state_),
On 2012/02/22 14:07:35, dvyukov wrote:
> Is there the Barrier_CompareAndSwap()? It would be more consistent.

Done.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode95
src/once.cc:95: state = LazyMutex::STATE_CREATED;
On 2012/02/22 14:07:35, dvyukov wrote:
> remove

Done.

http://codereview.appspot.com/5687064/diff/4004/src/once.cc#newcode99
src/once.cc:99: // TODO(pliard): Use an event to wait.
On 2012/02/22 14:07:35, dvyukov wrote:
> Most likely it will never be required. The wait is one-time and short. I would
> remove the TODO.

Done.

http://codereview.appspot.com/5687064/diff/4004/src/once.h
File src/once.h (right):

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode92
src/once.h:92: uint32_t storage[8];
On 2012/02/22 14:07:35, dvyukov wrote:
> s/uint32_t/uintptr_t/
> alignment

I removed all the fields and made OnceType an AtomicWord.

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode100
src/once.h:100: #define V8_ONCE_INIT /* Nothing (Zeros are fine) */
On 2012/02/22 14:07:35, dvyukov wrote:
> I think it's better to be "{}" and then V8_DECLARE_ONCE can be
> platform-independent.

Done.

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode106
src/once.h:106: #if !defined(PTHREAD_MUTEX_INITIALIZER) &&
defined(PTHREAD_MUTEX_INITIALIZER_NP)
On 2012/02/22 14:07:35, dvyukov wrote:
> What is it required for? PTHREAD_MUTEX_INITIALIZER is POSIX.

PTHREAD_MUTEX_INITIALIZER_NP was defined instead of PTHREAD_MUTEX_INITIALIZER on
some platforms.

Anyway I don't use any mutex anymore thanks to your comment in the cc file.

http://codereview.appspot.com/5687064/diff/4004/src/once.h#newcode121
src/once.h:121: if (!Acquire_Load(&once->already_done)) {
On 2012/02/22 14:07:35, dvyukov wrote:
> Acquire_Load() w/o a pairing Release_Store() is always senseless.

Done.

[jbates, 2012-02-23 02:17:41]

FYI, I'm working on an alignment fix for LazyInstance in chromium
(http://codereview.chromium.org/9186057/). As is, if the LazyInstance contains
SSE vectors, they may not be 16-byte aligned so you couldn't use the higher
performance aligned SSE intrinsics.

[pliard, 2012-02-23 08:41:39]

That is interesting! I will pull your AlignedMemory<> implementation (once
it is submitted) in a next CL.

Thanks

On Thu, Feb 23, 2012 at 3:17 AM, <jbates@chromium.org> wrote:

> FYI, I'm working on an alignment fix for LazyInstance in chromium
>
(http://codereview.chromium.**org/9186057/<http://codereview.chromium.org/9186...).
> As is, if the LazyInstance
> contains SSE vectors, they may not be 16-byte aligned so you couldn't
> use the higher performance aligned SSE intrinsics.
>
>
http://codereview.appspot.com/**5687064/<http://codereview.appspot.com/5687064/>
>

[dvyukov, 2012-02-23 09:06:03]

Much better now
There is a minor issue with memory barriers, LGTM other than that

http://codereview.appspot.com/5687064/diff/7008/src/lazy-instance.h
File src/lazy-instance.h (right):

http://codereview.appspot.com/5687064/diff/7008/src/lazy-instance.h#newcode177
src/lazy-instance.h:177: mutable StorageType storage_;
I think it will serve us until you pull AlignedMemory<>.

http://codereview.appspot.com/5687064/diff/7008/src/once.cc
File src/once.cc (right):

http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode61
src/once.cc:61: state = NoBarrier_CompareAndSwap(
Remove the MemoryBarrier() and make this Acquire_CompareAndSwap(). Here you need
to acquire visibility over the object if CAS returns STATE_DONE. Just as you use
Acquire_Load() on line 77. Both the CAS and the Load pair with the
Release_Store() on line 67.

[pliard1, 2012-02-23 09:27:21]

http://codereview.appspot.com/5687064/diff/7008/src/lazy-instance.h
File src/lazy-instance.h (right):

http://codereview.appspot.com/5687064/diff/7008/src/lazy-instance.h#newcode177
src/lazy-instance.h:177: mutable StorageType storage_;
On 2012/02/23 09:06:03, dvyukov wrote:
> I think it will serve us until you pull AlignedMemory<>.

Yes that should be fine (until AlignedMemory<>).

http://codereview.appspot.com/5687064/diff/7008/src/once.cc
File src/once.cc (right):

http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode61
src/once.cc:61: state = NoBarrier_CompareAndSwap(
On 2012/02/23 09:06:03, dvyukov wrote:
> Remove the MemoryBarrier() and make this Acquire_CompareAndSwap(). Here you
need
> to acquire visibility over the object if CAS returns STATE_DONE. Just as you
use
> Acquire_Load() on line 77. Both the CAS and the Load pair with the
> Release_Store() on line 67.

Done. Thanks for the explanation.

[fschneider, 2012-02-23 12:26:44]

Please make sure that the base URL of your changes in the future always points
to the bleeding_edge branch
(https://v8.googlecode.com/svn/branches/bleeding_edge) and not to trunk.

To me it would seems cleaner to reduce callback.h to the minimum required for
the purpose of static initialization.

http://codereview.appspot.com/5687064/diff/7008/src/once.cc
File src/once.cc (right):

http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode41
src/once.cc:41: STATE_INITIALIZED = 0,
Maybe STATE_UNINITIALIZED would be clearer about what it means.

http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode45
src/once.cc:45: 
Two \n here.

http://codereview.appspot.com/5687064/diff/7008/src/once.h
File src/once.h (right):

http://codereview.appspot.com/5687064/diff/7008/src/once.h#newcode75
src/once.h:75: #ifndef V8_STUBS_ONCE_H_
This should be V8_ONCE_H_ for once.h per convention.

http://codereview.appspot.com/5687064/diff/7008/src/once.h#newcode95
src/once.h:95: FunctionClosure0 func(init_func, false);
If you only ever need stack-allocated instances of closures, the class could be
simplified and changed  to forbid heap-allocation (in V8 we use the
BASE_EMBEDDED macro for this)

You could then probably get rid of the self_deleting flag, too.

http://codereview.appspot.com/5687064/diff/7008/src/once.h#newcode99
src/once.h:99: 
Style nit: two empty lines between function bodies.

http://codereview.appspot.com/5687064/diff/11001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

http://codereview.appspot.com/5687064/diff/11001/tools/gyp/v8.gyp#newcode385
tools/gyp/v8.gyp:385: '../../src/once.cc',
Please also update the scons build file: Add once.cc to src/SConscript

[pliard1, 2012-02-23 13:56:35]

http://codereview.appspot.com/5687064/diff/7008/src/once.cc
File src/once.cc (right):

http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode41
src/once.cc:41: STATE_INITIALIZED = 0,
On 2012/02/23 12:26:44, fschneider wrote:
> Maybe STATE_UNINITIALIZED would be clearer about what it means.

Done.

http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode45
src/once.cc:45: 
On 2012/02/23 12:26:44, fschneider wrote:
> Two \n here.

Done.

http://codereview.appspot.com/5687064/diff/7008/src/once.h
File src/once.h (right):

http://codereview.appspot.com/5687064/diff/7008/src/once.h#newcode75
src/once.h:75: #ifndef V8_STUBS_ONCE_H_
On 2012/02/23 12:26:44, fschneider wrote:
> This should be V8_ONCE_H_ for once.h per convention.

Done.

http://codereview.appspot.com/5687064/diff/7008/src/once.h#newcode95
src/once.h:95: FunctionClosure0 func(init_func, false);
On 2012/02/23 12:26:44, fschneider wrote:
> If you only ever need stack-allocated instances of closures, the class could
be
> simplified and changed  to forbid heap-allocation (in V8 we use the
> BASE_EMBEDDED macro for this)
> 
> You could then probably get rid of the self_deleting flag, too.

Done. Good idea.

http://codereview.appspot.com/5687064/diff/7008/src/once.h#newcode99
src/once.h:99: 
On 2012/02/23 12:26:44, fschneider wrote:
> Style nit: two empty lines between function bodies.

Done. I added one extra line between each struct/class in lazy_instance.h as
well.

http://codereview.appspot.com/5687064/diff/11001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

http://codereview.appspot.com/5687064/diff/11001/tools/gyp/v8.gyp#newcode385
tools/gyp/v8.gyp:385: '../../src/once.cc',
On 2012/02/23 12:26:44, fschneider wrote:
> Please also update the scons build file: Add once.cc to src/SConscript

Done.

[fschneider, 2012-02-24 08:34:53]

[+v8-dev]

lgtm. Please create the CL on the chromium code review tool:
http://codereview.chromium.org/

https://codereview.appspot.com/5687064/diff/9020/src/callback.h
File src/callback.h (right):

https://codereview.appspot.com/5687064/diff/9020/src/callback.h#newcode96
src/callback.h:96: */
You can cut out most of this comment, too.

[dvyukov, 2012-02-24 08:43:29]

On 2012/02/23 12:26:44, fschneider wrote:
> Please make sure that the base URL of your changes in the future always points
> to the bleeding_edge branch
> (https://v8.googlecode.com/svn/branches/bleeding_edge) and not to trunk.
> 
> To me it would seems cleaner to reduce callback.h to the minimum required for
> the purpose of static initialization.

Well, actually if we limit callbacks to "void (*init_func)(Arg*)" (instead of
"void (*init_func)(Arg)" which is IMVHO quite reasonable, then we can remove
callbacks.h entirely. The implementation will just cast "void
(*init_func)(Arg*)" to "void (*init_func)(void*)" and Arg* to void* and then
pass it to non-templated implementation.

[pliard1, 2012-02-24 09:29:32]

On 2012/02/24 08:43:29, dvyukov wrote:
> On 2012/02/23 12:26:44, fschneider wrote:
> > Please make sure that the base URL of your changes in the future always
points
> > to the bleeding_edge branch
> > (https://v8.googlecode.com/svn/branches/bleeding_edge) and not to trunk.
> > 
> > To me it would seems cleaner to reduce callback.h to the minimum required
for
> > the purpose of static initialization.
> 
> Well, actually if we limit callbacks to "void (*init_func)(Arg*)" (instead of
> "void (*init_func)(Arg)" which is IMVHO quite reasonable, then we can remove
> callbacks.h entirely. The implementation will just cast "void
> (*init_func)(Arg*)" to "void (*init_func)(void*)" and Arg* to void* and then
> pass it to non-templated implementation.

I removed callback.h.

[pliard, 2012-02-24 11:59:48]

Here is the link to the same CL on chromium codereview:
http://codereview.chromium.org/9447052/

On Fri, Feb 24, 2012 at 10:29 AM, <pliard@chromium.org> wrote:

> On 2012/02/24 08:43:29, dvyukov wrote:
>
>> On 2012/02/23 12:26:44, fschneider wrote:
>> > Please make sure that the base URL of your changes in the future
>>
> always points
>
>> > to the bleeding_edge branch
>> >
(https://v8.googlecode.com/**svn/branches/bleeding_edge<https://v8.googlecode....)
>> and not to
>>
> trunk.
>
>> >
>> > To me it would seems cleaner to reduce callback.h to the minimum
>>
> required for
>
>> > the purpose of static initialization.
>>
>
>  Well, actually if we limit callbacks to "void (*init_func)(Arg*)"
>>
> (instead of
>
>> "void (*init_func)(Arg)" which is IMVHO quite reasonable, then we can
>>
> remove
>
>> callbacks.h entirely. The implementation will just cast "void
>> (*init_func)(Arg*)" to "void (*init_func)(void*)" and Arg* to void*
>>
> and then
>
>> pass it to non-templated implementation.
>>
>
> I removed callback.h.
>
>
http://codereview.appspot.com/**5687064/<http://codereview.appspot.com/5687064/>
>