[PATCH] compat: Allow static initializer for pthreads on Windows

[PATCH] compat: Allow static initializer for pthreads on Windows

[Stefan Beller]

In Windows it is not possible to have a static initialized mutex as of
now, but that seems to be painful for the upcoming refactoring of the
attribute subsystem, as we have no good place to put the initialization
of the attr global lock.

The trick is to get a named mutex as CreateMutex[1] will return the
existing named mutex if it exists in a thread safe way, or return
a newly created mutex with that name.

Inside the critical section of the single named mutex, we need to double
check if the mutex was already initialized because the outer check is
not sufficient.
(e.g. 2 threads enter the first condition `(!a)` at the same time, but
only one of them will acquire the named mutex first and proceeds to
initialize the given mutex a. The second thread shall not re-initialize
the given mutex `a`, which is why we have the inner condition on `(!a)`.

Due to the use of memory barriers inside the critical section the mutex
`a` gets updated to other threads, such that any further invocation
will skip the initialization check code altogether on the first condition.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682411(v=vs.85).aspx

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Flying blind here, i.e. not compiled, not tested. For a system I do not
 have deep knowledge of. The only help was the online documentation.

 compat/win32/pthread.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 1c16408..a900513 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -21,9 +21,25 @@
 static inline int return_0(int i) {
 	return 0;
 }
+
+#define PTHREAD_MUTEX_INITIALIZER NULL
 #define pthread_mutex_init(a,b) return_0((InitializeCriticalSection((a)), 0))
 #define pthread_mutex_destroy(a) DeleteCriticalSection((a))
-#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_lock(a) \
+{ \
+	if (!a) { \
+		HANDLE p = CreateMutex(NULL, FALSE, "Git-Global-Windows-Mutex"); \
+		EnterCriticalSection(p); \
+		MemoryBarrier(); \
+		if (!a)
+			pthread_mutex_init(a); \
+		MemoryBarrier(); \
+		ReleaseMutex(p); \
+	} \
+	EnterCriticalSection(a); \
+}
+
+
 #define pthread_mutex_unlock LeaveCriticalSection
 
 typedef int pthread_mutexattr_t;
-- 
2.10.1.508.g6572022

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Stefan Beller]

