Port of pthreads to Windows API threads

---

builtin-pack-objects.c +8 -0

+#ifndef _WIN32
 static void *threaded_find_deltas(void *arg)
+#else
+static DWORD WINAPI threaded_find_deltas(LPVOID arg)
+#endif
...
+#ifndef _WIN32
 	return NULL;
+#else
+	return 0;
+#endif

etc ...

compat/winthread.h new file +6 -0

+	if (NULL == cond->sema)
+		return -1;
+	return 0;
+}
+
+static __inline int win32_cond_destroy(win32_cond_t *cond)

compat/winthread.h new file +5 -0

+
+	if (0 != result)
+		ret = -1;
+
+	/* one waiter less */

compat/winthread.h line 28

Quoted 7 lines · Andrzej K. Haczewski

compat/winthread.h new file +7 -0

+static __inline int win32_cond_init(win32_cond_t *cond)
+{
+     cond->waiters = 0;
+
+     InitializeCriticalSection(&cond->waiters_lock);
+
+     cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);

Wouldn't an Event object be lighter-weight? (I'm only guessing.)

Can't you just use the pthread package that is included in msysgit?

Quoted 8 lines

+     /* we're waiting... */
+     EnterCriticalSection(&cond->waiters_lock);
+     ++cond->waiters;
+     LeaveCriticalSection(&cond->waiters_lock); +
+     /* unlock external mutex and wait for signal */
+     LeaveCriticalSection(mutex);
+     result = WaitForSingleObject(cond->sema, INFINITE);

Releasing the mutex and entering the wait state as well as leaving the wait state and reacquiring the mutex should be atomic. Neither are in this implementation. You are not mentioning why you are implementing things like this and why this would be acceptable.

---

builtin-pack-objects.c +0 -0

index 02f9246..a8a4f59 100644
@@ -18,8 +18,12 @@
 #include "refs.h"
 
 #ifdef THREADED_DELTA_SEARCH

Could you please add the reasoning from the cover letter to this commit message?  And add a sign-off?

It is unlikely that an #ifdef "contamination" of this extent will go through easily, but I have a suggestion that may make your patch both easier to read and more likely to be accepted into git.git: Try to wrap the win32 calls into pthread-compatible function signatures.  Then you can add a compat/win32/pthread.h and not even touch core files of git.git at all.

Oh, and you definitely do not want to copy-paste err_win_to_posix().  You definitely want to reuse the existing instance.

Quoted 10 lines

  #ifdef THREADED_DELTA_SEARCH

builtin-pack-objects.c +6 -2

-#include "thread-utils.h"
-#include<pthread.h>
+# include "thread-utils.h"
+# ifndef _WIN32
+#  include<pthread.h>
+# else
+#  include<winthread.h>
+# endif
  #endif

Quoted 2 lines

It is unlikely that an #ifdef "contamination" of this extent will go through easily, but I have a suggestion that may make your patch both easier to read and more likely to be accepted into git.git: Try to wrap the win32 calls into pthread-compatible function signatures. Then you can add a compat/win32/pthread.h and not even touch core files of git.git at all.

Pardon my ignorance, but is there a reason to not use Pthreads for Win32?
 http://sourceware.org/pthreads-win32/

Quoted 3 lines

Pardon my ignorance, but is there a reason to not use Pthreads for Win32?
http://sourceware.org/pthreads-win32/

Not using pthreads on Windows makes Git:
1. faster on that platform

2. not depend on Pthreads for Win32

IMHO that makes Git one step closer to become native on Windows, and is a sensible step.

----- Original Message -----
From: Johannes Schindelin
Date: 11/3/2009 4:38 PM

Quoted 10 lines

Quoted 10 lines

  #ifdef THREADED_DELTA_SEARCH

builtin-pack-objects.c +6 -2

-#include "thread-utils.h"
-#include<pthread.h>
+# include "thread-utils.h"
+# ifndef _WIN32
+#  include<pthread.h>
+# else
+#  include<winthread.h>
+# endif
  #endif

