Improve GoogleOnceInit() in Protocol Buffers.

Improve GoogleOnceInit() in Protocol Buffers.

It is based on V8's new CallOnce(): http://codereview.chromium.org/9447052/.

This patch includes the following changes:
  - POD (no static initializer generated) and faster implementation on Windows.
  - GoogleOnceInit() can now take an additional parameter which is forwarded to
    the function provided by the user.

This patch is part of the static initializers removal initiative.


BUG=351

[Jisi Liu, 2012-02-28 18:22:24]

Thanks for the change!

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:59: // * This implementation supports recursive
calls (rather than deadlocking).
Any reason why we need to support recursive calls?

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:97: struct ProtobufOnceType {
Do we need to export this type? Could you please test under MSVC and try to
compile the protobuf library as a dll? See vsprojects/readme.txt

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:118: inline void
GoogleOnceInit(ProtobufOnceType* once,void (*init_func)(Arg),
need space after comma.

[kenton, 2012-02-28 19:18:50]

FYI

Can we now get rid of the GOOGLE_PROTOBUF_DECLARE_ONCE macro and instead
match the syntax used by the internal code?

On Tue, Feb 28, 2012 at 6:07 AM, <pliard@chromium.org> wrote:

> Reviewers: Pherl Liu,
>
> Description:
> Improve GoogleOnceInit() in Protocol Buffers.
>
> It is based on V8's new CallOnce():
>
http://codereview.chromium.**org/9447052/<http://codereview.chromium.org/9447...
> .
>
> This patch includes the following changes:
>  - POD (no static initializer generated) and faster implementation on
> Windows.
>  - GoogleOnceInit() can now take an additional parameter which is
> forwarded to
>    the function provided by the user.
>  - GoogleOnceInit() now supports recursive calls in a single thread.
> That
>    resulted in a deadlock before in the POSIX implementation.
>
> This patch is part of the static initializers removal initiative.
>
> I have not tested this change on Windows yet (my new Windows VM is being
> installed).
>
> BUG=351
>
>
> Please review this at
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>
> Affected files:
>  M src/google/protobuf/stubs/**once.cc
>  M src/google/protobuf/stubs/**once.h
>  M src/google/protobuf/stubs/**once_unittest.cc
>
>
>

[pliard1, 2012-02-29 11:17:43]

I updated the interface to match the internal code. Once my Windows VM is ready,
I can test this change with MSVC.

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:59: // * This implementation supports recursive
calls (rather than deadlocking).
On 2012/02/28 18:22:24, Pherl Liu wrote:
> Any reason why we need to support recursive calls?

The initial CL I sent, depending on this one, relies on this behavior.
With the appropriate compilation flags, protoc makes default_instance() use
GoogleOnceInit() to lazily call AddDesc() (which was initially called at static
init. time). But unfortunately, AddDesc(), when calling its dependencies'
AddDesc(), can indirectly call default_instance().

Here is a link to the initial CL: http://codereview.appspot.com/5643065

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:97: struct ProtobufOnceType {
On 2012/02/28 18:22:24, Pherl Liu wrote:
> Do we need to export this type? Could you please test under MSVC and try to
> compile the protobuf library as a dll? See vsprojects/readme.txt

I am still setting up my new Windows VM. I added LIBPROTOBUF_EXPORT back which I
accidentally removed.

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:118: inline void
GoogleOnceInit(ProtobufOnceType* once,void (*init_func)(Arg),
On 2012/02/28 18:22:24, Pherl Liu wrote:
> need space after comma.

Done.

[pliard1, 2012-02-29 14:36:45]

On 2012/02/29 11:17:43, pliard1 wrote:
> I updated the interface to match the internal code. Once my Windows VM is
ready,
> I can test this change with MSVC.
> 
> http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h
> File src/google/protobuf/stubs/once.h (right):
> 
>
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
> src/google/protobuf/stubs/once.h:59: // * This implementation supports
recursive
> calls (rather than deadlocking).
> On 2012/02/28 18:22:24, Pherl Liu wrote:
> > Any reason why we need to support recursive calls?
> 
> The initial CL I sent, depending on this one, relies on this behavior.
> With the appropriate compilation flags, protoc makes default_instance() use
> GoogleOnceInit() to lazily call AddDesc() (which was initially called at
static
> init. time). But unfortunately, AddDesc(), when calling its dependencies'
> AddDesc(), can indirectly call default_instance().
> 
> Here is a link to the initial CL: http://codereview.appspot.com/5643065
> 
>
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
> src/google/protobuf/stubs/once.h:97: struct ProtobufOnceType {
> On 2012/02/28 18:22:24, Pherl Liu wrote:
> > Do we need to export this type? Could you please test under MSVC and try to
> > compile the protobuf library as a dll? See vsprojects/readme.txt
> 
> I am still setting up my new Windows VM. I added LIBPROTOBUF_EXPORT back which
I
> accidentally removed.
> 
>
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
> src/google/protobuf/stubs/once.h:118: inline void
> GoogleOnceInit(ProtobufOnceType* once,void (*init_func)(Arg),
> On 2012/02/28 18:22:24, Pherl Liu wrote:
> > need space after comma.
> 
> Done.

I also added a naive implementation for users who don't care about thread safety
as we discussed in the previous CL (atomicops).

[pliard1, 2012-02-29 18:19:30]

On 2012/02/29 14:36:45, pliard1 wrote:
> On 2012/02/29 11:17:43, pliard1 wrote:
> > I updated the interface to match the internal code. Once my Windows VM is
> ready,
> > I can test this change with MSVC.
> > 
> >
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h
> > File src/google/protobuf/stubs/once.h (right):
> > 
> >
>
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
> > src/google/protobuf/stubs/once.h:59: // * This implementation supports
> recursive
> > calls (rather than deadlocking).
> > On 2012/02/28 18:22:24, Pherl Liu wrote:
> > > Any reason why we need to support recursive calls?
> > 
> > The initial CL I sent, depending on this one, relies on this behavior.
> > With the appropriate compilation flags, protoc makes default_instance() use
> > GoogleOnceInit() to lazily call AddDesc() (which was initially called at
> static
> > init. time). But unfortunately, AddDesc(), when calling its dependencies'
> > AddDesc(), can indirectly call default_instance().
> > 
> > Here is a link to the initial CL: http://codereview.appspot.com/5643065
> > 
> >
>
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
> > src/google/protobuf/stubs/once.h:97: struct ProtobufOnceType {
> > On 2012/02/28 18:22:24, Pherl Liu wrote:
> > > Do we need to export this type? Could you please test under MSVC and try
to
> > > compile the protobuf library as a dll? See vsprojects/readme.txt
> > 
> > I am still setting up my new Windows VM. I added LIBPROTOBUF_EXPORT back
which
> I
> > accidentally removed.
> > 
> >
>
http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
> > src/google/protobuf/stubs/once.h:118: inline void
> > GoogleOnceInit(ProtobufOnceType* once,void (*init_func)(Arg),
> > On 2012/02/28 18:22:24, Pherl Liu wrote:
> > > need space after comma.
> > 
> > Done.
> 
> I also added a naive implementation for users who don't care about thread
safety
> as we discussed in the previous CL (atomicops).

I uploaded a new patch set fixing the build on Windows. Yield was already
defined as a macro by windows.h (not surprisingly).

[Jisi Liu, 2012-03-01 19:24:11]

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:59: // * This implementation supports recursive
calls (rather than deadlocking).
Hmm, so does the google3 version of GoogleOnceInit support recursive calls? I'm
thinking about how we up-integrate this change back into google3.

On 2012/02/29 11:17:43, pliard1 wrote:
> On 2012/02/28 18:22:24, Pherl Liu wrote:
> > Any reason why we need to support recursive calls?
> 
> The initial CL I sent, depending on this one, relies on this behavior.
> With the appropriate compilation flags, protoc makes default_instance() use
> GoogleOnceInit() to lazily call AddDesc() (which was initially called at
static
> init. time). But unfortunately, AddDesc(), when calling its dependencies'
> AddDesc(), can indirectly call default_instance().
> 
> Here is a link to the initial CL: http://codereview.appspot.com/5643065

[pliard1, 2012-03-02 11:08:14]

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:59: // * This implementation supports recursive
calls (rather than deadlocking).
On 2012/03/01 19:24:11, Pherl Liu wrote:
> Hmm, so does the google3 version of GoogleOnceInit support recursive calls?
I'm
> thinking about how we up-integrate this change back into google3.
> 
> On 2012/02/29 11:17:43, pliard1 wrote:
> > On 2012/02/28 18:22:24, Pherl Liu wrote:
> > > Any reason why we need to support recursive calls?
> > 
> > The initial CL I sent, depending on this one, relies on this behavior.
> > With the appropriate compilation flags, protoc makes default_instance() use
> > GoogleOnceInit() to lazily call AddDesc() (which was initially called at
> static
> > init. time). But unfortunately, AddDesc(), when calling its dependencies'
> > AddDesc(), can indirectly call default_instance().
> > 
> > Here is a link to the initial CL: http://codereview.appspot.com/5643065
> 

It doesn't unfortunately. I double checked, and it deadlocks in that case.
I am afraid that we have internally to copy the reference implementation and
modify it slightly to support recursive calls.
I could take care of that.

[kenton, 2012-03-02 20:00:45]

Unfortunately I think it's going to be hard sell to either change google3's
GoogleOnce or introduce a new version of it that works differently.
 Concurrency primitives are extremely difficult to get right, and so Mike
Burrows is seen as the gatekeeper for any such code.  Knowing his opinions
on these things, I can pretty much guarantee that he's going to demand that
the protobuf code be rewritten to work with the existing GoogleOnce.

What are the challenges to avoiding recursive calls in protobufs?

On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:

>
> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
> File src/google/protobuf/stubs/**once.h (right):
>
> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
> src/google/protobuf/stubs/**once.h:59: // * This implementation supports
> recursive calls (rather than deadlocking).
> On 2012/03/01 19:24:11, Pherl Liu wrote:
>
>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>
> calls? I'm
>
>> thinking about how we up-integrate this change back into google3.
>>
>
>  On 2012/02/29 11:17:43, pliard1 wrote:
>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>> > > Any reason why we need to support recursive calls?
>> >
>> > The initial CL I sent, depending on this one, relies on this
>>
> behavior.
>
>> > With the appropriate compilation flags, protoc makes
>>
> default_instance() use
>
>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>
> called at
>
>> static
>> > init. time). But unfortunately, AddDesc(), when calling its
>>
> dependencies'
>
>> > AddDesc(), can indirectly call default_instance().
>> >
>> > Here is a link to the initial CL:
>>
> http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>
>
> It doesn't unfortunately. I double checked, and it deadlocks in that
> case.
> I am afraid that we have internally to copy the reference implementation
> and modify it slightly to support recursive calls.
> I could take care of that.
>
>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>

[pliard, 2012-03-05 11:10:45]

Avoiding recursive calls in the initialization code means making sure that
we don't have any cycle in the initialization call graph. Cycles are
present in multiple places including during message extensions registration
(RegisterMessageExtension() call) and InitAsDefaultInstance()
implementation.
Modifying InitAsDefaultInstance() to make it use default_instance_ instead
of default_instance() would require to generate some friends declarations
(default_instance_ is private). That seems totally possible.

I am more concerned about message extensions registration. One way to avoid
the cycle would be to do a topological sort and generate an Init() function
which calls all the sub init functions in topological order. That also
seems possible but would definitely require more time.

Using a modified version of GoogleOnceInit() would make our life
(especially mine) much easier. If you really think we can't have our own
(private) implementation of GoogleOnceInit() internally (which I
understand), I may send this change to Chromium first (if they agree) to
have an impact soon. I can still address the points I pointed out in
parallel to have all versions in sync eventually.

What do you think?

On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com> wrote:

> Unfortunately I think it's going to be hard sell to either change
> google3's GoogleOnce or introduce a new version of it that works
> differently.  Concurrency primitives are extremely difficult to get right,
> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
> his opinions on these things, I can pretty much guarantee that he's going
> to demand that the protobuf code be rewritten to work with the existing
> GoogleOnce.
>
> What are the challenges to avoiding recursive calls in protobufs?
>
>
> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>
>>
>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>> File src/google/protobuf/stubs/**once.h (right):
>>
>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>> src/google/protobuf/stubs/**once.h:59: // * This implementation supports
>> recursive calls (rather than deadlocking).
>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>
>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>
>> calls? I'm
>>
>>> thinking about how we up-integrate this change back into google3.
>>>
>>
>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>> > > Any reason why we need to support recursive calls?
>>> >
>>> > The initial CL I sent, depending on this one, relies on this
>>>
>> behavior.
>>
>>> > With the appropriate compilation flags, protoc makes
>>>
>> default_instance() use
>>
>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>
>> called at
>>
>>> static
>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>
>> dependencies'
>>
>>> > AddDesc(), can indirectly call default_instance().
>>> >
>>> > Here is a link to the initial CL:
>>>
>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>
>>
>> It doesn't unfortunately. I double checked, and it deadlocks in that
>> case.
>> I am afraid that we have internally to copy the reference implementation
>> and modify it slightly to support recursive calls.
>> I could take care of that.
>>
>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>
>
>

[pliard, 2012-03-05 15:28:34]

I will remove support for recursive calls so that we can already benefit
from this change (faster implementation for Windows users) and land it
soon. I will add support for recursive calls in a next CL if this is the
direction we choose (which seems  unlikely now).

On Mon, Mar 5, 2012 at 12:10 PM, Philippe Liard <pliard@google.com> wrote:

> Avoiding recursive calls in the initialization code means making sure that
> we don't have any cycle in the initialization call graph. Cycles are
> present in multiple places including during message extensions registration
> (RegisterMessageExtension() call) and InitAsDefaultInstance()
> implementation.
> Modifying InitAsDefaultInstance() to make it use default_instance_ instead
> of default_instance() would require to generate some friends declarations
> (default_instance_ is private). That seems totally possible.
>
> I am more concerned about message extensions registration. One way to
> avoid the cycle would be to do a topological sort and generate an Init()
> function which calls all the sub init functions in topological order. That
> also seems possible but would definitely require more time.
>
> Using a modified version of GoogleOnceInit() would make our life
> (especially mine) much easier. If you really think we can't have our own
> (private) implementation of GoogleOnceInit() internally (which I
> understand), I may send this change to Chromium first (if they agree) to
> have an impact soon. I can still address the points I pointed out in
> parallel to have all versions in sync eventually.
>
> What do you think?
>
> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com> wrote:
>
>> Unfortunately I think it's going to be hard sell to either change
>> google3's GoogleOnce or introduce a new version of it that works
>> differently.  Concurrency primitives are extremely difficult to get right,
>> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
>> his opinions on these things, I can pretty much guarantee that he's going
>> to demand that the protobuf code be rewritten to work with the existing
>> GoogleOnce.
>>
>> What are the challenges to avoiding recursive calls in protobufs?
>>
>>
>> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>>
>>>
>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>>> File src/google/protobuf/stubs/**once.h (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>>> src/google/protobuf/stubs/**once.h:59: // * This implementation supports
>>> recursive calls (rather than deadlocking).
>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>>
>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>>
>>> calls? I'm
>>>
>>>> thinking about how we up-integrate this change back into google3.
>>>>
>>>
>>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>>> > > Any reason why we need to support recursive calls?
>>>> >
>>>> > The initial CL I sent, depending on this one, relies on this
>>>>
>>> behavior.
>>>
>>>> > With the appropriate compilation flags, protoc makes
>>>>
>>> default_instance() use
>>>
>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>>
>>> called at
>>>
>>>> static
>>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>>
>>> dependencies'
>>>
>>>> > AddDesc(), can indirectly call default_instance().
>>>> >
>>>> > Here is a link to the initial CL:
>>>>
>>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>>
>>>
>>> It doesn't unfortunately. I double checked, and it deadlocks in that
>>> case.
>>> I am afraid that we have internally to copy the reference implementation
>>> and modify it slightly to support recursive calls.
>>> I could take care of that.
>>>
>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>
>>
>>
>

[pliard1, 2012-03-05 15:49:24]

On 2012/03/05 15:28:34, pliard wrote:
> I will remove support for recursive calls so that we can already benefit
> from this change (faster implementation for Windows users) and land it
> soon. I will add support for recursive calls in a next CL if this is the
> direction we choose (which seems  unlikely now).
> 
> On Mon, Mar 5, 2012 at 12:10 PM, Philippe Liard <mailto:pliard@google.com>
wrote:
> 
> > Avoiding recursive calls in the initialization code means making sure that
> > we don't have any cycle in the initialization call graph. Cycles are
> > present in multiple places including during message extensions registration
> > (RegisterMessageExtension() call) and InitAsDefaultInstance()
> > implementation.
> > Modifying InitAsDefaultInstance() to make it use default_instance_ instead
> > of default_instance() would require to generate some friends declarations
> > (default_instance_ is private). That seems totally possible.
> >
> > I am more concerned about message extensions registration. One way to
> > avoid the cycle would be to do a topological sort and generate an Init()
> > function which calls all the sub init functions in topological order. That
> > also seems possible but would definitely require more time.
> >
> > Using a modified version of GoogleOnceInit() would make our life
> > (especially mine) much easier. If you really think we can't have our own
> > (private) implementation of GoogleOnceInit() internally (which I
> > understand), I may send this change to Chromium first (if they agree) to
> > have an impact soon. I can still address the points I pointed out in
> > parallel to have all versions in sync eventually.
> >
> > What do you think?
> >
> > On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <mailto:kenton@google.com>
wrote:
> >
> >> Unfortunately I think it's going to be hard sell to either change
> >> google3's GoogleOnce or introduce a new version of it that works
> >> differently.  Concurrency primitives are extremely difficult to get right,
> >> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
> >> his opinions on these things, I can pretty much guarantee that he's going
> >> to demand that the protobuf code be rewritten to work with the existing
> >> GoogleOnce.
> >>
> >> What are the challenges to avoiding recursive calls in protobufs?
> >>
> >>
> >> On Fri, Mar 2, 2012 at 3:08 AM, <mailto:pliard@chromium.org> wrote:
> >>
> >>>
> >>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
> >>>
>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
> >>> File src/google/protobuf/stubs/**once.h (right):
> >>>
> >>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
> >>>
>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
> >>> src/google/protobuf/stubs/**once.h:59: // * This implementation supports
> >>> recursive calls (rather than deadlocking).
> >>> On 2012/03/01 19:24:11, Pherl Liu wrote:
> >>>
> >>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
> >>>>
> >>> calls? I'm
> >>>
> >>>> thinking about how we up-integrate this change back into google3.
> >>>>
> >>>
> >>>  On 2012/02/29 11:17:43, pliard1 wrote:
> >>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
> >>>> > > Any reason why we need to support recursive calls?
> >>>> >
> >>>> > The initial CL I sent, depending on this one, relies on this
> >>>>
> >>> behavior.
> >>>
> >>>> > With the appropriate compilation flags, protoc makes
> >>>>
> >>> default_instance() use
> >>>
> >>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
> >>>>
> >>> called at
> >>>
> >>>> static
> >>>> > init. time). But unfortunately, AddDesc(), when calling its
> >>>>
> >>> dependencies'
> >>>
> >>>> > AddDesc(), can indirectly call default_instance().
> >>>> >
> >>>> > Here is a link to the initial CL:
> >>>>
> >>>
>
http://codereview.appspot.com/**5643065%3Chttp://codereview.appspot.com/5643065>
> >>>
> >>>
> >>> It doesn't unfortunately. I double checked, and it deadlocks in that
> >>> case.
> >>> I am afraid that we have internally to copy the reference implementation
> >>> and modify it slightly to support recursive calls.
> >>> I could take care of that.
> >>>
> >>>
>
http://codereview.appspot.com/**5708043/%3Chttp://codereview.appspot.com/5708...>
> >>>
> >>
> >>
> >

I removed recursive calls support so that we can submit this change when you
approve it.

[kenton, 2012-03-05 19:49:05]

FYI

Assuming default instances are initialized first, why would extensions
introduce cycles at all?  Each extension is totally independent of any
other extension.

On Mon, Mar 5, 2012 at 3:10 AM, Philippe Liard <pliard@google.com> wrote:

> Avoiding recursive calls in the initialization code means making sure that
> we don't have any cycle in the initialization call graph. Cycles are
> present in multiple places including during message extensions registration
> (RegisterMessageExtension() call) and InitAsDefaultInstance()
> implementation.
> Modifying InitAsDefaultInstance() to make it use default_instance_ instead
> of default_instance() would require to generate some friends declarations
> (default_instance_ is private). That seems totally possible.
>
> I am more concerned about message extensions registration. One way to
> avoid the cycle would be to do a topological sort and generate an Init()
> function which calls all the sub init functions in topological order. That
> also seems possible but would definitely require more time.
>
> Using a modified version of GoogleOnceInit() would make our life
> (especially mine) much easier. If you really think we can't have our own
> (private) implementation of GoogleOnceInit() internally (which I
> understand), I may send this change to Chromium first (if they agree) to
> have an impact soon. I can still address the points I pointed out in
> parallel to have all versions in sync eventually.
>
> What do you think?
>
> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com> wrote:
>
>> Unfortunately I think it's going to be hard sell to either change
>> google3's GoogleOnce or introduce a new version of it that works
>> differently.  Concurrency primitives are extremely difficult to get right,
>> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
>> his opinions on these things, I can pretty much guarantee that he's going
>> to demand that the protobuf code be rewritten to work with the existing
>> GoogleOnce.
>>
>> What are the challenges to avoiding recursive calls in protobufs?
>>
>>
>> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>>
>>>
>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>>> File src/google/protobuf/stubs/**once.h (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>>> src/google/protobuf/stubs/**once.h:59: // * This implementation supports
>>> recursive calls (rather than deadlocking).
>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>>
>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>>
>>> calls? I'm
>>>
>>>> thinking about how we up-integrate this change back into google3.
>>>>
>>>
>>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>>> > > Any reason why we need to support recursive calls?
>>>> >
>>>> > The initial CL I sent, depending on this one, relies on this
>>>>
>>> behavior.
>>>
>>>> > With the appropriate compilation flags, protoc makes
>>>>
>>> default_instance() use
>>>
>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>>
>>> called at
>>>
>>>> static
>>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>>
>>> dependencies'
>>>
>>>> > AddDesc(), can indirectly call default_instance().
>>>> >
>>>> > Here is a link to the initial CL:
>>>>
>>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>>
>>>
>>> It doesn't unfortunately. I double checked, and it deadlocks in that
>>> case.
>>> I am afraid that we have internally to copy the reference implementation
>>> and modify it slightly to support recursive calls.
>>> I could take care of that.
>>>
>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>
>>
>>
>

[pliard, 2012-03-06 10:57:15]

I was initially a bit skeptical about the fact that default instances were
guaranteed to be initialized first.

It seems that we can at least guarantee that when static initializers are
disabled (with my change related to AddDesc()). When static initializers
are used we obviously can't predict the order of events (the order of the
various calls to AddDesc()). In that case we would have to keep using
default_instance() instead of default_instance_ (which is totally fine).

When static initializers are disabled (not completely actually because of
extension identifiers), we can guarantee that the extendee's
default_instance() is called first thanks to the last parameter provided to
the extension identifier when it is declared globally. Therefore we can use
default_instance_ instead of default_instance() where it formed a cycle
previously. We will have to add a few friends declarations to be allowed to
do that.
Example of extension identifier declaration in Chrome sync:
::google::protobuf::internal::ExtensionIdentifier<
::sync_pb::EntitySpecifics,
    ::google::protobuf::internal::MessageTypeTraits<
::sync_pb::AppSpecifics >, 11, false >


  app(kAppFieldNumber, ::sync_pb::AppSpecifics::default_instance());

I will make sure of these points when I update my other CL related to
AddDesc().

It finally seems that we actually don't need support for recursive calls.
Therefore we can move forward on this CL. I updated it so that
GoogleOnceInit() here has the same behavior as in Google3.

On Mon, Mar 5, 2012 at 8:48 PM, Kenton Varda <kenton@google.com> wrote:

> FYI
>
> Assuming default instances are initialized first, why would extensions
> introduce cycles at all?  Each extension is totally independent of any
> other extension.
>
> On Mon, Mar 5, 2012 at 3:10 AM, Philippe Liard <pliard@google.com> wrote:
>
>> Avoiding recursive calls in the initialization code means making sure
>> that we don't have any cycle in the initialization call graph. Cycles are
>> present in multiple places including during message extensions registration
>> (RegisterMessageExtension() call) and InitAsDefaultInstance()
>> implementation.
>> Modifying InitAsDefaultInstance() to make it use default_instance_
>> instead of default_instance() would require to generate some friends
>> declarations (default_instance_ is private). That seems totally possible.
>>
>> I am more concerned about message extensions registration. One way to
>> avoid the cycle would be to do a topological sort and generate an Init()
>> function which calls all the sub init functions in topological order. That
>> also seems possible but would definitely require more time.
>>
>> Using a modified version of GoogleOnceInit() would make our life
>> (especially mine) much easier. If you really think we can't have our own
>> (private) implementation of GoogleOnceInit() internally (which I
>> understand), I may send this change to Chromium first (if they agree) to
>> have an impact soon. I can still address the points I pointed out in
>> parallel to have all versions in sync eventually.
>>
>> What do you think?
>>
>> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com> wrote:
>>
>>> Unfortunately I think it's going to be hard sell to either change
>>> google3's GoogleOnce or introduce a new version of it that works
>>> differently.  Concurrency primitives are extremely difficult to get right,
>>> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
>>> his opinions on these things, I can pretty much guarantee that he's going
>>> to demand that the protobuf code be rewritten to work with the existing
>>> GoogleOnce.
>>>
>>> What are the challenges to avoiding recursive calls in protobufs?
>>>
>>>
>>> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>>>
>>>>
>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>>>> File src/google/protobuf/stubs/**once.h (right):
>>>>
>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>>>> src/google/protobuf/stubs/**once.h:59: // * This implementation
>>>> supports
>>>> recursive calls (rather than deadlocking).
>>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>>>
>>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>>>
>>>> calls? I'm
>>>>
>>>>> thinking about how we up-integrate this change back into google3.
>>>>>
>>>>
>>>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>>>> > > Any reason why we need to support recursive calls?
>>>>> >
>>>>> > The initial CL I sent, depending on this one, relies on this
>>>>>
>>>> behavior.
>>>>
>>>>> > With the appropriate compilation flags, protoc makes
>>>>>
>>>> default_instance() use
>>>>
>>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>>>
>>>> called at
>>>>
>>>>> static
>>>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>>>
>>>> dependencies'
>>>>
>>>>> > AddDesc(), can indirectly call default_instance().
>>>>> >
>>>>> > Here is a link to the initial CL:
>>>>>
>>>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>>>
>>>>
>>>> It doesn't unfortunately. I double checked, and it deadlocks in that
>>>> case.
>>>> I am afraid that we have internally to copy the reference implementation
>>>> and modify it slightly to support recursive calls.
>>>> I could take care of that.
>>>>
>>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>>
>>>
>>>
>>
>

[pliard, 2012-03-15 15:01:43]

Can we submit this CL (or complete its review) while the main one is being
reviewed?

Thanks,
Philippe.

On Tue, Mar 6, 2012 at 11:57 AM, Philippe Liard <pliard@google.com> wrote:

> I was initially a bit skeptical about the fact that default instances were
> guaranteed to be initialized first.
>
> It seems that we can at least guarantee that when static initializers are
> disabled (with my change related to AddDesc()). When static initializers
> are used we obviously can't predict the order of events (the order of the
> various calls to AddDesc()). In that case we would have to keep using
> default_instance() instead of default_instance_ (which is totally fine).
>
> When static initializers are disabled (not completely actually because of
> extension identifiers), we can guarantee that the extendee's
> default_instance() is called first thanks to the last parameter provided to
> the extension identifier when it is declared globally. Therefore we can use
> default_instance_ instead of default_instance() where it formed a cycle
> previously. We will have to add a few friends declarations to be allowed to
> do that.
> Example of extension identifier declaration in Chrome sync:
> ::google::protobuf::internal::ExtensionIdentifier<
> ::sync_pb::EntitySpecifics,
>     ::google::protobuf::internal::MessageTypeTraits<
> ::sync_pb::AppSpecifics >, 11, false >
>
>
>   app(kAppFieldNumber, ::sync_pb::AppSpecifics::default_instance());
>
> I will make sure of these points when I update my other CL related to
> AddDesc().
>
> It finally seems that we actually don't need support for recursive calls.
> Therefore we can move forward on this CL. I updated it so that
> GoogleOnceInit() here has the same behavior as in Google3.
>
> On Mon, Mar 5, 2012 at 8:48 PM, Kenton Varda <kenton@google.com> wrote:
>
>> FYI
>>
>> Assuming default instances are initialized first, why would extensions
>> introduce cycles at all?  Each extension is totally independent of any
>> other extension.
>>
>> On Mon, Mar 5, 2012 at 3:10 AM, Philippe Liard <pliard@google.com> wrote:
>>
>>> Avoiding recursive calls in the initialization code means making sure
>>> that we don't have any cycle in the initialization call graph. Cycles are
>>> present in multiple places including during message extensions registration
>>> (RegisterMessageExtension() call) and InitAsDefaultInstance()
>>> implementation.
>>> Modifying InitAsDefaultInstance() to make it use default_instance_
>>> instead of default_instance() would require to generate some friends
>>> declarations (default_instance_ is private). That seems totally possible.
>>>
>>> I am more concerned about message extensions registration. One way to
>>> avoid the cycle would be to do a topological sort and generate an Init()
>>> function which calls all the sub init functions in topological order. That
>>> also seems possible but would definitely require more time.
>>>
>>> Using a modified version of GoogleOnceInit() would make our life
>>> (especially mine) much easier. If you really think we can't have our own
>>> (private) implementation of GoogleOnceInit() internally (which I
>>> understand), I may send this change to Chromium first (if they agree) to
>>> have an impact soon. I can still address the points I pointed out in
>>> parallel to have all versions in sync eventually.
>>>
>>> What do you think?
>>>
>>> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com> wrote:
>>>
>>>> Unfortunately I think it's going to be hard sell to either change
>>>> google3's GoogleOnce or introduce a new version of it that works
>>>> differently.  Concurrency primitives are extremely difficult to get right,
>>>> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
>>>> his opinions on these things, I can pretty much guarantee that he's going
>>>> to demand that the protobuf code be rewritten to work with the existing
>>>> GoogleOnce.
>>>>
>>>> What are the challenges to avoiding recursive calls in protobufs?
>>>>
>>>>
>>>> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>>>>
>>>>>
>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>>>>> File src/google/protobuf/stubs/**once.h (right):
>>>>>
>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>>>>> src/google/protobuf/stubs/**once.h:59: // * This implementation
>>>>> supports
>>>>> recursive calls (rather than deadlocking).
>>>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>>>>
>>>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>>>>
>>>>> calls? I'm
>>>>>
>>>>>> thinking about how we up-integrate this change back into google3.
>>>>>>
>>>>>
>>>>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>>>>> > > Any reason why we need to support recursive calls?
>>>>>> >
>>>>>> > The initial CL I sent, depending on this one, relies on this
>>>>>>
>>>>> behavior.
>>>>>
>>>>>> > With the appropriate compilation flags, protoc makes
>>>>>>
>>>>> default_instance() use
>>>>>
>>>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>>>>
>>>>> called at
>>>>>
>>>>>> static
>>>>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>>>>
>>>>> dependencies'
>>>>>
>>>>>> > AddDesc(), can indirectly call default_instance().
>>>>>> >
>>>>>> > Here is a link to the initial CL:
>>>>>>
>>>>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>>>>
>>>>>
>>>>> It doesn't unfortunately. I double checked, and it deadlocks in that
>>>>> case.
>>>>> I am afraid that we have internally to copy the reference
>>>>> implementation
>>>>> and modify it slightly to support recursive calls.
>>>>> I could take care of that.
>>>>>
>>>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>>>
>>>>
>>>>
>>>
>>
>

[kenton, 2012-03-15 21:14:51]

>   uint64 tid;  // Used to handle recursive calls.

That should be removed now, right?  And the various stuff relating to
thread IDs.

Otherwise, design-wise, seems fine.

Pherl, can you do the full review and let me know if there's anything in
particular you want me to look at?

This is going to be a bit annoying to up-integrate, even though the
eventual result is that there will be fewer differences between the public
code and google3...

On Thu, Mar 15, 2012 at 8:01 AM, Philippe Liard <pliard@google.com> wrote:

> Can we submit this CL (or complete its review) while the main one is being
> reviewed?
>
> Thanks,
> Philippe.
>
> On Tue, Mar 6, 2012 at 11:57 AM, Philippe Liard <pliard@google.com> wrote:
>
>> I was initially a bit skeptical about the fact that default instances
>> were guaranteed to be initialized first.
>>
>> It seems that we can at least guarantee that when static initializers are
>> disabled (with my change related to AddDesc()). When static initializers
>> are used we obviously can't predict the order of events (the order of the
>> various calls to AddDesc()). In that case we would have to keep using
>> default_instance() instead of default_instance_ (which is totally fine).
>>
>> When static initializers are disabled (not completely actually because of
>> extension identifiers), we can guarantee that the extendee's
>> default_instance() is called first thanks to the last parameter provided to
>> the extension identifier when it is declared globally. Therefore we can use
>> default_instance_ instead of default_instance() where it formed a cycle
>> previously. We will have to add a few friends declarations to be allowed to
>> do that.
>> Example of extension identifier declaration in Chrome sync:
>> ::google::protobuf::internal::ExtensionIdentifier<
>> ::sync_pb::EntitySpecifics,
>>     ::google::protobuf::internal::MessageTypeTraits<
>> ::sync_pb::AppSpecifics >, 11, false >
>>
>>
>>   app(kAppFieldNumber, ::sync_pb::AppSpecifics::default_instance());
>>
>> I will make sure of these points when I update my other CL related to
>> AddDesc().
>>
>> It finally seems that we actually don't need support for recursive calls.
>> Therefore we can move forward on this CL. I updated it so that
>> GoogleOnceInit() here has the same behavior as in Google3.
>>
>> On Mon, Mar 5, 2012 at 8:48 PM, Kenton Varda <kenton@google.com> wrote:
>>
>>> FYI
>>>
>>> Assuming default instances are initialized first, why would extensions
>>> introduce cycles at all?  Each extension is totally independent of any
>>> other extension.
>>>
>>> On Mon, Mar 5, 2012 at 3:10 AM, Philippe Liard <pliard@google.com>wrote:
>>>
>>>> Avoiding recursive calls in the initialization code means making sure
>>>> that we don't have any cycle in the initialization call graph. Cycles are
>>>> present in multiple places including during message extensions registration
>>>> (RegisterMessageExtension() call) and InitAsDefaultInstance()
>>>> implementation.
>>>> Modifying InitAsDefaultInstance() to make it use default_instance_
>>>> instead of default_instance() would require to generate some friends
>>>> declarations (default_instance_ is private). That seems totally possible.
>>>>
>>>> I am more concerned about message extensions registration. One way to
>>>> avoid the cycle would be to do a topological sort and generate an Init()
>>>> function which calls all the sub init functions in topological order. That
>>>> also seems possible but would definitely require more time.
>>>>
>>>> Using a modified version of GoogleOnceInit() would make our life
>>>> (especially mine) much easier. If you really think we can't have our own
>>>> (private) implementation of GoogleOnceInit() internally (which I
>>>> understand), I may send this change to Chromium first (if they agree) to
>>>> have an impact soon. I can still address the points I pointed out in
>>>> parallel to have all versions in sync eventually.
>>>>
>>>> What do you think?
>>>>
>>>> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com> wrote:
>>>>
>>>>> Unfortunately I think it's going to be hard sell to either change
>>>>> google3's GoogleOnce or introduce a new version of it that works
>>>>> differently.  Concurrency primitives are extremely difficult to get right,
>>>>> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
>>>>> his opinions on these things, I can pretty much guarantee that he's going
>>>>> to demand that the protobuf code be rewritten to work with the existing
>>>>> GoogleOnce.
>>>>>
>>>>> What are the challenges to avoiding recursive calls in protobufs?
>>>>>
>>>>>
>>>>> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>>>>>
>>>>>>
>>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>>>>>> File src/google/protobuf/stubs/**once.h (right):
>>>>>>
>>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>>>>>> src/google/protobuf/stubs/**once.h:59: // * This implementation
>>>>>> supports
>>>>>> recursive calls (rather than deadlocking).
>>>>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>>>>>
>>>>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>>>>>
>>>>>> calls? I'm
>>>>>>
>>>>>>> thinking about how we up-integrate this change back into google3.
>>>>>>>
>>>>>>
>>>>>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>>>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>>>>>> > > Any reason why we need to support recursive calls?
>>>>>>> >
>>>>>>> > The initial CL I sent, depending on this one, relies on this
>>>>>>>
>>>>>> behavior.
>>>>>>
>>>>>>> > With the appropriate compilation flags, protoc makes
>>>>>>>
>>>>>> default_instance() use
>>>>>>
>>>>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>>>>>
>>>>>> called at
>>>>>>
>>>>>>> static
>>>>>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>>>>>
>>>>>> dependencies'
>>>>>>
>>>>>>> > AddDesc(), can indirectly call default_instance().
>>>>>>> >
>>>>>>> > Here is a link to the initial CL:
>>>>>>>
>>>>>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>>>>>
>>>>>>
>>>>>> It doesn't unfortunately. I double checked, and it deadlocks in that
>>>>>> case.
>>>>>> I am afraid that we have internally to copy the reference
>>>>>> implementation
>>>>>> and modify it slightly to support recursive calls.
>>>>>> I could take care of that.
>>>>>>
>>>>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[Jisi Liu, 2012-03-15 21:17:05]

On Thu, Mar 15, 2012 at 2:14 PM, Kenton Varda <kenton@google.com> wrote:

> >   uint64 tid;  // Used to handle recursive calls.
>
> That should be removed now, right?  And the various stuff relating to
> thread IDs.
>
> Otherwise, design-wise, seems fine.
>
> Pherl, can you do the full review and let me know if there's anything in
> particular you want me to look at?
>

Sure, will do

>
> This is going to be a bit annoying to up-integrate, even though the
> eventual result is that there will be fewer differences between the public
> code and google3...
>
> On Thu, Mar 15, 2012 at 8:01 AM, Philippe Liard <pliard@google.com> wrote:
>
>> Can we submit this CL (or complete its review) while the main one is
>> being reviewed?
>>
>> Thanks,
>> Philippe.
>>
>> On Tue, Mar 6, 2012 at 11:57 AM, Philippe Liard <pliard@google.com>wrote:
>>
>>> I was initially a bit skeptical about the fact that default instances
>>> were guaranteed to be initialized first.
>>>
>>> It seems that we can at least guarantee that when static initializers
>>> are disabled (with my change related to AddDesc()). When static
>>> initializers are used we obviously can't predict the order of events (the
>>> order of the various calls to AddDesc()). In that case we would have to
>>> keep using default_instance() instead of default_instance_ (which is
>>> totally fine).
>>>
>>> When static initializers are disabled (not completely actually because
>>> of extension identifiers), we can guarantee that the extendee's
>>> default_instance() is called first thanks to the last parameter provided to
>>> the extension identifier when it is declared globally. Therefore we can use
>>> default_instance_ instead of default_instance() where it formed a cycle
>>> previously. We will have to add a few friends declarations to be allowed to
>>> do that.
>>> Example of extension identifier declaration in Chrome sync:
>>> ::google::protobuf::internal::ExtensionIdentifier<
>>> ::sync_pb::EntitySpecifics,
>>>     ::google::protobuf::internal::MessageTypeTraits<
>>> ::sync_pb::AppSpecifics >, 11, false >
>>>
>>>
>>>   app(kAppFieldNumber, ::sync_pb::AppSpecifics::default_instance());
>>>
>>> I will make sure of these points when I update my other CL related to
>>> AddDesc().
>>>
>>> It finally seems that we actually don't need support for recursive
>>> calls. Therefore we can move forward on this CL. I updated it so that
>>> GoogleOnceInit() here has the same behavior as in Google3.
>>>
>>> On Mon, Mar 5, 2012 at 8:48 PM, Kenton Varda <kenton@google.com> wrote:
>>>
>>>> FYI
>>>>
>>>> Assuming default instances are initialized first, why would extensions
>>>> introduce cycles at all?  Each extension is totally independent of any
>>>> other extension.
>>>>
>>>> On Mon, Mar 5, 2012 at 3:10 AM, Philippe Liard <pliard@google.com>wrote:
>>>>
>>>>> Avoiding recursive calls in the initialization code means making sure
>>>>> that we don't have any cycle in the initialization call graph. Cycles are
>>>>> present in multiple places including during message extensions
registration
>>>>> (RegisterMessageExtension() call) and InitAsDefaultInstance()
>>>>> implementation.
>>>>> Modifying InitAsDefaultInstance() to make it use default_instance_
>>>>> instead of default_instance() would require to generate some friends
>>>>> declarations (default_instance_ is private). That seems totally possible.
>>>>>
>>>>> I am more concerned about message extensions registration. One way to
>>>>> avoid the cycle would be to do a topological sort and generate an Init()
>>>>> function which calls all the sub init functions in topological order. That
>>>>> also seems possible but would definitely require more time.
>>>>>
>>>>> Using a modified version of GoogleOnceInit() would make our life
>>>>> (especially mine) much easier. If you really think we can't have our own
>>>>> (private) implementation of GoogleOnceInit() internally (which I
>>>>> understand), I may send this change to Chromium first (if they agree) to
>>>>> have an impact soon. I can still address the points I pointed out in
>>>>> parallel to have all versions in sync eventually.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com>wrote:
>>>>>
>>>>>> Unfortunately I think it's going to be hard sell to either change
>>>>>> google3's GoogleOnce or introduce a new version of it that works
>>>>>> differently.  Concurrency primitives are extremely difficult to get
right,
>>>>>> and so Mike Burrows is seen as the gatekeeper for any such code.  Knowing
>>>>>> his opinions on these things, I can pretty much guarantee that he's going
>>>>>> to demand that the protobuf code be rewritten to work with the existing
>>>>>> GoogleOnce.
>>>>>>
>>>>>> What are the challenges to avoiding recursive calls in protobufs?
>>>>>>
>>>>>>
>>>>>> On Fri, Mar 2, 2012 at 3:08 AM, <pliard@chromium.org> wrote:
>>>>>>
>>>>>>>
>>>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
>>>>>>> File src/google/protobuf/stubs/**once.h (right):
>>>>>>>
>>>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
>>>>>>>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
>>>>>>> src/google/protobuf/stubs/**once.h:59: // * This implementation
>>>>>>> supports
>>>>>>> recursive calls (rather than deadlocking).
>>>>>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
>>>>>>>
>>>>>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
>>>>>>>>
>>>>>>> calls? I'm
>>>>>>>
>>>>>>>> thinking about how we up-integrate this change back into google3.
>>>>>>>>
>>>>>>>
>>>>>>>  On 2012/02/29 11:17:43, pliard1 wrote:
>>>>>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
>>>>>>>> > > Any reason why we need to support recursive calls?
>>>>>>>> >
>>>>>>>> > The initial CL I sent, depending on this one, relies on this
>>>>>>>>
>>>>>>> behavior.
>>>>>>>
>>>>>>>> > With the appropriate compilation flags, protoc makes
>>>>>>>>
>>>>>>> default_instance() use
>>>>>>>
>>>>>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
>>>>>>>>
>>>>>>> called at
>>>>>>>
>>>>>>>> static
>>>>>>>> > init. time). But unfortunately, AddDesc(), when calling its
>>>>>>>>
>>>>>>> dependencies'
>>>>>>>
>>>>>>>> > AddDesc(), can indirectly call default_instance().
>>>>>>>> >
>>>>>>>> > Here is a link to the initial CL:
>>>>>>>>
>>>>>>>
http://codereview.appspot.com/**5643065<http://codereview.appspot.com/5643065>
>>>>>>>
>>>>>>>
>>>>>>> It doesn't unfortunately. I double checked, and it deadlocks in that
>>>>>>> case.
>>>>>>> I am afraid that we have internally to copy the reference
>>>>>>> implementation
>>>>>>> and modify it slightly to support recursive calls.
>>>>>>> I could take care of that.
>>>>>>>
>>>>>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[pliard1, 2012-03-16 09:08:47]

On 2012/03/15 21:17:05, Pherl Liu wrote:
> On Thu, Mar 15, 2012 at 2:14 PM, Kenton Varda <mailto:kenton@google.com>
wrote:
> 
> > >   uint64 tid;  // Used to handle recursive calls.
> >
> > That should be removed now, right?  And the various stuff relating to
> > thread IDs.
> >
> > Otherwise, design-wise, seems fine.
> >
> > Pherl, can you do the full review and let me know if there's anything in
> > particular you want me to look at?
> >
> 
> Sure, will do
> 
> >
> > This is going to be a bit annoying to up-integrate, even though the
> > eventual result is that there will be fewer differences between the public
> > code and google3...
> >
> > On Thu, Mar 15, 2012 at 8:01 AM, Philippe Liard <mailto:pliard@google.com>
wrote:
> >
> >> Can we submit this CL (or complete its review) while the main one is
> >> being reviewed?
> >>
> >> Thanks,
> >> Philippe.
> >>
> >> On Tue, Mar 6, 2012 at 11:57 AM, Philippe Liard <pliard@google.com>wrote:
> >>
> >>> I was initially a bit skeptical about the fact that default instances
> >>> were guaranteed to be initialized first.
> >>>
> >>> It seems that we can at least guarantee that when static initializers
> >>> are disabled (with my change related to AddDesc()). When static
> >>> initializers are used we obviously can't predict the order of events (the
> >>> order of the various calls to AddDesc()). In that case we would have to
> >>> keep using default_instance() instead of default_instance_ (which is
> >>> totally fine).
> >>>
> >>> When static initializers are disabled (not completely actually because
> >>> of extension identifiers), we can guarantee that the extendee's
> >>> default_instance() is called first thanks to the last parameter provided
to
> >>> the extension identifier when it is declared globally. Therefore we can
use
> >>> default_instance_ instead of default_instance() where it formed a cycle
> >>> previously. We will have to add a few friends declarations to be allowed
to
> >>> do that.
> >>> Example of extension identifier declaration in Chrome sync:
> >>> ::google::protobuf::internal::ExtensionIdentifier<
> >>> ::sync_pb::EntitySpecifics,
> >>>     ::google::protobuf::internal::MessageTypeTraits<
> >>> ::sync_pb::AppSpecifics >, 11, false >
> >>>
> >>>
> >>>   app(kAppFieldNumber, ::sync_pb::AppSpecifics::default_instance());
> >>>
> >>> I will make sure of these points when I update my other CL related to
> >>> AddDesc().
> >>>
> >>> It finally seems that we actually don't need support for recursive
> >>> calls. Therefore we can move forward on this CL. I updated it so that
> >>> GoogleOnceInit() here has the same behavior as in Google3.
> >>>
> >>> On Mon, Mar 5, 2012 at 8:48 PM, Kenton Varda <mailto:kenton@google.com>
wrote:
> >>>
> >>>> FYI
> >>>>
> >>>> Assuming default instances are initialized first, why would extensions
> >>>> introduce cycles at all?  Each extension is totally independent of any
> >>>> other extension.
> >>>>
> >>>> On Mon, Mar 5, 2012 at 3:10 AM, Philippe Liard <pliard@google.com>wrote:
> >>>>
> >>>>> Avoiding recursive calls in the initialization code means making sure
> >>>>> that we don't have any cycle in the initialization call graph. Cycles
are
> >>>>> present in multiple places including during message extensions
> registration
> >>>>> (RegisterMessageExtension() call) and InitAsDefaultInstance()
> >>>>> implementation.
> >>>>> Modifying InitAsDefaultInstance() to make it use default_instance_
> >>>>> instead of default_instance() would require to generate some friends
> >>>>> declarations (default_instance_ is private). That seems totally
possible.
> >>>>>
> >>>>> I am more concerned about message extensions registration. One way to
> >>>>> avoid the cycle would be to do a topological sort and generate an Init()
> >>>>> function which calls all the sub init functions in topological order.
That
> >>>>> also seems possible but would definitely require more time.
> >>>>>
> >>>>> Using a modified version of GoogleOnceInit() would make our life
> >>>>> (especially mine) much easier. If you really think we can't have our own
> >>>>> (private) implementation of GoogleOnceInit() internally (which I
> >>>>> understand), I may send this change to Chromium first (if they agree) to
> >>>>> have an impact soon. I can still address the points I pointed out in
> >>>>> parallel to have all versions in sync eventually.
> >>>>>
> >>>>> What do you think?
> >>>>>
> >>>>> On Fri, Mar 2, 2012 at 9:00 PM, Kenton Varda <kenton@google.com>wrote:
> >>>>>
> >>>>>> Unfortunately I think it's going to be hard sell to either change
> >>>>>> google3's GoogleOnce or introduce a new version of it that works
> >>>>>> differently.  Concurrency primitives are extremely difficult to get
> right,
> >>>>>> and so Mike Burrows is seen as the gatekeeper for any such code. 
Knowing
> >>>>>> his opinions on these things, I can pretty much guarantee that he's
going
> >>>>>> to demand that the protobuf code be rewritten to work with the existing
> >>>>>> GoogleOnce.
> >>>>>>
> >>>>>> What are the challenges to avoiding recursive calls in protobufs?
> >>>>>>
> >>>>>>
> >>>>>> On Fri, Mar 2, 2012 at 3:08 AM, <mailto:pliard@chromium.org> wrote:
> >>>>>>
> >>>>>>>
> >>>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
> >>>>>>>
>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h>
> >>>>>>> File src/google/protobuf/stubs/**once.h (right):
> >>>>>>>
> >>>>>>> http://codereview.appspot.com/**5708043/diff/1/src/google/**
> >>>>>>>
>
protobuf/stubs/once.h#**newcode59<http://codereview.appspot.com/5708043/diff/1/src/google/protobuf/stubs/once.h#newcode59>
> >>>>>>> src/google/protobuf/stubs/**once.h:59: // * This implementation
> >>>>>>> supports
> >>>>>>> recursive calls (rather than deadlocking).
> >>>>>>> On 2012/03/01 19:24:11, Pherl Liu wrote:
> >>>>>>>
> >>>>>>>> Hmm, so does the google3 version of GoogleOnceInit support recursive
> >>>>>>>>
> >>>>>>> calls? I'm
> >>>>>>>
> >>>>>>>> thinking about how we up-integrate this change back into google3.
> >>>>>>>>
> >>>>>>>
> >>>>>>>  On 2012/02/29 11:17:43, pliard1 wrote:
> >>>>>>>> > On 2012/02/28 18:22:24, Pherl Liu wrote:
> >>>>>>>> > > Any reason why we need to support recursive calls?
> >>>>>>>> >
> >>>>>>>> > The initial CL I sent, depending on this one, relies on this
> >>>>>>>>
> >>>>>>> behavior.
> >>>>>>>
> >>>>>>>> > With the appropriate compilation flags, protoc makes
> >>>>>>>>
> >>>>>>> default_instance() use
> >>>>>>>
> >>>>>>>> > GoogleOnceInit() to lazily call AddDesc() (which was initially
> >>>>>>>>
> >>>>>>> called at
> >>>>>>>
> >>>>>>>> static
> >>>>>>>> > init. time). But unfortunately, AddDesc(), when calling its
> >>>>>>>>
> >>>>>>> dependencies'
> >>>>>>>
> >>>>>>>> > AddDesc(), can indirectly call default_instance().
> >>>>>>>> >
> >>>>>>>> > Here is a link to the initial CL:
> >>>>>>>>
> >>>>>>>
>
http://codereview.appspot.com/**5643065%3Chttp://codereview.appspot.com/5643065>
> >>>>>>>
> >>>>>>>
> >>>>>>> It doesn't unfortunately. I double checked, and it deadlocks in that
> >>>>>>> case.
> >>>>>>> I am afraid that we have internally to copy the reference
> >>>>>>> implementation
> >>>>>>> and modify it slightly to support recursive calls.
> >>>>>>> I could take care of that.
> >>>>>>>
> >>>>>>>
>
http://codereview.appspot.com/**5708043/%3Chttp://codereview.appspot.com/5708...>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >

All the recursive call stuff (including thread id) is now removed.

Thanks,
Philippe.

[Jisi Liu, 2012-03-21 00:33:26]

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler...
File src/google/protobuf/compiler/cpp/cpp_file.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler...
src/google/protobuf/compiler/cpp/cpp_file.cc:451:
"::google::protobuf::GoogleOnceType protobuf_AssignDescriptors_once_ =\n"
reserve the macro so we don't need to change this.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descript...
File src/google/protobuf/descriptor.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descript...
src/google/protobuf/descriptor.cc:805: GoogleOnceType generated_pool_init_ =
GOOGLE_PROTOBUF_ONCE_INIT;
ditto

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extensio...
File src/google/protobuf/extension_set.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extensio...
src/google/protobuf/extension_set.cc:65: GoogleOnceType registry_init_ =
GOOGLE_PROTOBUF_ONCE_INIT;
ditto

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc
File src/google/protobuf/message.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message....
src/google/protobuf/message.cc:218: GoogleOnceType
generated_message_factory_once_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
ditto.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/co...
File src/google/protobuf/stubs/common.cc (left):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/co...
src/google/protobuf/stubs/common.cc:127:
GOOGLE_PROTOBUF_DECLARE_ONCE(log_silencer_count_init_);
ditto

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
File src/google/protobuf/stubs/once.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.cc:89: SchedYield();
Consider using futex in linux for spinlocks?

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
File src/google/protobuf/stubs/once.h (left):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:42: // * A macro GOOGLE_PROTOBUF_DECLARE_ONCE()
which declares a variable of type
Can we reserve the macro here, so it would make it easier when we upgrade the
change back to google3. There's a script automatically converting google3 style
once init to this macro. We should try remain consistent if possible.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:48: //     GoogleOnceInit(&my_once,
&MyFunctionExpectingIntArgument, 10);
I think the google3 version takes a pointer of the argument. Let's keep them
consistent.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:109: ONCE_STATE_DONE = 2
The google3 version uses improbable 32-bit value. Should we do something similar
here?

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:114: #define GOOGLE_PROTOBUF_ONCE_INIT 0
#define GOOGLE_PROTOBUF_ONCE_INIT ONCE_STATE_UNINITIALIZED

[kenton, 2012-03-21 00:46:24]

The new code (without the macro) ought to be a closer match to what is in
google3, so I think getting rid of the macro actually makes things easier
as far as matching google3 goes.  Or are you saying you'd rather keep the
macro now, but remove it later next time we down-integrate from google3?

On Tue, Mar 20, 2012 at 5:33 PM, <liujisi@google.com> wrote:

>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/compiler/cpp/cpp_**file.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler/cpp/cpp_file.cc>
> File src/google/protobuf/compiler/**cpp/cpp_file.cc (right):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/compiler/cpp/cpp_**file.cc#newcode451<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler/cpp/cpp_file.cc#newcode451>
> src/google/protobuf/compiler/**cpp/cpp_file.cc:451:
> "::google::protobuf::**GoogleOnceType protobuf_AssignDescriptors_**once_
> =\n"
> reserve the macro so we don't need to change this.
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/descriptor.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descriptor.cc>
> File src/google/protobuf/**descriptor.cc (right):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/descriptor.cc#**newcode805<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descriptor.cc#newcode805>
> src/google/protobuf/**descriptor.cc:805: GoogleOnceType
> generated_pool_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
> ditto
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/extension_set.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extension_set.cc>
> File src/google/protobuf/extension_**set.cc (right):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/extension_set.cc#**newcode65<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extension_set.cc#newcode65>
> src/google/protobuf/extension_**set.cc:65: GoogleOnceType registry_init_ =
> GOOGLE_PROTOBUF_ONCE_INIT;
> ditto
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/message.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc>
> File src/google/protobuf/message.cc (right):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/message.cc#newcode218<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc#newcode218>
> src/google/protobuf/message.**cc:218: GoogleOnceType
> generated_message_factory_**once_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
> ditto.
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/common.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/common.cc>
> File src/google/protobuf/stubs/**common.cc (left):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/common.cc#**oldcode127<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/common.cc#oldcode127>
> src/google/protobuf/stubs/**common.cc:127:
> GOOGLE_PROTOBUF_DECLARE_ONCE(**log_silencer_count_init_);
> ditto
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.cc>
> File src/google/protobuf/stubs/**once.cc (right):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.cc#**newcode89<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.cc#newcode89>
> src/google/protobuf/stubs/**once.cc:89: SchedYield();
> Consider using futex in linux for spinlocks?
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h>
> File src/google/protobuf/stubs/**once.h (left):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.h#**oldcode42<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#oldcode42>
> src/google/protobuf/stubs/**once.h:42: // * A macro
> GOOGLE_PROTOBUF_DECLARE_ONCE() which declares a variable of type
> Can we reserve the macro here, so it would make it easier when we
> upgrade the change back to google3. There's a script automatically
> converting google3 style once init to this macro. We should try remain
> consistent if possible.
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h>
> File src/google/protobuf/stubs/**once.h (right):
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.h#**newcode48<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode48>
> src/google/protobuf/stubs/**once.h:48: //     GoogleOnceInit(&my_once,
> &**MyFunctionExpectingIntArgument**, 10);
> I think the google3 version takes a pointer of the argument. Let's keep
> them consistent.
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.h#**newcode109<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode109>
> src/google/protobuf/stubs/**once.h:109: ONCE_STATE_DONE = 2
> The google3 version uses improbable 32-bit value. Should we do something
> similar here?
>
> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>
protobuf/stubs/once.h#**newcode114<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode114>
> src/google/protobuf/stubs/**once.h:114: #define GOOGLE_PROTOBUF_ONCE_INIT
> 0
> #define GOOGLE_PROTOBUF_ONCE_INIT ONCE_STATE_UNINITIALIZED
>
>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>

[Jisi Liu, 2012-03-21 00:56:57]

Yes, that would be easier. (remove one sed command) It will introduce less
code conflict during up-integrate step. Also should we reserve the macro
definitions, in case opensource users are using the once init already?

On Tue, Mar 20, 2012 at 5:45 PM, Kenton Varda <kenton@google.com> wrote:

> The new code (without the macro) ought to be a closer match to what is in
> google3, so I think getting rid of the macro actually makes things easier
> as far as matching google3 goes.  Or are you saying you'd rather keep the
> macro now, but remove it later next time we down-integrate from google3?
>
>
> On Tue, Mar 20, 2012 at 5:33 PM, <liujisi@google.com> wrote:
>
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/compiler/cpp/cpp_**file.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler/cpp/cpp_file.cc>
>> File src/google/protobuf/compiler/**cpp/cpp_file.cc (right):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/compiler/cpp/cpp_**file.cc#newcode451<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler/cpp/cpp_file.cc#newcode451>
>> src/google/protobuf/compiler/**cpp/cpp_file.cc:451:
>> "::google::protobuf::**GoogleOnceType protobuf_AssignDescriptors_**once_
>> =\n"
>> reserve the macro so we don't need to change this.
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/descriptor.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descriptor.cc>
>> File src/google/protobuf/**descriptor.cc (right):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/descriptor.cc#**newcode805<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descriptor.cc#newcode805>
>> src/google/protobuf/**descriptor.cc:805: GoogleOnceType
>> generated_pool_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
>> ditto
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/extension_set.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extension_set.cc>
>> File src/google/protobuf/extension_**set.cc (right):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/extension_set.cc#**newcode65<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extension_set.cc#newcode65>
>> src/google/protobuf/extension_**set.cc:65: GoogleOnceType registry_init_
>> =
>> GOOGLE_PROTOBUF_ONCE_INIT;
>> ditto
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/message.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc>
>> File src/google/protobuf/message.cc (right):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/message.cc#newcode218<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc#newcode218>
>> src/google/protobuf/message.**cc:218: GoogleOnceType
>> generated_message_factory_**once_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
>> ditto.
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/common.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/common.cc>
>> File src/google/protobuf/stubs/**common.cc (left):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/common.cc#**oldcode127<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/common.cc#oldcode127>
>> src/google/protobuf/stubs/**common.cc:127:
>> GOOGLE_PROTOBUF_DECLARE_ONCE(**log_silencer_count_init_);
>> ditto
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.cc>
>> File src/google/protobuf/stubs/**once.cc (right):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.cc#**newcode89<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.cc#newcode89>
>> src/google/protobuf/stubs/**once.cc:89: SchedYield();
>> Consider using futex in linux for spinlocks?
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h>
>> File src/google/protobuf/stubs/**once.h (left):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.h#**oldcode42<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#oldcode42>
>> src/google/protobuf/stubs/**once.h:42: // * A macro
>> GOOGLE_PROTOBUF_DECLARE_ONCE() which declares a variable of type
>> Can we reserve the macro here, so it would make it easier when we
>> upgrade the change back to google3. There's a script automatically
>> converting google3 style once init to this macro. We should try remain
>> consistent if possible.
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h>
>> File src/google/protobuf/stubs/**once.h (right):
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.h#**newcode48<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode48>
>> src/google/protobuf/stubs/**once.h:48: //     GoogleOnceInit(&my_once,
>> &**MyFunctionExpectingIntArgument**, 10);
>> I think the google3 version takes a pointer of the argument. Let's keep
>> them consistent.
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.h#**newcode109<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode109>
>> src/google/protobuf/stubs/**once.h:109: ONCE_STATE_DONE = 2
>> The google3 version uses improbable 32-bit value. Should we do something
>> similar here?
>>
>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>
protobuf/stubs/once.h#**newcode114<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode114>
>> src/google/protobuf/stubs/**once.h:114: #define GOOGLE_PROTOBUF_ONCE_INIT
>> 0
>> #define GOOGLE_PROTOBUF_ONCE_INIT ONCE_STATE_UNINITIALIZED
>>
>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>
>
>

[kenton, 2012-03-21 01:02:23]

The header comment says users should not include once.h, so I am not
worried about breaking them.  But I suppose keeping the macro for now
reduces diffs which will make the up-integrate easier.  We should make sure
to remove the macro replacements from the down-integrate script before the
next down-integrate.

On Tue, Mar 20, 2012 at 5:56 PM, Pherl Liu <liujisi@google.com> wrote:

> Yes, that would be easier. (remove one sed command) It will introduce less
> code conflict during up-integrate step. Also should we reserve the macro
> definitions, in case opensource users are using the once init already?
>
>
> On Tue, Mar 20, 2012 at 5:45 PM, Kenton Varda <kenton@google.com> wrote:
>
>> The new code (without the macro) ought to be a closer match to what is in
>> google3, so I think getting rid of the macro actually makes things easier
>> as far as matching google3 goes.  Or are you saying you'd rather keep the
>> macro now, but remove it later next time we down-integrate from google3?
>>
>>
>> On Tue, Mar 20, 2012 at 5:33 PM, <liujisi@google.com> wrote:
>>
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/compiler/cpp/cpp_**file.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler/cpp/cpp_file.cc>
>>> File src/google/protobuf/compiler/**cpp/cpp_file.cc (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/compiler/cpp/cpp_**file.cc#newcode451<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler/cpp/cpp_file.cc#newcode451>
>>> src/google/protobuf/compiler/**cpp/cpp_file.cc:451:
>>> "::google::protobuf::**GoogleOnceType protobuf_AssignDescriptors_**once_
>>> =\n"
>>> reserve the macro so we don't need to change this.
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/descriptor.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descriptor.cc>
>>> File src/google/protobuf/**descriptor.cc (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/descriptor.cc#**newcode805<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descriptor.cc#newcode805>
>>> src/google/protobuf/**descriptor.cc:805: GoogleOnceType
>>> generated_pool_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
>>> ditto
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/extension_set.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extension_set.cc>
>>> File src/google/protobuf/extension_**set.cc (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/extension_set.cc#**newcode65<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extension_set.cc#newcode65>
>>> src/google/protobuf/extension_**set.cc:65: GoogleOnceType
>>> registry_init_ =
>>> GOOGLE_PROTOBUF_ONCE_INIT;
>>> ditto
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/message.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc>
>>> File src/google/protobuf/message.cc (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/message.cc#newcode218<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc#newcode218>
>>> src/google/protobuf/message.**cc:218: GoogleOnceType
>>> generated_message_factory_**once_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
>>> ditto.
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/common.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/common.cc>
>>> File src/google/protobuf/stubs/**common.cc (left):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/common.cc#**oldcode127<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/common.cc#oldcode127>
>>> src/google/protobuf/stubs/**common.cc:127:
>>> GOOGLE_PROTOBUF_DECLARE_ONCE(**log_silencer_count_init_);
>>> ditto
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.cc<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.cc>
>>> File src/google/protobuf/stubs/**once.cc (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.cc#**newcode89<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.cc#newcode89>
>>> src/google/protobuf/stubs/**once.cc:89: SchedYield();
>>> Consider using futex in linux for spinlocks?
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h>
>>> File src/google/protobuf/stubs/**once.h (left):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.h#**oldcode42<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#oldcode42>
>>> src/google/protobuf/stubs/**once.h:42: // * A macro
>>> GOOGLE_PROTOBUF_DECLARE_ONCE() which declares a variable of type
>>> Can we reserve the macro here, so it would make it easier when we
>>> upgrade the change back to google3. There's a script automatically
>>> converting google3 style once init to this macro. We should try remain
>>> consistent if possible.
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.h<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h>
>>> File src/google/protobuf/stubs/**once.h (right):
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.h#**newcode48<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode48>
>>> src/google/protobuf/stubs/**once.h:48: //     GoogleOnceInit(&my_once,
>>> &**MyFunctionExpectingIntArgument**, 10);
>>> I think the google3 version takes a pointer of the argument. Let's keep
>>> them consistent.
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.h#**newcode109<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode109>
>>> src/google/protobuf/stubs/**once.h:109: ONCE_STATE_DONE = 2
>>> The google3 version uses improbable 32-bit value. Should we do something
>>> similar here?
>>>
>>> http://codereview.appspot.com/**5708043/diff/28001/src/google/**
>>>
protobuf/stubs/once.h#**newcode114<http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/once.h#newcode114>
>>> src/google/protobuf/stubs/**once.h:114: #define
>>> GOOGLE_PROTOBUF_ONCE_INIT
>>> 0
>>> #define GOOGLE_PROTOBUF_ONCE_INIT ONCE_STATE_UNINITIALIZED
>>>
>>>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>>>
>>
>>
>

[pliard1, 2012-03-22 11:06:59]

I reverted the changes that I made a few patch sets ago to match the Google3
interface. Now only the implementation is affected by this CL.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler...
File src/google/protobuf/compiler/cpp/cpp_file.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/compiler...
src/google/protobuf/compiler/cpp/cpp_file.cc:451:
"::google::protobuf::GoogleOnceType protobuf_AssignDescriptors_once_ =\n"
On 2012/03/21 00:33:26, Pherl Liu wrote:
> reserve the macro so we don't need to change this.

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descript...
File src/google/protobuf/descriptor.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/descript...
src/google/protobuf/descriptor.cc:805: GoogleOnceType generated_pool_init_ =
GOOGLE_PROTOBUF_ONCE_INIT;
On 2012/03/21 00:33:26, Pherl Liu wrote:
> ditto

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extensio...
File src/google/protobuf/extension_set.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/extensio...
src/google/protobuf/extension_set.cc:65: GoogleOnceType registry_init_ =
GOOGLE_PROTOBUF_ONCE_INIT;
On 2012/03/21 00:33:26, Pherl Liu wrote:
> ditto

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message.cc
File src/google/protobuf/message.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/message....
src/google/protobuf/message.cc:218: GoogleOnceType
generated_message_factory_once_init_ = GOOGLE_PROTOBUF_ONCE_INIT;
On 2012/03/21 00:33:26, Pherl Liu wrote:
> ditto.

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/co...
File src/google/protobuf/stubs/common.cc (left):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/co...
src/google/protobuf/stubs/common.cc:127:
GOOGLE_PROTOBUF_DECLARE_ONCE(log_silencer_count_init_);
On 2012/03/21 00:33:26, Pherl Liu wrote:
> ditto

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
File src/google/protobuf/stubs/once.cc (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.cc:89: SchedYield();
On 2012/03/21 00:33:26, Pherl Liu wrote:
> Consider using futex in linux for spinlocks?

I can use futex if you really want me to. The only thing is that it would force
me to make ONCE_STATE_DONE equal FUTEX_WAKE in once.h for linux only. Here in
once.cc it would also make me replace SchedField() with WaitForCompletion()
which would have 3 implementations (one for Windows, one for Linux and another
one for other POSIX systems). I can make the change you suggest, I'm just afraid
that you tell me then that it does not add much value in comparison with the
complexity it introduces.

Please let me know what you want me to do finally. In my opinion the current
approach is reasonable (and is the one currently used in Chromium and v8).

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
File src/google/protobuf/stubs/once.h (left):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:42: // * A macro GOOGLE_PROTOBUF_DECLARE_ONCE()
which declares a variable of type
On 2012/03/21 00:33:26, Pherl Liu wrote:
> Can we reserve the macro here, so it would make it easier when we upgrade the
> change back to google3. There's a script automatically converting google3
style
> once init to this macro. We should try remain consistent if possible.

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:48: //     GoogleOnceInit(&my_once,
&MyFunctionExpectingIntArgument, 10);
On 2012/03/21 00:33:26, Pherl Liu wrote:
> I think the google3 version takes a pointer of the argument. Let's keep them
> consistent.

Done.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:109: ONCE_STATE_DONE = 2
On 2012/03/21 00:33:26, Pherl Liu wrote:
> The google3 version uses improbable 32-bit value. Should we do something
similar
> here?

I'm waiting for your opinion about futex() (which would make ONCE_STATE_DONE
equal FUTEX_WAKE here). Also I'm not sure how much value the improbable values
add in our case.

http://codereview.appspot.com/5708043/diff/28001/src/google/protobuf/stubs/on...
src/google/protobuf/stubs/once.h:114: #define GOOGLE_PROTOBUF_ONCE_INIT 0
On 2012/03/21 00:33:26, Pherl Liu wrote:
> #define GOOGLE_PROTOBUF_ONCE_INIT ONCE_STATE_UNINITIALIZED

Done.

[Jisi Liu, 2012-03-24 01:13:58]

LGTM

Fine. I guess we can put a comment about the choice to use futex for linux.

[kenton, 2012-03-24 01:32:12]

FYI

FWIW, I think futex() is too platform-specific and too subtle to be worth
trying to use here, unless we observe a real need.

On Fri, Mar 23, 2012 at 6:13 PM, <liujisi@google.com> wrote:

> LGTM
>
> Fine. I guess we can put a comment about the choice to use futex for
> linux.
>
>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>

[pliard1, 2012-03-26 09:02:40]

On 2012/03/24 01:32:12, kenton wrote:
> FYI
> 
> FWIW, I think futex() is too platform-specific and too subtle to be worth
> trying to use here, unless we observe a real need.
> 
> On Fri, Mar 23, 2012 at 6:13 PM, <mailto:liujisi@google.com> wrote:
> 
> > LGTM
> >
> > Fine. I guess we can put a comment about the choice to use futex for
> > linux.
> >
> >
>
http://codereview.appspot.com/**5708043/%3Chttp://codereview.appspot.com/5708...>
> >

Thanks for the review!

I added a comment about futex(). I'm also waiting for your LGTM Kenton before I
submit that.

Cheers,
Philippe.

[kenton, 2012-03-26 19:16:52]

LGTM

Only briefly skimmed the code; mostly trusting Pherl's review.

On Mon, Mar 26, 2012 at 2:02 AM, <pliard@chromium.org> wrote:

> On 2012/03/24 01:32:12, kenton wrote:
>
>> FYI
>>
>
>  FWIW, I think futex() is too platform-specific and too subtle to be
>>
> worth
>
>> trying to use here, unless we observe a real need.
>>
>
>  On Fri, Mar 23, 2012 at 6:13 PM, <mailto:liujisi@google.com> wrote:
>>
>
>  > LGTM
>> >
>> > Fine. I guess we can put a comment about the choice to use futex for
>> > linux.
>> >
>> >
>>
>
> http://codereview.appspot.com/****5708043/%3Chttp://**
>
codereview.appspot.com/**5708043/<http://codereview.appspot.com/**5708043/%3Chttp://codereview.appspot.com/5708043/>
> >
>
>> >
>>
>
> Thanks for the review!
>
> I added a comment about futex(). I'm also waiting for your LGTM Kenton
> before I submit that.
>
> Cheers,
> Philippe.
>
>
http://codereview.appspot.com/**5708043/<http://codereview.appspot.com/5708043/>
>