On Wed, Oct 26, 2016 at 2:57 PM, Stefan Beller <sbeller@google.com> wrote:
> In Windows it is not possible to have a static initialized mutex as of
> now, but that seems to be painful for the upcoming refactoring of the
> attribute subsystem, as we have no good place to put the initialization
> of the attr global lock.
>
> The trick is to get a named mutex as CreateMutex[1] will return the
> existing named mutex if it exists in a thread safe way, or return
> a newly created mutex with that name.
>
> Inside the critical section of the single named mutex, we need to double
> check if the mutex was already initialized because the outer check is
> not sufficient.
> (e.g. 2 threads enter the first condition `(!a)` at the same time, but
> only one of them will acquire the named mutex first and proceeds to
> initialize the given mutex a. The second thread shall not re-initialize
> the given mutex `a`, which is why we have the inner condition on `(!a)`.
>
> Due to the use of memory barriers inside the critical section the mutex
> `a` gets updated to other threads, such that any further invocation
> will skip the initialization check code altogether on the first condition.
>
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682411(v=vs.85).aspx
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  Flying blind here, i.e. not compiled, not tested. For a system I do not
>  have deep knowledge of. The only help was the online documentation.

This is of course a Double Check Locking Pattern, that Johannes warned about
a couple of days ago. However according to
https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
we can make it work by adding enough memory barriers, (See section
"Making it work with explicit memory barriers").

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 26.10.2016 um 23:57 schrieb Stefan Beller:
> In Windows it is not possible to have a static initialized mutex as of
> now, but that seems to be painful for the upcoming refactoring of the
> attribute subsystem, as we have no good place to put the initialization
> of the attr global lock.

Please rewrite the attribute system such that it can have a dynamic 
initialization. If you find a global initialization in main() too gross 
(I would agree) then setup_git_directory() might be the right place.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 26.10.2016 um 23:57 schrieb Stefan Beller:
>> In Windows it is not possible to have a static initialized mutex as of
>> now, but that seems to be painful for the upcoming refactoring of the
>> attribute subsystem, as we have no good place to put the initialization
>> of the attr global lock.
>
> Please rewrite the attribute system such that it can have a dynamic
> initialization. If you find a global initialization in main() too
> gross (I would agree) then setup_git_directory() might be the right
> place.

As many codepaths may not even need access to the attributes, I
doubt that would be a very productive direction to go.

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 27.10.2016 um 08:02 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 26.10.2016 um 23:57 schrieb Stefan Beller:
>>> In Windows it is not possible to have a static initialized mutex as of
>>> now, but that seems to be painful for the upcoming refactoring of the
>>> attribute subsystem, as we have no good place to put the initialization
>>> of the attr global lock.
>>
>> Please rewrite the attribute system such that it can have a dynamic
>> initialization. If you find a global initialization in main() too
>> gross (I would agree) then setup_git_directory() might be the right
>> place.
>
> As many codepaths may not even need access to the attributes, I
> doubt that would be a very productive direction to go.

So, what is productive then? Pessimizing one (not exactly minor) platform?

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> As many codepaths may not even need access to the attributes, I
>> doubt that would be a very productive direction to go.
>
> So, what is productive then? Pessimizing one (not exactly minor) platform?

Lazy on-demand initialization as needed, perhaps?  The on-demand
initialization mechanism may become no-op on some platforms that can
do static initialization.

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 27.10.2016 um 08:21 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> As many codepaths may not even need access to the attributes, I
>>> doubt that would be a very productive direction to go.
>>
>> So, what is productive then? Pessimizing one (not exactly minor) platform?
>
> Lazy on-demand initialization as needed, perhaps?  The on-demand
> initialization mechanism may become no-op on some platforms that can
> do static initialization.

This is the pessimization that I am talking about. I would not mind at 
all if it were only for the attribute subsystem, but the proposed patch 
would pessimize *all* uses of pthread_mutex_lock.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> As many codepaths may not even need access to the attributes, I
>>> doubt that would be a very productive direction to go.
>>
>> So, what is productive then? Pessimizing one (not exactly minor) platform?
>
> Lazy on-demand initialization as needed, perhaps?  The on-demand
> initialization mechanism may become no-op on some platforms that can
> do static initialization.

Ah, I think I misunderstood your "please rewrite".  Did you mean to
add "void attr_start(void)" helper function to attr.c that does
series of pthread_mutex_init() calls as needed?  That function can
be called from main() of platforms that cannot statically initialize
mutices, while on other platforms it can be a no-op as long as the
variables are statically initialized?  If so, that would not pessimize
any platform, I would think.


    

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Stefan Beller]

> Johannes Sixt <j6t@kdbg.org> writes:
> This is the pessimization that I am talking about. I would not mind at all if
> it were only for the attribute subsystem, but the proposed patch would
> pessimize *all* uses of pthread_mutex_lock.

It would only pessimize *uninitialized* mutexes? For initialized mutexes
the added burden is super cheap (one additional condition).

The positive aspect of this way the patch proposes would be that any
future contributor not knowing the details of how to do mutexes right
on Windows, would not totally break the whole system, i.e. this seems
to be more maintainable in the future as it reduces the friction between
pthreads mutexes and the way we can do things in Git in a platform
independent way

On Wed, Oct 26, 2016 at 11:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Lazy on-demand initialization as needed, perhaps?  The on-demand
>> initialization mechanism may become no-op on some platforms that can
>> do static initialization.
>
> Ah, I think I misunderstood your "please rewrite".  Did you mean to
> add "void attr_start(void)" helper function to attr.c that does
> series of pthread_mutex_init() calls as needed?

Well one init for now.

>  That function can
> be called from main() of platforms that cannot statically initialize
> mutices,

By main you mean the main() in common-main.c or cmd_main in git.c ?

Those both look like the wrong place. Of course it would work adding it
there, but it smells like a maintenance nightmare.

And then we would modify the attr_start command depending on the
platform, i.e.

#ifdef WIN32
void attr_start(void)
{
    pthread_mutex_init(..);
}
#else
void attr_start(void)
{
    /* nothing as it is statically init'd */
}
#endif

> while on other platforms it can be a no-op as long as the
> variables are statically initialized?  If so, that would not pessimize
> any platform, I would think.