Quoted 9 lines

Quoted 2 lines

It is unlikely that an #ifdef "contamination" of this extent will go through easily, but I have a suggestion that may make your patch both easier to read and more likely to be accepted into git.git: Try to wrap the win32 calls into pthread-compatible function signatures. Then you can add a compat/win32/pthread.h and not even touch core files of git.git at all.

Pardon my ignorance, but is there a reason to not use Pthreads for Win32? http://sourceware.org/pthreads-win32/

+static THREAD_RET_TYPE threaded_find_deltas(void *arg)

Quoted 2 lines

+     NO_STATIC_PTHREADS_INIT = YesPlease

Let's not go that route please.  If Windows can't get away without runtime initializations then let's use them on all platforms.  There is no gain in exploding the code path combinations here wrt testing coverage.

Quoted 1 line

+static THREAD_RET_TYPE threaded_find_deltas(void *arg)

Why can't you just cast the function pointer in your pthread_create wrapper instead?  No one cares about the returned value anyway.

Quoted 4 lines

builtin-pack-objects.c +0 -0

@@ -2327,6 +2354,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
  #ifdef THREADED_DELTA_SEARCH

      if (!delta_search_threads)      /* --threads=0 means autodetect */ delta_search_threads = online_cpus();

Quoted 2 lines

+
+     init_threaded_delta_search();

What about doing this at the beginning of ll_find_deltas() instead? And similarly for cleanup_threaded_delta_search(): call it right before leaving ll_find_deltas().  This way thread issues would remain more localized.  In fact I'd move the whole thing above in ll_find_deltas() as well (separately from this patch though).

Quoted 4 lines

Quoted 1 line

+static THREAD_RET_TYPE threaded_find_deltas(void *arg)

Why can't you just cast the function pointer in your pthread_create wrapper instead?  No one cares about the returned value anyway.

Because of calling convention - I'd have to cast cdecl function as stdcall function, which would change the function call clean up (in cdecl caller is responsible for unwinding stack, stdcall callee; the effect - double stack unwinding).

Couldn't the windows version of pthread_create have a wrapper function, that corrected the calling convention, much like the function run_thread that start_async in run-command.c has?

Erik Faye-Lund pisze:

Quoted 3 lines

Couldn't the windows version of pthread_create have a wrapper function, that corrected the calling convention, much like the function run_thread that start_async in run-command.c has?

Can't be done without allocations. I'd have to pass to that wrapping thread function an address of original function *and* an original argument, and there's no way to pack that as one void*.

What about:

typedef struct {
       HANDLE handle;
       void *(*start_routine)(void *);
       void *arg;
} pthread_t;

DWORD __stdcall windows_thread_start(LPVOID _self) {
       pthread_t *self = _self;
       void *ret = self->start_routine(self->arg); return (DWORD)ret;
}

static inline int pthread_create(pthread_t *thread, const void *unused, void *(*start_routine)(void *), void *arg) {
       thread->handle = CreateThread(NULL, 0, windows_thread_start, thread, 0, NULL);
       [...]
}

And thread creation is a relatively rare event compared to e.g. mutex lock/unlock, so the indirection shouldn't be noticeable.  For the same reason, I also think that you could make pthread_create() and pthread_join() into a C file instead of being inlined which would reduce the code footprint at every call site, and allow for only one instance of windows_thread_start() which could then be made static.

Makefile +2 -1

index bc039ac..30089a8 100644
@@ -453,6 +453,7 @@ LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
+LIB_H += compat/win32/pthread.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h

@@ -971,15 +972,15 @@ ifdef MSVC
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	NO_CURL = YesPlease
-	NO_PTHREADS = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 	BLK_SHA1 = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl

Makefile +59 -0

