code review 6442107: re2: eliminate global c++ constructors

re2: eliminate global c++ constructors

[rsc, 2012-08-09 20:19:16]

Hello battre (cc: re2-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/re2

[Dominic Battre, 2012-08-13 14:43:01]

Awesome. Thanks so much for doing this.

I am adding digit@ for his consent, whether util/mutex.h:201 is ok for us. I
think this will introduce two static initializers on Windows only.

http://codereview.appspot.com/6442107/diff/8001/re2/re2.h
File re2/re2.h (right):

http://codereview.appspot.com/6442107/diff/8001/re2/re2.h#newcode577
re2/re2.h:577: /*implicit*/ Options(CannedOptions);
Wow, this is an awesome idea!

http://codereview.appspot.com/6442107/diff/8001/util/mutex.h
File util/mutex.h (right):

http://codereview.appspot.com/6442107/diff/8001/util/mutex.h#newcode201
util/mutex.h:201: static Mutex name
This one will probably still create a static initializer.

[digit, 2012-08-13 14:53:41]

lgtm (not an owner) :-) Thanks for doing this, btw.

[rsc_swtch, 2012-08-13 16:05:32]

On Windows if you know what the right declarations are it should be
easy to implement the macros like for pthread and not have any static
initializers. If that matters to you, just let me know what to type.
:-)

[Dominic Battre, 2012-08-13 16:16:17]

On 2012/08/13 16:05:32, rsc_swtch wrote:
> On Windows if you know what the right declarations are it should be
> easy to implement the macros like for pthread and not have any static
> initializers. If that matters to you, just let me know what to type.
> :-)

:-)

David suggested this snippet for windows (and using pthread_once_t for pthread):

// MSVC ensures that loads of volatile variables have acquire semantics, and
stores have release semantics.

typedef LONG volatile  OnceType;
enum {
  ONCE_INIT = 0,
  ONCE_CONSTRUCTING,
  ONCE_COMPLETED
};
void inline OnceInit(OnceType* once, void(*callback)()) {
  if (*once == ONCE_COMPLETED)  // inlined fast-path
    return;
  OnceInitImpl(once, callback);

}
....
void OnceInitImpl(OnceType* once, void (*callback)()) {
  OnceType state = reinterpret_cast<OnceType>(InterlockedCompareExchange(once,
ONCE_CONSTRUCTING, ONCE_INIT));

  if (state == ONCE_COMPLETED)
    return;
  else if (state == ONCE_INIT) { // we're the first thread, run the callback
    (*callback)();

    *once = ONCE_COMPLETED;
  } else while (*once == ONCE_CONSTRUCTING) { // need to wait for completion
    Sleep(0);  // yield thread on Windows

  }
}

Bug I am happy with the current proposal if David is.

Best regards,
Dominic

[Dominic Battre, 2012-08-16 16:03:41]

On 2012/08/13 16:16:17, Dominic Battre wrote:
> On 2012/08/13 16:05:32, rsc_swtch wrote:
> > On Windows if you know what the right declarations are it should be
> > easy to implement the macros like for pthread and not have any static
> > initializers. If that matters to you, just let me know what to type.
> > :-)
> 
> :-)
> 
> David suggested this snippet for windows (and using pthread_once_t for
pthread):
> 
> // MSVC ensures that loads of volatile variables have acquire semantics, and
> stores have release semantics.
> 
> typedef LONG volatile  OnceType;
> enum {
>   ONCE_INIT = 0,
>   ONCE_CONSTRUCTING,
>   ONCE_COMPLETED
> };
> void inline OnceInit(OnceType* once, void(*callback)()) {
>   if (*once == ONCE_COMPLETED)  // inlined fast-path
>     return;
>   OnceInitImpl(once, callback);
> 
> }
> ....
> void OnceInitImpl(OnceType* once, void (*callback)()) {
>   OnceType state = reinterpret_cast<OnceType>(InterlockedCompareExchange(once,
> ONCE_CONSTRUCTING, ONCE_INIT));
> 
>   if (state == ONCE_COMPLETED)
>     return;
>   else if (state == ONCE_INIT) { // we're the first thread, run the callback
>     (*callback)();
> 
>     *once = ONCE_COMPLETED;
>   } else while (*once == ONCE_CONSTRUCTING) { // need to wait for completion
>     Sleep(0);  // yield thread on Windows
> 
>   }
> }
> 
> Bug I am happy with the current proposal if David is.
> 
> Best regards,
> Dominic

friendly ping @digit

[digit1, 2012-08-16 16:06:09]

lgtm

Sorry if I wasn't clear. I think this patch is good enough for now. I.e. I
prefer that proper support for the Windows global mutex be added in another
patch, which will make it easier to review (it's after all quite tricky).

(Note that I work on Chrome for Android, which is in no way related to Windows
:-))

[rsc_swtch, 2012-08-21 14:16:09]

Sorry, I was away last week at the last minute. Will submit this now.

[rsc, 2012-08-21 14:35:31]

*** Submitted as http://code.google.com/p/re2/source/detail?r=401ab4168e8e ***

re2: eliminate global c++ constructors

R=battre, battre, digit, rsc, digit
CC=re2-dev
http://codereview.appspot.com/6442107