I would think this pessimizes ALL platforms from a maintenance perspective
(but what do I know about maintaining stuff as an eager young contributor ;)

So I am willing to go that route, though I do not quite understand where exactly
you'd expect me to put the initializer as all places I can think of
are not the right
place.

Thanks,
Stefan

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Jeff King]

On Thu, Oct 27, 2016 at 10:01:02AM -0700, Stefan Beller wrote:

> >  That function can
> > be called from main() of platforms that cannot statically initialize
> > mutices,
> 
> By main you mean the main() in common-main.c or cmd_main in git.c ?

Any setup or initialization for library functions needs to go in main()
of common-main.c.  Otherwise, only builtins get it, and external
programs are left to segfault (or whatever).

> Those both look like the wrong place. Of course it would work adding it
> there, but it smells like a maintenance nightmare.

I agree it's ugly, but I suspect it would work OK in practice.
We don't have that many mutexes and initializing them is cheap.

Languages like C++ have non-constant initializers, and the compiler
takes care of running the startup code before main() begins. There's no
portable way to do that in C, but our cmd_main() is roughly the same
thing.

I still prefer the lazy initialization if it can work without incurring
measurable overhead. That's the normal way to do things in C; the only
complication here is the thread safety.

-Peff

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>> Johannes Sixt <j6t@kdbg.org> writes:
>> This is the pessimization that I am talking about. I would not mind at all if
>> it were only for the attribute subsystem, but the proposed patch would
>> pessimize *all* uses of pthread_mutex_lock.
>
> It would only pessimize *uninitialized* mutexes? For initialized mutexes
> the added burden is super cheap (one additional condition).

It is not possible to mark a mutex uninitialized on Windows without an 
extra piece of data. A solution would become quite complicated quite 
quickly, and at the cost of additional operations that are in the same 
ballpark as an uncontended mutex. I'm not enthused.

> The positive aspect of this way the patch proposes would be that any
> future contributor not knowing the details of how to do mutexes right
> on Windows, would not totally break the whole system, i.e. this seems
> to be more maintainable in the future as it reduces the friction between
> pthreads mutexes and the way we can do things in Git in a platform
> independent way

This is a non-argument. Coders have to know their tools.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
> ...
> It is not possible to mark a mutex uninitialized on Windows without an
> extra piece of data. A solution would become quite complicated quite
> quickly, and at the cost of additional operations that are in the same
> ballpark as an uncontended mutex. I'm not enthused.
>
>> The positive aspect of this way the patch proposes would be that any
>> future contributor not knowing the details of how to do mutexes right
>> on Windows, would not totally break the whole system, i.e. this seems
>> to be more maintainable in the future as it reduces the friction between
>> pthreads mutexes and the way we can do things in Git in a platform
>> independent way
>
> This is a non-argument. Coders have to know their tools.

The codebase should strive to give coders a coherent abstraction
that can be implemented efficiently on platforms, so that coders do
not have to care too deeply about quirks that exist on individual
platforms.

It is OK to argue that the particular solution Stefan lifted from
somewhere (perhaps msdn article he cited???) does not qualify as
such an abstraction.  But I do not agree with your "Coders have to
know" as a blanket statement.

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Stefan Beller]

On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>> ...
>> It is not possible to mark a mutex uninitialized on Windows without an
>> extra piece of data. A solution would become quite complicated quite
>> quickly, and at the cost of additional operations that are in the same
>> ballpark as an uncontended mutex. I'm not enthused.
>>
>>> The positive aspect of this way the patch proposes would be that any
>>> future contributor not knowing the details of how to do mutexes right
>>> on Windows, would not totally break the whole system, i.e. this seems
>>> to be more maintainable in the future as it reduces the friction between
>>> pthreads mutexes and the way we can do things in Git in a platform
>>> independent way
>>
>> This is a non-argument. Coders have to know their tools.

Windows is not my tool.

>
> The codebase should strive to give coders a coherent abstraction
> that can be implemented efficiently on platforms, so that coders do
> not have to care too deeply about quirks that exist on individual
> platforms.