+static inline int pthread_cond_init(pthread_cond_t *cond, const void *unused)
+{
+	cond->waiters = 0;
+
+	InitializeCriticalSection(&cond->waiters_lock);
+
+	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
+	if (!cond->sema)
+		die("");
+	return 0;
+}
+
+static inline int pthread_cond_destroy(pthread_cond_t *cond)
+{
+	CloseHandle(cond->sema);
+	cond->sema = NULL;
+
+	DeleteCriticalSection(&cond->waiters_lock);
+
+	return 0;
+}
+
+static inline int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
+{
+	/* serialize access to waiters count */
+	EnterCriticalSection(&cond->waiters_lock);
+	++cond->waiters;
+	LeaveCriticalSection(&cond->waiters_lock);
+
+	/*
+	 * Unlock external mutex and wait for signal.
+	 * NOTE: we've held mutex locked long enough to increment
+	 * waiters count above, so there's no problem with
+	 * leaving mutex unlocked before we wait on semaphore.
+	 */
+	LeaveCriticalSection(mutex);
+
+	/* let's wait */
+	WaitForSingleObject(cond->sema, INFINITE))
+
+	/* we're done waiting, so make sure we decrease waiters count */
+	EnterCriticalSection(&cond->waiters_lock);
+	--cond->waiters;
+	LeaveCriticalSection(&cond->waiters_lock);
+
+	/* lock external mutex again */
+	EnterCriticalSection(mutex);
+
+	return 0;
+}
+
+static inline int pthread_cond_signal(pthread_cond_t *cond)
+{
+	int have_waiters;
+
+	/* serialize access to waiters count */
+	EnterCriticalSection(&cond->waiters_lock);
+	have_waiters = cond->waiters > 0;
+	LeaveCriticalSection(&cond->waiters_lock);

Elsewhere we use _beginthreadex(). What's the difference?

The pthread_cond_* functions are quite voluminous, but not performance critical. Could you please move them to pthread.c as well?

That way we don't need allocations to simulate pthread init/join API

2009/11/5 Nicolas Pitre <[email protected]>:

Quoted 17 lines · Andrzej K. Haczewski

What about:

typedef struct {
       HANDLE handle;
       void *(*start_routine)(void *);
       void *arg;
} pthread_t;

DWORD __stdcall windows_thread_start(LPVOID _self) {
       pthread_t *self = _self;
       void *ret = self->start_routine(self->arg); return (DWORD)ret;
}

Quoted 7 lines

static inline int pthread_create(pthread_t *thread, const void *unused, void *(*start_routine)(void *), void *arg) {
       thread->handle = CreateThread(NULL, 0, windows_thread_start, thread, 0, NULL);
       [...]
}

The problem I see is not with pthread_init, but pthread_join. Here's how it looks:

int pthread_join(pthread_t thread, void **value_ptr);

If pthread_t would be a struct, then we can't call pthread_join like that...

At least that's what I though yesterday, but maybe it can be done like this:

int win32_pthread_join(pthread_t *thread, void **value_ptr)

{
        [...]
}

#define pthread_join(a, b) win32_pthread_join(&(a), (b))

That way we don't need allocations to simulate pthread init/join API

Why not?  At least gcc is quite happy with such a construct.  It probably makes a copy of the stack before passing it though.

2009/11/5 Nicolas Pitre <[email protected]>:

Quoted 2 lines

Why not?  At least gcc is quite happy with such a construct.  It probably makes a copy of the stack before passing it though.

Err... my mind is rotted with all that ugly java, c#, python and ruby, even c++. I should start taking some medications I suppose... what was I thinking is that C can't copy-construct a struct. Damn, too much sun (of java fame)...

That way I rediscovered simple struct construct...man I missed C so much :)

Anyway, the solution with passing pthread_t as pointer saves some stack, so it's probably not that bad.

This patch implements native to Windows subset of pthreads API used by Git. It allows to remove Pthreads for Win32 dependency for msysgit and cygwin.