Currently working as a coder I care about "submodules, that work on
linux." I do not care about Windows in the big picture. I am however
willing to go an extra step to not break Windows. However that requires
a little bit of effort from both me and you:
* I need to be aware of what I cannot do with "not-my-tools". (So please
  somebody tell me, also in the future when I break obscure platforms. Mind
  that I don't equate obscure with not widely used or in any other way negative.
  It's just that people working on linux find some quirks on Windows
not "obvious")
* the workaround should not be too time consuming in the bigger picture,
  which is why I propose to make the API a bit clearer by emulating posix
  mutexes harder. (From a Windows POV this might sound like making it
  more obscure because posix mutexes itself are obscure.)

>
> It is OK to argue that the particular solution Stefan lifted from
> somewhere (perhaps msdn article he cited???)

A stack overflow article that I found with my search engine of choice, because
I could not believe that Windows cannot have statically initialized mutexes.

> does not qualify as
> such an abstraction.

The implementation under discussion (well we did not discuss the
implementation a
whole lot yet) may even contain an error as the first memory barrier
needs to be in front
of the first condition.

>  But I do not agree with your "Coders have to
> know" as a blanket statement.

Well I do to some extent, I just disagree what my tools are.

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 27.10.2016 um 21:08 schrieb Stefan Beller:
> On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>>> ...
>>> It is not possible to mark a mutex uninitialized on Windows without an
>>> extra piece of data. A solution would become quite complicated quite
>>> quickly, and at the cost of additional operations that are in the same
>>> ballpark as an uncontended mutex. I'm not enthused.
>>>
>>>> The positive aspect of this way the patch proposes would be that any
>>>> future contributor not knowing the details of how to do mutexes right
>>>> on Windows, would not totally break the whole system, i.e. this seems
>>>> to be more maintainable in the future as it reduces the friction between
>>>> pthreads mutexes and the way we can do things in Git in a platform
>>>> independent way
>>>
>>> This is a non-argument. Coders have to know their tools.
>
> Windows is not my tool.

The tool I meant is pthreads. For example, you can't have a 
pthread_mutex_t variable and not initialize it with either 
PTHREAD_MUTEX_INITIALIZER or pthread_mutex_init.

>> The codebase should strive to give coders a coherent abstraction
>> that can be implemented efficiently on platforms, so that coders do
>> not have to care too deeply about quirks that exist on individual
>> platforms.
>
> Currently working as a coder I care about "submodules, that work on
> linux." I do not care about Windows in the big picture.

I don't know what you meant to say with this sentence, but taken 
literally, are you on the right ship here, I have to ask?

> I am however
> willing to go an extra step to not break Windows.

"Not break Windows" is equivalent to "make it work on Windows", mind 
you. We can't have a new feature only on Linux when there is no reason 
not to have it on Windows as well. Sorry, "Git is for Unix only" is long 
over.

> However that requires
> a little bit of effort from both me and you:
> * I need to be aware of what I cannot do with "not-my-tools". (So please
>   somebody tell me, also in the future when I break obscure platforms. Mind
>   that I don't equate obscure with not widely used or in any other way negative.
>   It's just that people working on linux find some quirks on Windows
> not "obvious")

That goes without saying.

> * the workaround should not be too time consuming in the bigger picture,

That, however, is wishful thinking. If you want to have a feature 
dearly, you have to make it work, and that may take its time. I'm not 
saying that *you* have to make it work (there are platform experts 
around to lend a hand), but just an extra step to "not break" an 
important platform is not enough.

>   which is why I propose to make the API a bit clearer by emulating posix
>   mutexes harder. (From a Windows POV this might sound like making it
>   more obscure because posix mutexes itself are obscure.)

So, when you think that POSIX mutexes are obscure, why don't we settle 
on the simpler Windows critical sections? Emulating them with pthreads 
should be child's play.

> The implementation under discussion (well we did not discuss the
> implementation a
> whole lot yet) ...

There's not a whole lot to discuss: it must be rewritten from scratch 
(it's not just the memory barriers, it is everything else, too). But 
time is much better spent on an attr_start() solution.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Jacob Keller]

On Thu, Oct 27, 2016 at 1:05 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> The implementation under discussion (well we did not discuss the
>> implementation a
>> whole lot yet) ...
>
>
> There's not a whole lot to discuss: it must be rewritten from scratch (it's
> not just the memory barriers, it is everything else, too). But time is much
> better spent on an attr_start() solution.
>
> -- Hannes
>

Ok, so I've been reading this thread. I don't understand your
objections to emulating in this way.. Could you clearly spell out why
you believe this solution isn't acceptable? So far all I've understood
was "it's not critical sections" and "it penalizes Windows too much"
but... If Windows cannot statically initialize a pthread mutex, then
we *have* to dynamically initialize it somewhere. This solution adds a
single check before each lock and is safe due to use of memory
barriers. Yes, this will cost a tiny bit extra overhead for each use
of "pthread_mutex_lock" but I fail to see how that is a huge
penalty...

So I'm trying to understand if that really is your only concern,
because I don't actually buy that it is a huge overhead to pay.

I agree with Stefan that there isn't really a great place to put a
dynamic initialization.

To be clear I am trying to understand the objections since so far all
I have read is "this is bad, and I object" without clearly explaining
reasoning.

Regards,
Jake

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Stefan Beller]

> there isn't really a great place to put a dynamic initialization.

Answering this question specifically (Where to put it?),
I am about to send out a patch that puts it in compat/mingw.c:2232:

diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5..9881c3d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2232,6 +2232,9 @@ void mingw_startup(void)
         /* initialize critical section for waitpid pinfo_t list */
         InitializeCriticalSection(&pinfo_cs);

+        /* initialize critical sections in the attr code */
+        start_attr();
+
         /* set up default file mode and file modes for stdin/out/err */
         _fmode = _O_BINARY;
         _setmode(_fileno(stdin), _O_BINARY);

If I understood Johannes correctly this is the place to put it.
Junio seems to be ok with static mutex init for non windows platforms,
so I put it into the mingw specifc startup code.

Thanks,
Stefan

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 27.10.2016 um 23:49 schrieb Jacob Keller:
> Ok, so I've been reading this thread. I don't understand your
> objections to emulating in this way.. Could you clearly spell out why
> you believe this solution isn't acceptable? So far all I've understood
> was "it's not critical sections" and "it penalizes Windows too much"
> but... If Windows cannot statically initialize a pthread mutex, then
> we *have* to dynamically initialize it somewhere. This solution adds a
> single check before each lock and is safe due to use of memory
> barriers. Yes, this will cost a tiny bit extra overhead for each use
> of "pthread_mutex_lock" but I fail to see how that is a huge
> penalty...

One point is that the DCLP idiom must be implemented correctly. There 
are solutions, of course, and when the initialization is over, we have a 
miniscule overhead at each pthread_mutex_lock call.

The main point is that the initialization has to solve a chicken-and-egg 
problem: After we have found an uninitialized critical section, we have 
to have mutual exclusion for the initialization. We need another 
critical section for this, but we cannot have one that is initialized. 
For this reason, the solution uses a different kind of mutual exclusion 
primitive, which is more akin to POSIX semaphores and works across 
processes. In the patch proposed by Stefan, a *session-wide* mutex is 
used. That means that all concurrent git invocations in a user's session 
synchronize their initialization of critical section objects.

That's just ridiculous. It's like waiting for a ... no, *the* ... battle 
ship just to get our bouncers in their position. We are talking 
milliseconds here, not nanoseconds.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Jacob Keller]

On Thu, Oct 27, 2016 at 10:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> One point is that the DCLP idiom must be implemented correctly. There are
> solutions, of course, and when the initialization is over, we have a
> miniscule overhead at each pthread_mutex_lock call.
>

Right, this I understood, but appeared to be solved.

> The main point is that the initialization has to solve a chicken-and-egg
> problem: After we have found an uninitialized critical section, we have to
> have mutual exclusion for the initialization. We need another critical
> section for this, but we cannot have one that is initialized. For this
> reason, the solution uses a different kind of mutual exclusion primitive,
> which is more akin to POSIX semaphores and works across processes. In the
> patch proposed by Stefan, a *session-wide* mutex is used. That means that
> all concurrent git invocations in a user's session synchronize their
> initialization of critical section objects.
>