The patch modifies Makefile only for MSVC (that's the environment I'm capable of testing on), so it requires further corrections to compile with MinGW or Cygwin.

Careful.  At the beginning of the function you'll find:

       if (delta_search_threads <= 1) {
               find_deltas(list, &list_size, window, depth, processed); return;
       }

That is, if we have thread support compiled in but we're told to use only one thread, then the bulk of the work splitting is bypassed entirely.  Inside find_deltas() there will still be pthread_mutex_lock() and pthread_mutex_unlock() calls even if no threads are spawned.

2009/11/5 Nicolas Pitre <[email protected]>:

Quoted 7 lines

Careful.  At the beginning of the function you'll find:

       if (delta_search_threads <= 1) {
               find_deltas(list, &list_size, window, depth, processed); return;
       }

Quoted 4 lines

That is, if we have thread support compiled in but we're told to use only one thread, then the bulk of the work splitting is bypassed entirely.  Inside find_deltas() there will still be pthread_mutex_lock() and pthread_mutex_unlock() calls even if no threads are spawned.

Ah, I wasn't aware of that. Actually why would find_deltas lock if no threads are used? Maybe, for non-threaded call to find_deltas, locking could be factored out?

Please use die("CreateSemaphore() failed") in the failure case instead of returning success.

However, my pthread_cond_init man page says:

Quoted 1 line

However, my pthread_cond_init man page says:

And that is weird, because mine man page says:
[[[
pthread_cond_init, pthread_cond_signal, pthread_cond_broadcast, and pthread_cond_wait never return an error code.
]]]

2009/11/4 Nicolas Pitre <[email protected]>:

Quoted 12 lines

Quoted 6 lines

(quoted diff) +0 -0

@@ -2327,6 +2354,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
  #ifdef THREADED_DELTA_SEARCH

      if (!delta_search_threads)      /* --threads=0 means autodetect */ delta_search_threads = online_cpus();
+
+     init_threaded_delta_search();

What about doing this at the beginning of ll_find_deltas() instead? And similarly for cleanup_threaded_delta_search(): call it right before leaving ll_find_deltas().  This way thread issues would remain more localized.  In fact I'd move the whole thing above in ll_find_deltas() as well (separately from this patch though).

Let's keep thread stuff close together if possible. And in this case, this even reduces the #ifdef noise, and allows for skipping the autodetection altogether if delta search is not needed (like with a pure clone).

2009/11/4 Nicolas Pitre <[email protected]>:

Quoted 4 lines · Andrzej K. Haczewski

Quoted 2 lines

+     NO_STATIC_PTHREADS_INIT = YesPlease

Quoted 5 lines

Let's not go that route please.  If Windows can't get away without runtime initializations then let's use them on all platforms.  There is no gain in exploding the code path combinations here wrt testing coverage.

I don't like that approach either, but I was frighten of Junio being anal about static inits ;).

Let's make it clear: has anyone have any objections that I add explicit initialization of mutexes and condition variables for POSIX also?

Makefile ifdef MSVC · line 943

Quoted 1 line · Andrzej K. Haczewski

Makefile +1 -0

+	NO_STATIC_PTHREADS_INIT = YesPlease

Let's not go that route please. If Windows can't get away without runtime initializations then let's use them on all platforms. There is no gain in exploding the code path combinations here wrt testing coverage.

+

Seeing how THREAD_RETURN is only called with NULL-parameter, perhaps it would be better to do this?

git-compat-util.h +2 -0

+/*
+ * Properly defines thread routine for Windows and POSIX

git-compat-util.h void git_qsort(void *base, size_t nmemb, size_t size, · line 479

Quoted 13 lines · Andrzej K. Haczewski

git-compat-util.h +13 -0

+/*
+ * Properly defines thread routine for Windows and POSIX
+ */
+#ifndef NO_PTHREADS
+# ifndef _WIN32
+#  define THREAD_FUNC(f, a) void *f(void *a)
+#  define THREAD_RETURN(x) return (x)
+# else
+#  define THREAD_FUNC(f, a) DWORD __stdcall f(LPVOID a)
+#  define THREAD_RETURN(x) return (DWORD)(x);
+# endif
+#endif
+

Seeing how THREAD_RETURN is only called with NULL-parameter, perhaps it would be better to do this?

git-compat-util.h +12 -0

+/*
+ * Properly defines thread routine for Windows and POSIX
+ */
+#ifndef NO_PTHREADS
+# ifndef _WIN32
+#  define THREAD_FUNC(f, a) void *f(void *a)
+#  define THREAD_RETURN() return NULL;
+# else
+#  define THREAD_FUNC(f, a) DWORD __stdcall f(LPVOID a)
+#  define THREAD_RETURN() return 0;
+# endif
+#endif

Quoted 1 line

+

Here is slightly modified patch with more comments where explanations were requested (ie. non atomic release mutex and wait).

The implementation of conditional variable is based on ACE.

The patch needs testing from someone capable of compiling Git on Windows and running it with msysgit environment. I can confirm that it compiles cleanly on both Linux and Windows. I modified Makefile only for MSVC part, so if you'd like to compile it with mingw or cygwin, proper corrections have to be made. I aim for native MSVC compilation, that's why I did it like that. That's also the reason I don't like having Pthreads for Win32 dependency - it's faster to use native calls than depend on 3rd party wrapper library to do it for you (ie. pthreads for win32 does allocations to implement POSIX standard, and full-conformance isn't required by Git, since Git uses only small subset of pthreads).

One more motivation I had for the patch: as I was reading through archives I had a feeling that Git aims to be as lightweight as possible, hence removing additional dependencies (even for Windows platform) seems sensible to me.

Signed-off-by Andrzej K. Haczewski <[email protected]>

builtin-pack-objects.c +1 -0

@@ -2327,6 +2327,18 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 #ifdef THREADED_DELTA_SEARCH
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
+

builtin-pack-objects.c +6 -0

+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <[email protected]>
+ */
+ 
+#ifndef PTHREAD_H
+#define PTHREAD_H

Please do not cull Cc list when you resend a patch, if possible.

After staring some time on the code, I have convinced myself that the pthread_cond_wait and pthread_cond_signal implementation will work *in our usage scenario* that has these preconditions:

- pthread_cond_signal is called while the mutex is held.

- We retest the condition after pthread_cond_wait returns.

These conditions should be attached in BIG BOLD letters to this implementation; particularly, the last one.

The subject is a bit misleading, IMHO. You are not porting the (p)threading code, but you are adding pthread_* function wrappers for Windows.

Your patch adds whitespace-at-eol. Please use git show --check to see where.

Please drop words from the commit message that do not make sense once this commit is in git's history. Look at existing commit messages to get a feeling for the style. Do write about "why" (motivation), "how" (design choices) and "how not" (dead ends that you tried).

I think it would be OK to drop '= PTHREAD_{MUTEX,COND}_INITIALIZER' and use *_init function calls without the #ifdef. Likewise for *_destroy.

-- Hannes

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 1 line

- pthread_cond_signal is called while the mutex is held.

AFAIK that is a requirement for condition variable to be signaled while holding the same mutex that other threads cond_wait on. I just don't check that it is true, because Git is locking mutex.

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 3 lines

After staring some time on the code, I have convinced myself that the pthread_cond_wait and pthread_cond_signal implementation will work *in our usage scenario* that has these preconditions:

But it is not impossible with that implementation.

On resubmit I'll give more credit to ACE.

Quoted 1 line

- pthread_cond_signal is called while the mutex is held.

AFAIK that is a requirement for condition variable to be signaled while holding the same mutex that other threads cond_wait on. I just don't check that it is true, because Git is locking mutex.

Quoted 4 lines

- We retest the condition after pthread_cond_wait returns.