Thank you for explaining this. Now I understand why this would be
considered a big issue, and potentially worth considering the
alternatives like attr_start. This was missing since I don't think any
of the rest of us knew (correct me if I am wrong) that the
synchronization would be global. For many cases it's probably not that
bad, but we do have a suitable explanation, and I think living with
"attr_start()" in the win32 initialization path which is something
Stefan suggested in a previous email would make the most sense then.

> That's just ridiculous. It's like waiting for a ... no, *the* ... battle
> ship just to get our bouncers in their position. We are talking milliseconds
> here, not nanoseconds.
>

Right. Thanks for filling in the missing details.

Regards,
Jake

> -- Hannes
>

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Schindelin]

Hi Jake,

On Thu, 27 Oct 2016, Jacob Keller wrote:

> I agree with Stefan that there isn't really a great place to put a
> dynamic initialization.

Ummm. Wait. What???

https://github.com/git/git/blob/v2.10.1/common-main.c#L25-L41

Ciao,
Johannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Philip Oakley]

From: "Johannes Sixt" <j6t@kdbg.org>
>
> One point is that the DCLP idiom must be implemented correctly. There are 
> solutions, of course, and when the initialization is over, we have a 
> miniscule overhead at each pthread_mutex_lock call.
>

I had to look up DCLP ( = Double Checked Locking Patterns), and found a good 
write up on the issues..

http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf "C++ and the 
Perils of Double-Checked Locking", which include 'C' issues, and 
multi-thread, multi-processor issues. Not an easy issue when fighting 
optimisers..

--

Philip 

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Jacob Keller]

On Fri, Oct 28, 2016 at 4:58 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Thu, 27 Oct 2016, Jacob Keller wrote:
>
>> I agree with Stefan that there isn't really a great place to put a
>> dynamic initialization.
>
> Ummm. Wait. What???
>
> https://github.com/git/git/blob/v2.10.1/common-main.c#L25-L41
>
> Ciao,
> Johannes

I think Stefan has since provided a better suggestion of isolating the
dynamic initialization into the MINGW startup section. However, you
are right either place works, though I think platforms which support
static initialization should use it.

Thanks,
Jake

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Jacob Keller]

On Fri, Oct 28, 2016 at 6:01 AM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Johannes Sixt" <j6t@kdbg.org>
>>
>>
>> One point is that the DCLP idiom must be implemented correctly. There are
>> solutions, of course, and when the initialization is over, we have a
>> miniscule overhead at each pthread_mutex_lock call.
>>
>
> I had to look up DCLP ( = Double Checked Locking Patterns), and found a good
> write up on the issues..
>
> http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf "C++ and the
> Perils of Double-Checked Locking", which include 'C' issues, and
> multi-thread, multi-processor issues. Not an easy issue when fighting
> optimisers..
>
> --
>
> Philip


Yep, this is why we have memory barriers. Ofcourse languages like C
don't really allow you to express them in the language and we restore
to various platform specific methods.

Thanks,
Jake

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 28.10.2016 um 20:48 schrieb Jacob Keller:
> On Fri, Oct 28, 2016 at 4:58 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Jake,
>>
>> On Thu, 27 Oct 2016, Jacob Keller wrote:
>>
>>> I agree with Stefan that there isn't really a great place to put a
>>> dynamic initialization.
>>
>> Ummm. Wait. What???
>>
>> https://github.com/git/git/blob/v2.10.1/common-main.c#L25-L41
>
> I think Stefan has since provided a better suggestion of isolating the
> dynamic initialization into the MINGW startup section. However, you
> are right either place works, though I think platforms which support
> static initialization should use it.

It's a pity that this new patch introduces the first use of 
PTHREAD_MUTEX_INITILIZER. (I do not count the one in compat/nedmalloc, 
it's in platform specific code.) We have to introduce a dummy definition 
on Windows to even compile the code.

Now we have a double-edged sword: Other coders who are not aware of this 
thread might think it works. But it does not.

Another problem with the proposed patch is that there is no declaration 
for attr_start() before the call in compat/mingw.c. We would have to 
move the declaration of attr_start() to cache.h (for example), because 
#including attr.h in compat/mingw.c is plainly wrong. However, it would 
not be a major offense to #include attr.h in common-main.c. But when we 
do that, we can certainly spare the few cycles to call pthread_mutex_init.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Another problem with the proposed patch is that there is no
> declaration for attr_start() before the call in compat/mingw.c. We
> would have to move the declaration of attr_start() to cache.h (for
> example), because #including attr.h in compat/mingw.c is plainly
> wrong. However, it would not be a major offense to #include attr.h in
> common-main.c. But when we do that, we can certainly spare the few
> cycles to call pthread_mutex_init.

That sounds like a good argument to have it in common-main.c.  

Would it mean that the code that defines the mutex needs to have
#ifdef that defines a no-op attr_start() and defines the mutex with
PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
without initializatin, with the real attr_start(), though?

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Stefan Beller]

On Fri, Oct 28, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Another problem with the proposed patch is that there is no
>> declaration for attr_start() before the call in compat/mingw.c. We
>> would have to move the declaration of attr_start() to cache.h (for
>> example), because #including attr.h in compat/mingw.c is plainly
>> wrong. However, it would not be a major offense to #include attr.h in
>> common-main.c. But when we do that, we can certainly spare the few
>> cycles to call pthread_mutex_init.
>
> That sounds like a good argument to have it in common-main.c.

If we're going that route, I would get rid of PTHREAD_MUTEX_INITILIZER
and call a pthread_mutex_init platform independently.

>
> Would it mean that the code that defines the mutex needs to have
> #ifdef that defines a no-op attr_start() and defines the mutex with
> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
> without initializatin, with the real attr_start(), though?

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Oct 28, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Another problem with the proposed patch is that there is no
>>> declaration for attr_start() before the call in compat/mingw.c. We
>>> would have to move the declaration of attr_start() to cache.h (for
>>> example), because #including attr.h in compat/mingw.c is plainly
>>> wrong. However, it would not be a major offense to #include attr.h in
>>> common-main.c. But when we do that, we can certainly spare the few
>>> cycles to call pthread_mutex_init.
>>
>> That sounds like a good argument to have it in common-main.c.
>
> If we're going that route, I would get rid of PTHREAD_MUTEX_INITILIZER
> and call a pthread_mutex_init platform independently.

Yup, but earlier j6t was trying hard not to penalize any single
platform, and that would certainly penalize the ones with static
initialization, so I was hoping to hear if there is a clever
workaround to avoid that.

>> Would it mean that the code that defines the mutex needs to have
>> #ifdef that defines a no-op attr_start() and defines the mutex with
>> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
>> without initializatin, with the real attr_start(), though?

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Johannes Sixt]

Am 28.10.2016 um 22:29 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Another problem with the proposed patch is that there is no
>> declaration for attr_start() before the call in compat/mingw.c. We
>> would have to move the declaration of attr_start() to cache.h (for
>> example), because #including attr.h in compat/mingw.c is plainly
>> wrong. However, it would not be a major offense to #include attr.h in
>> common-main.c. But when we do that, we can certainly spare the few
>> cycles to call pthread_mutex_init.
>
> That sounds like a good argument to have it in common-main.c.
>
> Would it mean that the code that defines the mutex needs to have
> #ifdef that defines a no-op attr_start() and defines the mutex with
> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
> without initializatin, with the real attr_start(), though?

No. My intent was to call pthread_mutex_init for all platforms. We 
already call open("/dev/null") unconditionally on every program startup. 
The few cycles spent in pthread_mutex_init should be the least of our 
worries.

-- Hannes

Re: [PATCH] compat: Allow static initializer for pthreads on Windows

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 28.10.2016 um 22:29 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Another problem with the proposed patch is that there is no
>>> declaration for attr_start() before the call in compat/mingw.c. We
>>> would have to move the declaration of attr_start() to cache.h (for
>>> example), because #including attr.h in compat/mingw.c is plainly
>>> wrong. However, it would not be a major offense to #include attr.h in
>>> common-main.c. But when we do that, we can certainly spare the few
>>> cycles to call pthread_mutex_init.
>>
>> That sounds like a good argument to have it in common-main.c.
>>
>> Would it mean that the code that defines the mutex needs to have
>> #ifdef that defines a no-op attr_start() and defines the mutex with
>> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
>> without initializatin, with the real attr_start(), though?
>
> No. My intent was to call pthread_mutex_init for all platforms.

Ah, OK, so there was no magic plan.  That's OK.