These conditions should be attached in BIG BOLD letters to this implementation; particularly, the last one.

That's also a known requirement for working with cond vars.

Quoted 2 lines

I think it would be OK to drop '= PTHREAD_{MUTEX,COND}_INITIALIZER' and use *_init function calls without the #ifdef. Likewise for *_destroy.

Actually it won't save us many #ifdefs. There's one #ifdef for initialization that could be saved, but then comes #ifdef for cleanup:
#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)

What you propose will remove one #ifdef _WIN32 for initialization, but the cleanup will look almost the same:
#ifdef THREADED_DELTA_SEARCH

You are right. But #ifdef THREADED_DELTA_SEARCH is about a "generic" property of the code and is already used elsewhere in the file, whereas #ifdef WIN32 would be new and is is about platform differences.

Anyway, we would have to see what Junio says about the new function calls, because he's usually quite anal when it comes to added code vs. static initialization. ;)

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 8 lines

You are right. But #ifdef THREADED_DELTA_SEARCH is about a "generic" property of the code and is already used elsewhere in the file, whereas #ifdef WIN32 would be new and is is about platform differences.

Anyway, we would have to see what Junio says about the new function calls, because he's usually quite anal when it comes to added code vs. static initialization. ;)

I could do it with wrappers for pthread_mutex_lock and _unlock and lazy init there plus lazy init cond var in cond_wait and _signal, that way it could be done without any additional code in the first #ifdef. But I don't see any simple solution for working around deinitialization, that's why I'd leave non-static initialization. Let me put some touchups and resubmit for another round.

Quoted 10 lines · Andrzej K. Haczewski

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 8 lines

You are right. But #ifdef THREADED_DELTA_SEARCH is about a "generic" property of the code and is already used elsewhere in the file, whereas #ifdef WIN32 would be new and is is about platform differences.

Anyway, we would have to see what Junio says about the new function calls, because he's usually quite anal when it comes to added code vs. static initialization. ;)

Quoted 6 lines

I could do it with wrappers for pthread_mutex_lock and _unlock and lazy init there plus lazy init cond var in cond_wait and _signal, that way it could be done without any additional code in the first #ifdef. But I don't see any simple solution for working around deinitialization, that's why I'd leave non-static initialization. Let me put some touchups and resubmit for another round.

Is it actually necessary to deinitialize? Since the variables are static and therefore can't leak, and would presumably not need to be reinitialized differently if they were used again, I think they should be able to just stay. If Windows is unhappy about processes still having locks initialized at exit, I suppose we could go through and destroy all our mutexes and conds at cleanup time. Pthreads does have the appropriate functions, and it would be correct to use them, although unnecessary.

Quoted 19 lines · Daniel Barkalow

Quoted 10 lines · Andrzej K. Haczewski

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 8 lines

You are right. But #ifdef THREADED_DELTA_SEARCH is about a "generic" property of the code and is already used elsewhere in the file, whereas #ifdef WIN32 would be new and is is about platform differences.

Anyway, we would have to see what Junio says about the new function calls, because he's usually quite anal when it comes to added code vs. static initialization. ;)

Quoted 6 lines

I could do it with wrappers for pthread_mutex_lock and _unlock and lazy init there plus lazy init cond var in cond_wait and _signal, that way it could be done without any additional code in the first #ifdef. But I don't see any simple solution for working around deinitialization, that's why I'd leave non-static initialization. Let me put some touchups and resubmit for another round.

Quoted 7 lines

Is it actually necessary to deinitialize? Since the variables are static and therefore can't leak, and would presumably not need to be reinitialized differently if they were used again, I think they should be able to just stay. If Windows is unhappy about processes still having locks initialized at exit, I suppose we could go through and destroy all our mutexes and conds at cleanup time. Pthreads does have the appropriate functions, and it would be correct to use them, although unnecessary.

Lazy initialization would probably turn up to be more expensive (checking a flag on each usage) than unconditionally initializing them once. Remember that those are used at least once per object meaning a lot.

And I much prefer having runtime initialization for both Unix and Windows than having separate paths on each platform potentially hiding different bugs. And given that on Unix you can statically initialize those, then a runtime initialization is certainly not going to be _that_ costly.

Quoted 29 lines · Nicolas Pitre

Quoted 26 lines · Daniel Barkalow

Quoted 10 lines · Andrzej K. Haczewski

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 8 lines

You are right. But #ifdef THREADED_DELTA_SEARCH is about a "generic" property of the code and is already used elsewhere in the file, whereas #ifdef WIN32 would be new and is is about platform differences.

Anyway, we would have to see what Junio says about the new function calls, because he's usually quite anal when it comes to added code vs. static initialization. ;)

Quoted 6 lines

I could do it with wrappers for pthread_mutex_lock and _unlock and lazy init there plus lazy init cond var in cond_wait and _signal, that way it could be done without any additional code in the first #ifdef. But I don't see any simple solution for working around deinitialization, that's why I'd leave non-static initialization. Let me put some touchups and resubmit for another round.

Is it actually necessary to deinitialize? Since the variables are static and therefore can't leak, and would presumably not need to be reinitialized differently if they were used again, I think they should be able to just stay. If Windows is unhappy about processes still having locks initialized at exit, I suppose we could go through and destroy all our mutexes and conds at cleanup time. Pthreads does have the appropriate functions, and it would be correct to use them, although unnecessary.

Quoted 4 lines

Lazy initialization would probably turn up to be more expensive (checking a flag on each usage) than unconditionally initializing them once. Remember that those are used at least once per object meaning a lot.

Meh, checking a flag on the same cache line as the lock you're about to take can't be a big incremental cost, especially if it's actually checking whether some sort of cookie is non-zero before doing something with it.

I don't think it matters terribly much either way which we use, so long as its consistent. It'd be nice if the static initializers worked, just because people seem to write code with them, but we could just not do that in the future.

Quoted 1 line

+static __inline int pthread_cond_init(pthread_cond_t *cond, const void *unused)

What's wrong with 'static inline int ...' (without the underscores)?

2009/11/4 Johannes Sixt <[email protected]>:

Quoted 4 lines

Quoted 1 line

+static __inline int pthread_cond_init(pthread_cond_t *cond, const void *unused)

What's wrong with 'static inline int ...' (without the underscores)?

Forgot to answer. 'inline' is avaliable in C++ only (on MSVC at least), while '__inline' is MS extension for C and C++.

compat/win32/pthread.c line 16

Quoted 7 lines

compat/win32/pthread.c new file +6 -0

+static unsigned __stdcall win32_start_routine(void *arg)
+{
+     pthread_t *thread = arg;
+     thread->value = thread->start_routine(thread->arg);
+     return 0;
+}

I suppose you could reuse thread->arg for both the argument and the returned value to save a word.

Why are you casting thread->handle here?  Why not simply declaring it as a HANDLE?

2009/11/5 Nicolas Pitre <[email protected]>:

Quoted 11 lines · Andrzej K. Haczewski

compat/win32/pthread.c line 16

Quoted 7 lines

compat/win32/pthread.c new file +6 -0

+static unsigned __stdcall win32_start_routine(void *arg)
+{
+     pthread_t *thread = arg;
+     thread->value = thread->start_routine(thread->arg);
+     return 0;
+}

I suppose you could reuse thread->arg for both the argument and the returned value to save a word.

You're right! J6t committed already, what can I do now?

Quoted 2 lines

Why are you casting thread->handle here?  Why not simply declaring it as a HANDLE?

Just to silence MSVC warnings. WaitForSingleObject requires HANDLE, _beginthreadex() returns uintptr_t. It's just a matter of where would I put cast ;).

One more round of that patch with Nicolas' comments considered, and disclamer about the implementation added.

Johannes, can you replace previous commit with that patch?