[PATCH 0/1] Port of pthreads to Windows API threads

[PATCH 0/1] Port of pthreads to Windows API threads

[Andrzej K. Haczewski]

Here is a small patch that allows use of native Windows API threads for
where git uses pthreads. I don't have full msysgit environment to test it,
all I know is it compiles, so someone else could test it a little. I enabled
it for MSVC only (since msysgit uses win32-pthreads and cygwin has it's own).
I assume that patch could help with getting rid of that one dependency for
msysgit.

PS. I'm new here (actually new for whole kernel-like-development-cycle-with-
patches-flying-low-on-mailing-lists), so please be kind to my stupidity for
a while... please? ;-)

Andrzej K. Haczewski (1):
  MSVC: port pthread code to native Windows threads

 Makefile               |    2 +-
 builtin-pack-objects.c |   42 +++++++++-
 compat/winthread.h     |  219 ++++++++++++++++++++++++++++++++++++++++++++++++
 preload-index.c        |   12 +++
 4 files changed, 272 insertions(+), 3 deletions(-)
 create mode 100644 compat/winthread.h

[PATCH 1/1] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

---
 Makefile               |    2 +-
 builtin-pack-objects.c |   42 +++++++++-
 compat/winthread.h     |  219 ++++++++++++++++++++++++++++++++++++++++++++++++
 preload-index.c        |   12 +++
 4 files changed, 272 insertions(+), 3 deletions(-)
 create mode 100644 compat/winthread.h

diff --git a/Makefile b/Makefile
index 28d6ecf..126ab43 100644
--- a/Makefile
+++ b/Makefile
@@ -940,7 +940,7 @@ 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
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..a8a4f59 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -18,8 +18,12 @@
 #include "refs.h"
 
 #ifdef THREADED_DELTA_SEARCH
-#include "thread-utils.h"
-#include <pthread.h>
+# include "thread-utils.h"
+# ifndef _WIN32
+#  include <pthread.h>
+# else
+#  include <winthread.h>
+# endif
 #endif
 
 static const char pack_usage[] =
@@ -1592,7 +1596,11 @@ struct thread_params {
 
 static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
 
+#ifndef _WIN32
 static void *threaded_find_deltas(void *arg)
+#else
+static DWORD WINAPI threaded_find_deltas(LPVOID arg)
+#endif
 {
 	struct thread_params *me = arg;
 
@@ -1620,7 +1628,11 @@ static void *threaded_find_deltas(void *arg)
 		pthread_mutex_unlock(&me->mutex);
 	}
 	/* leave ->working 1 so that this doesn't get more work assigned */
+#ifndef _WIN32
 	return NULL;
+#else
+	return 0;
+#endif
 }
 
 static void ll_find_deltas(struct object_entry **list, unsigned list_size,
@@ -2327,6 +2339,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();
+
+#ifdef _WIN32
+	/*
+	 * Windows require initialization of mutex (CRITICAL_SECTION)
+	 * and conditional variable.
+	 */
+	pthread_mutex_init(&read_mutex);
+	pthread_mutex_init(&cache_mutex);
+	pthread_mutex_init(&progress_mutex);
+	win32_cond_init(&progress_cond);
+#endif
+
 #endif
 
 	prepare_packed_git();
@@ -2345,7 +2369,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	stop_progress(&progress_state);
 
 	if (non_empty && !nr_result)
+#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)
+		goto cleanup;
+#else
 		return 0;
+#endif
 	if (nr_result)
 		prepare_pack(window, depth);
 	write_pack_file();
@@ -2353,5 +2381,15 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "Total %"PRIu32" (delta %"PRIu32"),"
 			" reused %"PRIu32" (delta %"PRIu32")\n",
 			written, written_delta, reused, reused_delta);
+
+#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)
+cleanup:
+	/* cleanup Windows threads thingies */
+	win32_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+#endif
+
 	return 0;
 }
diff --git a/compat/winthread.h b/compat/winthread.h
new file mode 100644
index 0000000..32c9010
--- /dev/null
+++ b/compat/winthread.h
@@ -0,0 +1,219 @@
+/*
+ * Header used to "adapt" pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef WINTHREAD_H
+#define WINTHREAD_H
+
+#define WIN32_LEAN_AND_MEAN
+#include <windows.h>
+
+/* Implement simple condition variable for Windows threads, based on ACE implementation */
+typedef struct win32_cond {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} win32_cond_t;
+
+#define PTHREAD_COND_INITIALIZER { 0, { 0 }, NULL }
+
+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);
+	if (NULL == cond->sema)
+		return -1;
+	return 0;
+}
+
+static __inline int win32_cond_destroy(win32_cond_t *cond)
+{
+	CloseHandle(cond->sema);
+	cond->sema = NULL;
+
+	DeleteCriticalSection(&cond->waiters_lock);
+
+	return 0;
+}
+
+static __inline int win32_cond_wait(win32_cond_t *cond, CRITICAL_SECTION *mutex)
+{
+	DWORD result;
+	int ret = 0;
+
+	/* 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);
+
+	if (0 != result)
+		ret = -1;
+
+	/* one waiter less */
+	EnterCriticalSection(&cond->waiters_lock);
+	--cond->waiters;
+	LeaveCriticalSection(&cond->waiters_lock);
+
+	/* lock external mutex again */
+	EnterCriticalSection(mutex);
+
+	return ret;
+}
+
+static __inline int win32_cond_signal(win32_cond_t *cond)
+{
+	int have_waiters;
+
+	EnterCriticalSection(&cond->waiters_lock);
+	have_waiters = cond->waiters > 0;
+	LeaveCriticalSection(&cond->waiters_lock);
+
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ? 0 : -1;
+	else
+		return 0;
+}
+
+#define pthread_t HANDLE
+#define pthread_mutex_t CRITICAL_SECTION
+#define pthread_cond_t win32_cond_t
+
+#define PTHREAD_MUTEX_INITIALIZER { 0 }
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+#define pthread_cond_init(a,b) win32_cond_init((a))
+#define pthread_cond_destroy win32_cond_destroy
+#define pthread_cond_wait(a,b) win32_cond_wait((a), (b))
+#define pthread_cond_signal win32_cond_signal
+
+#define pthread_create(a,b,c,d) ((NULL == (*(a) = CreateThread(NULL, 0, (c), (d), 0, NULL))) ? (errno = err_win_to_posix(), -1) : (errno = 0, 0))
+#define pthread_join(a,b) ((WAIT_OBJECT_0 == WaitForSingleObject((a), INFINITE)) ? 0 : -1)
+
+/* almost copy-paste code of mingw.c */
+static int err_win_to_posix()
+{
+	int error = ENOSYS;
+	switch(GetLastError()) {
+	case ERROR_ACCESS_DENIED: error = EACCES; break;
+	case ERROR_ACCOUNT_DISABLED: error = EACCES; break;
+	case ERROR_ACCOUNT_RESTRICTION: error = EACCES; break;
+	case ERROR_ALREADY_ASSIGNED: error = EBUSY; break;
+	case ERROR_ALREADY_EXISTS: error = EEXIST; break;
+	case ERROR_ARITHMETIC_OVERFLOW: error = ERANGE; break;
+	case ERROR_BAD_COMMAND: error = EIO; break;
+	case ERROR_BAD_DEVICE: error = ENODEV; break;
+	case ERROR_BAD_DRIVER_LEVEL: error = ENXIO; break;
+	case ERROR_BAD_EXE_FORMAT: error = ENOEXEC; break;
+	case ERROR_BAD_FORMAT: error = ENOEXEC; break;
+	case ERROR_BAD_LENGTH: error = EINVAL; break;
+	case ERROR_BAD_PATHNAME: error = ENOENT; break;
+	case ERROR_BAD_PIPE: error = EPIPE; break;
+	case ERROR_BAD_UNIT: error = ENODEV; break;
+	case ERROR_BAD_USERNAME: error = EINVAL; break;
+	case ERROR_BROKEN_PIPE: error = EPIPE; break;
+	case ERROR_BUFFER_OVERFLOW: error = ENAMETOOLONG; break;
+	case ERROR_BUSY: error = EBUSY; break;
+	case ERROR_BUSY_DRIVE: error = EBUSY; break;
+	case ERROR_CALL_NOT_IMPLEMENTED: error = ENOSYS; break;
+	case ERROR_CANNOT_MAKE: error = EACCES; break;
+	case ERROR_CANTOPEN: error = EIO; break;
+	case ERROR_CANTREAD: error = EIO; break;
+	case ERROR_CANTWRITE: error = EIO; break;
+	case ERROR_CRC: error = EIO; break;
+	case ERROR_CURRENT_DIRECTORY: error = EACCES; break;
+	case ERROR_DEVICE_IN_USE: error = EBUSY; break;
+	case ERROR_DEV_NOT_EXIST: error = ENODEV; break;
+	case ERROR_DIRECTORY: error = EINVAL; break;
+	case ERROR_DIR_NOT_EMPTY: error = ENOTEMPTY; break;
+	case ERROR_DISK_CHANGE: error = EIO; break;
+	case ERROR_DISK_FULL: error = ENOSPC; break;
+	case ERROR_DRIVE_LOCKED: error = EBUSY; break;
+	case ERROR_ENVVAR_NOT_FOUND: error = EINVAL; break;
+	case ERROR_EXE_MARKED_INVALID: error = ENOEXEC; break;
+	case ERROR_FILENAME_EXCED_RANGE: error = ENAMETOOLONG; break;
+	case ERROR_FILE_EXISTS: error = EEXIST; break;
+	case ERROR_FILE_INVALID: error = ENODEV; break;
+	case ERROR_FILE_NOT_FOUND: error = ENOENT; break;
+	case ERROR_GEN_FAILURE: error = EIO; break;
+	case ERROR_HANDLE_DISK_FULL: error = ENOSPC; break;
+	case ERROR_INSUFFICIENT_BUFFER: error = ENOMEM; break;
+	case ERROR_INVALID_ACCESS: error = EACCES; break;
+	case ERROR_INVALID_ADDRESS: error = EFAULT; break;
+	case ERROR_INVALID_BLOCK: error = EFAULT; break;
+	case ERROR_INVALID_DATA: error = EINVAL; break;
+	case ERROR_INVALID_DRIVE: error = ENODEV; break;
+	case ERROR_INVALID_EXE_SIGNATURE: error = ENOEXEC; break;
+	case ERROR_INVALID_FLAGS: error = EINVAL; break;
+	case ERROR_INVALID_FUNCTION: error = ENOSYS; break;
+	case ERROR_INVALID_HANDLE: error = EBADF; break;
+	case ERROR_INVALID_LOGON_HOURS: error = EACCES; break;
+	case ERROR_INVALID_NAME: error = EINVAL; break;
+	case ERROR_INVALID_OWNER: error = EINVAL; break;
+	case ERROR_INVALID_PARAMETER: error = EINVAL; break;
+	case ERROR_INVALID_PASSWORD: error = EPERM; break;
+	case ERROR_INVALID_PRIMARY_GROUP: error = EINVAL; break;
+	case ERROR_INVALID_SIGNAL_NUMBER: error = EINVAL; break;
+	case ERROR_INVALID_TARGET_HANDLE: error = EIO; break;
+	case ERROR_INVALID_WORKSTATION: error = EACCES; break;
+	case ERROR_IO_DEVICE: error = EIO; break;
+	case ERROR_IO_INCOMPLETE: error = EINTR; break;
+	case ERROR_LOCKED: error = EBUSY; break;
+	case ERROR_LOCK_VIOLATION: error = EACCES; break;
+	case ERROR_LOGON_FAILURE: error = EACCES; break;
+	case ERROR_MAPPED_ALIGNMENT: error = EINVAL; break;
+	case ERROR_META_EXPANSION_TOO_LONG: error = E2BIG; break;
+	case ERROR_MORE_DATA: error = EPIPE; break;
+	case ERROR_NEGATIVE_SEEK: error = ESPIPE; break;
+	case ERROR_NOACCESS: error = EFAULT; break;
+	case ERROR_NONE_MAPPED: error = EINVAL; break;
+	case ERROR_NOT_ENOUGH_MEMORY: error = ENOMEM; break;
+	case ERROR_NOT_READY: error = EAGAIN; break;
+	case ERROR_NOT_SAME_DEVICE: error = EXDEV; break;
+	case ERROR_NO_DATA: error = EPIPE; break;
+	case ERROR_NO_MORE_SEARCH_HANDLES: error = EIO; break;
+	case ERROR_NO_PROC_SLOTS: error = EAGAIN; break;
+	case ERROR_NO_SUCH_PRIVILEGE: error = EACCES; break;
+	case ERROR_OPEN_FAILED: error = EIO; break;
+	case ERROR_OPEN_FILES: error = EBUSY; break;
+	case ERROR_OPERATION_ABORTED: error = EINTR; break;
+	case ERROR_OUTOFMEMORY: error = ENOMEM; break;
+	case ERROR_PASSWORD_EXPIRED: error = EACCES; break;
+	case ERROR_PATH_BUSY: error = EBUSY; break;
+	case ERROR_PATH_NOT_FOUND: error = ENOENT; break;
+	case ERROR_PIPE_BUSY: error = EBUSY; break;
+	case ERROR_PIPE_CONNECTED: error = EPIPE; break;
+	case ERROR_PIPE_LISTENING: error = EPIPE; break;
+	case ERROR_PIPE_NOT_CONNECTED: error = EPIPE; break;
+	case ERROR_PRIVILEGE_NOT_HELD: error = EACCES; break;
+	case ERROR_READ_FAULT: error = EIO; break;
+	case ERROR_SEEK: error = EIO; break;
+	case ERROR_SEEK_ON_DEVICE: error = ESPIPE; break;
+	case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
+	case ERROR_SHARING_VIOLATION: error = EACCES; break;
+	case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
+	case ERROR_SWAPERROR: error = ENOENT; break;
+	case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
+	case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;
+	case ERROR_UNRECOGNIZED_MEDIA: error = ENXIO; break;
+	case ERROR_UNRECOGNIZED_VOLUME: error = ENODEV; break;
+	case ERROR_WAIT_NO_CHILDREN: error = ECHILD; break;
+	case ERROR_WRITE_FAULT: error = EIO; break;
+	case ERROR_WRITE_PROTECT: error = EROFS; break;
+	}
+	return error;
+}
+
+#endif /* WINTHREAD_H */
diff --git a/preload-index.c b/preload-index.c
index 9289933..6d69a8d 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -10,7 +10,11 @@ static void preload_index(struct index_state *index, const char **pathspec)
 }
 #else
 
+#ifndef _WIN32
 #include <pthread.h>
+#else
+#include <winthread.h>
+#endif
 
 /*
  * Mostly randomly chosen maximum thread counts: we
@@ -28,7 +32,11 @@ struct thread_data {
 	int offset, nr;
 };
 
+#ifndef _WIN32
 static void *preload_thread(void *_data)
+#else
+static DWORD WINAPI preload_thread(LPVOID _data)
+#endif
 {
 	int nr;
 	struct thread_data *p = _data;
@@ -59,7 +67,11 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
+#ifndef _WIN32
 	return NULL;
+#else
+	return 0;
+#endif
 }
 
 static void preload_index(struct index_state *index, const char **pathspec)
-- 
1.6.5.2

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Johannes Schindelin]

Hi,

On Tue, 3 Nov 2009, Andrzej K. Haczewski wrote:

> ---

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

> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 02f9246..a8a4f59 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -18,8 +18,12 @@
>  #include "refs.h"
>  
>  #ifdef THREADED_DELTA_SEARCH
> -#include "thread-utils.h"
> -#include <pthread.h>
> +# include "thread-utils.h"
> +# ifndef _WIN32
> +#  include <pthread.h>
> +# else
> +#  include <winthread.h>
> +# endif
>  #endif
>  

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.

Ciao,
Dscho

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Joshua Jensen]

----- Original Message -----
From: Johannes Schindelin
Date: 11/3/2009 4:38 PM
>>   #ifdef THREADED_DELTA_SEARCH
>> -#include "thread-utils.h"
>> -#include<pthread.h>
>> +# include "thread-utils.h"
>> +# ifndef _WIN32
>> +#  include<pthread.h>
>> +# else
>> +#  include<winthread.h>
>> +# endif
>>   #endif
>>
>>      
> 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/

Josh

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

>
> 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.

Andrew

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Johannes Sixt]

Andrzej K. Haczewski schrieb:
> ---

You should sign-off your patches.

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

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

> +#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 ...

You have far too many #ifdef in the generic code. There must be a better
way to hide the implementation details of this emulation.

> +#ifdef _WIN32
> +	/*
> +	 * Windows require initialization of mutex (CRITICAL_SECTION)
> +	 * and conditional variable.
> +	 */
> +	pthread_mutex_init(&read_mutex);
> +	pthread_mutex_init(&cache_mutex);
> +	pthread_mutex_init(&progress_mutex);
> +	win32_cond_init(&progress_cond);
> +#endif

*If* we are going to use this minimal pthreads implementation, then I
think it will be OK to call pthread_*_init even on non-Windows.

> +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.)

> +	if (NULL == cond->sema)
> +		return -1;
> +	return 0;
> +}
> +
> +static __inline int win32_cond_destroy(win32_cond_t *cond)
> +{
> +	CloseHandle(cond->sema);
> +	cond->sema = NULL;
> +
> +	DeleteCriticalSection(&cond->waiters_lock);
> +
> +	return 0;
> +}
> +
> +static __inline int win32_cond_wait(win32_cond_t *cond, CRITICAL_SECTION *mutex)

And the reason that this is not pthread_cond_wait, is...?

> +{
> +	DWORD result;
> +	int ret = 0;
> +
> +	/* 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.

> +
> +	if (0 != result)
> +		ret = -1;
> +
> +	/* one waiter less */
> +	EnterCriticalSection(&cond->waiters_lock);
> +	--cond->waiters;
> +	LeaveCriticalSection(&cond->waiters_lock);
> +
> +	/* lock external mutex again */
> +	EnterCriticalSection(mutex);

> +/* almost copy-paste code of mingw.c */
> +static int err_win_to_posix()
> +{

There must be a better way than to just copy & paste this huge piece of code.

-- Hannes

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

2009/11/4 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Could you please add the reasoning from the cover letter to this commit
> message?  And add a sign-off?

Sure, will do so for next submission of that patch.

> 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.

First of all I didn't want to use wrappers because (if not inlined)
they introduce one additional call, that can be avoided with #defines
(as you can see even pthread_init can be done with macro). Second
reason is that I didn't want to create wrapping structures that would
need to be initialized / allocated / tracked. That patch translates
pthread calls to purely Win32 calls without anything in between.

Here are my reasoning for some of these #ifdefs and what can be done
and what can't (without using wrappers):

1. Thread routine has very different signature:
void *__cdecl func(void *); /* pthreads */
uint32_t __stdcall func(void *); /* Windows API */
First I thought it might be a problem to do (especially return value,
which is different size for 64-bit architectures), but since Git
doesn't use return value, it can be done.

2. Initialization of CRITICAL_SECTION and SEMAPHORE (used by condition
variables implementation). These need explicit initialization on
Windows and can't be done statically with PTHREAD_MUTEX_INITIALIZER
and PTHREAD_COND_INITIALIZER. There's no easy way around that (read:
it needs wrappers).

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

Yeah, that was lazy, mea culpa.

I'll resubmit the patch with some fixes shortly,
Andrew

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Johannes Sixt]

[please don't cull Cc list on this ML]

Andrzej K. Haczewski schrieb:
>> 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

I believe this only if you present hard numbers. My guess is that (for
example) packing objects with two threads is still faster with a slow
pthreads emulation than without threading at all.

> 2. not depend on Pthreads for Win32

Why is this an advantage?

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

Emulating pthreads on Windows with all its facets is an extremely
difficult task. If exact POSIX conformance is needed, I would choose an
existing package over doing it myself at any time.

Granted, we don't need the esoteric parts (cancelation points), which
would simplify the emulation a lot. But, as I pointed out in my other
mail, even a pthread_cond_wait() is not that trivial to implement with the
Windows API.

-- Hannes

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Michael Wookey]

2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> Andrzej K. Haczewski schrieb:
>
>> +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.)

Both events and semaphores resolve to wait-able kernel objects; so
neither is "lighter-weight" than the other.

[PATCH] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

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 <ahaczewski@gmail.com>
---
 Makefile               |    4 +-
 builtin-pack-objects.c |   29 +++++++++-
 compat/mingw.c         |    2 +-
 compat/win32/pthread.h |  143 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h      |   13 ++++
 preload-index.c        |    4 +-
 6 files changed, 187 insertions(+), 8 deletions(-)
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index 521e8a5..450d8fe 100644
--- a/Makefile
+++ b/Makefile
@@ -939,7 +939,7 @@ 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
@@ -947,7 +947,7 @@ ifdef MSVC
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..c96d293 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1592,7 +1592,7 @@ struct thread_params {
 
 static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
 
-static void *threaded_find_deltas(void *arg)
+static THREAD_FUNC(threaded_find_deltas, arg)
 {
 	struct thread_params *me = arg;
 
@@ -1620,7 +1620,7 @@ static void *threaded_find_deltas(void *arg)
 		pthread_mutex_unlock(&me->mutex);
 	}
 	/* leave ->working 1 so that this doesn't get more work assigned */
-	return NULL;
+	THREAD_RETURN(NULL);
 }
 
 static void ll_find_deltas(struct object_entry **list, unsigned list_size,
@@ -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();
+
+#ifdef _WIN32
+	/*
+	 * Windows requires initialization of mutex (CRITICAL_SECTION)
+	 * and conditional variable.
+	 */
+	pthread_mutex_init(&read_mutex);
+	pthread_mutex_init(&cache_mutex);
+	pthread_mutex_init(&progress_mutex);
+	pthread_cond_init(&progress_cond, NULL);
+#endif
+
 #endif
 
 	prepare_packed_git();
@@ -2345,7 +2357,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	stop_progress(&progress_state);
 
 	if (non_empty && !nr_result)
-		return 0;
+		goto cleanup;
 	if (nr_result)
 		prepare_pack(window, depth);
 	write_pack_file();
@@ -2353,5 +2365,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "Total %"PRIu32" (delta %"PRIu32"),"
 			" reused %"PRIu32" (delta %"PRIu32")\n",
 			written, written_delta, reused, reused_delta);
+
+cleanup:
+#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)
+	/* cleanup Windows threads thingies */
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+#endif
+
 	return 0;
 }
+
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..8f82d3c
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,143 @@
+/*
+ * Header used to "adapt" pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+ 
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * don't include mingw.h for err_win_to_posix function - mingw.h doesn't 
+ * have include-guards
+ */
+extern int err_win_to_posix(DWORD winerr);
+
+/* Implement simple condition variable for Windows threads, based on ACE implementation */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+#define PTHREAD_COND_INITIALIZER { 0, { 0 }, NULL }
+
+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 (NULL == cond->sema) 
+		return -1;
+	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)
+{
+	int ret = 0;
+
+	/* 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 */
+	if (0 != WaitForSingleObject(cond->sema, INFINITE))
+		ret = -1;
+
+	/* 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 ret;
+}
+
+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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ? 0 : -1;
+	else
+		return 0;
+}
+
+#define pthread_t HANDLE
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define PTHREAD_MUTEX_INITIALIZER { 0 }
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+static __inline int pthread_create(pthread_t *t, const void *unused, DWORD (__stdcall *start_routine)(LPVOID), void *arg)
+{
+	*t = CreateThread(NULL, 0, start_routine, arg, 0, NULL);
+
+	if (NULL == *t) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	} else {
+		errno = 0;
+		return 0;
+	}
+}
+
+static __inline int pthread_join(pthread_t t, void **unused)
+{
+	DWORD result = WaitForSingleObject(t, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			errno = 0;
+			return 0;
+		case WAIT_ABANDONED:
+			errno = EINVAL;
+			return -1;
+		default:
+			errno = err_win_to_posix(GetLastError());
+			return -1;
+	}
+}
+
+#endif /* PTHREAD_H */
+
diff --git a/git-compat-util.h b/git-compat-util.h
index ef60803..202b90e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -464,4 +464,17 @@ void git_qsort(void *base, size_t nmemb, size_t size,
  */
 int unlink_or_warn(const char *path);
 
+/*
+ * 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
+
 #endif
diff --git a/preload-index.c b/preload-index.c
index 9289933..ace10fe 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -28,7 +28,7 @@ struct thread_data {
 	int offset, nr;
 };
 
-static void *preload_thread(void *_data)
+static THREAD_FUNC(preload_thread, _data)
 {
 	int nr;
 	struct thread_data *p = _data;
@@ -59,7 +59,7 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
-	return NULL;
+	THREAD_RETURN(NULL);
 }
 
 static void preload_index(struct index_state *index, const char **pathspec)
-- 
1.6.5.2

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Erik Faye-Lund]

On Wed, Nov 4, 2009 at 11:37 AM, Andrzej K. Haczewski
<ahaczewski@gmail.com> wrote:
> +/*
> + * 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?

+/*
+ * 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
> +

-- 
Erik "kusma" Faye-Lund

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> Can't you just use the pthread package that is included in msysgit?

I don't like bloat, and msysgit is bloated. Sure, there are parts of
msysgit that are even heavier (bash, perl), but this will be removed
with further C'ification of scripts. I did what I thought could be
sensible for my first patch. I'm newbie after all.

>> +     /* 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.

It's safe to do it like this here because we're serializing waiters
count and when signaling we make sure we have waiters before we
release semaphore. That implementation is based on ACE.

Andrew

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

2009/11/4 Erik Faye-Lund <kusmabite@googlemail.com>:
>
> Seeing how THREAD_RETURN is only called with NULL-parameter, perhaps
> it would be better to do this?
>
> +/*
> + * 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

Good point. Should I resubmit the patch again?

Andrew

Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

[Johannes Schindelin]

Hi,

I do not appreciate at all that you culled me from the Cc: list.

On Tue, 3 Nov 2009, Joshua Jensen wrote:

> ----- Original Message -----
> From: Johannes Schindelin
> Date: 11/3/2009 4:38 PM
> > >   #ifdef THREADED_DELTA_SEARCH
> > > -#include "thread-utils.h"
> > > -#include<pthread.h>
> > > +# include "thread-utils.h"
> > > +# ifndef _WIN32
> > > +#  include<pthread.h>
> > > +# else
> > > +#  include<winthread.h>
> > > +# endif
> > >   #endif
> > >
> > >      
> > 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/

Pthreads is a rather large dependency we do not really need.

Ciao,
Dscho

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Paolo Bonzini]

On 11/04/2009 11:50 AM, Erik Faye-Lund wrote:
> On Wed, Nov 4, 2009 at 11:37 AM, Andrzej K. Haczewski
> <ahaczewski@gmail.com>  wrote:
>> +/*
>> + * 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?
>
> +/*
> + * 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
>> +

Even better, "return 0" is good under either platform (0 converts to 
void *), and LPVOID is the same thing as void*, so you can just do

#ifndef _WIN32
# define THREAD_RET_TYPE DWORD __stdcall
#else
# define THREAD_RET_TYPE void *
#endif

Paolo

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Paolo Bonzini]

The condition variable implementation seems more complicated than 
necessary.  The mutex can be used to protect access to cond->waiters, so 
waiters_lock is not necessary.  On the other hand, it seems to me that 
pthread_cond_signal should be the one that decrements the waiters count. 
  Otherwise, a loop like

	while (pthread_cond_signal (cond, mutex));

will fill the semaphore with signals and the waiters will get lots of 
spurious accesses.

static __inline int pthread_cond_wait(pthread_cond_t *cond,
                                       CRITICAL_SECTION *mutex)
{
	int ret = 0;

	/* the mutex protects access to waiters count */
	++cond->waiters;

	/*
	 * Unlock external mutex and wait for signal.
	 * NOTE: cond->waiters > 0 now.  If pthread_cond_signal
	 * is called after leaving mutex unlocked before we wait on
	 * semaphore, it will add a signal to the semaphore,
	 * and we'll happily go on with the wait.  This would not
	 * happen with an event, for example.
	 */
	LeaveCriticalSection(mutex);
	if (0 != WaitForSingleObject(cond->sema, INFINITE))
		ret = -1;

	EnterCriticalSection(mutex);
	return ret;
}

static __inline int pthread_cond_signal(pthread_cond_t *cond)
{
	/* the mutex protects access to waiters count */
	if (cond->waiters > 0) {
		--cond->waiters;
		return ReleaseSemaphore(cond->sema, 1, NULL) ? 0 : -1;
	} else
		return 0;
}

Paolo

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Johannes Sixt]

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:

- There is no more than one thread waiting on any particular condition
variable instance.

- 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.

On to your patch...

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.

Andrzej K. Haczewski schrieb:
> 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 <ahaczewski@gmail.com>

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).

> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 02f9246..c96d293 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1592,7 +1592,7 @@ struct thread_params {
>  
>  static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
>  
> -static void *threaded_find_deltas(void *arg)
> +static THREAD_FUNC(threaded_find_deltas, arg)
> ...
> -	return NULL;
> +	THREAD_RETURN(NULL);

See Erik's and Paolo's comments.

> @@ -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();
> +
> +#ifdef _WIN32
> +	/*
> +	 * Windows requires initialization of mutex (CRITICAL_SECTION)
> +	 * and conditional variable.
> +	 */
> +	pthread_mutex_init(&read_mutex);
> +	pthread_mutex_init(&cache_mutex);
> +	pthread_mutex_init(&progress_mutex);
> +	pthread_cond_init(&progress_cond, NULL);
> +#endif

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

> +cleanup:
> +#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)
> +	/* cleanup Windows threads thingies */
> +	pthread_cond_destroy(&progress_cond);
> +	pthread_mutex_destroy(&read_mutex);
> +	pthread_mutex_destroy(&cache_mutex);
> +	pthread_mutex_destroy(&progress_mutex);
> +#endif
> +
>  	return 0;
>  }
> +

Drop this empty line at EOF.

> @@ -0,0 +1,143 @@
> +/*
> + * Header used to "adapt" pthread-based POSIX code to Windows API threads.

I think "adapt" is the right word here. You don't need to put it in quotes. ;)

> + *
> + * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
> + */
> + 
> +#ifndef PTHREAD_H
> +#define PTHREAD_H
> +
> +#ifndef WIN32_LEAN_AND_MEAN
> +#define WIN32_LEAN_AND_MEAN
> +#endif
> +
> +#include <windows.h>
> +
> +/*
> + * don't include mingw.h for err_win_to_posix function - mingw.h doesn't 
> + * have include-guards

So what? Is there an #include loop? Can't you add include guards?

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

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

> +{
> +	cond->waiters = 0;
> +
> +	InitializeCriticalSection(&cond->waiters_lock);
> +
> +	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
> +	if (NULL == cond->sema) 
> +		return -1;
> +	return 0;

In case of failure, the pthread_* functions return the error number, not
-1. Moreover, we write

	if (!cond->sema)
		return err_win_to_posix(GetLastError());
or
	return cond->sema ? 0 : err_win_to_posix(GetLastError());

> +static __inline int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
> +{
> ...
> +	/* let's wait */
> +	if (0 != WaitForSingleObject(cond->sema, INFINITE))
> +		ret = -1;

Mind the return value!

> +static __inline int pthread_cond_signal(pthread_cond_t *cond)
> +{
> ...
> +	if (have_waiters)
> +		return ReleaseSemaphore(cond->sema, 1, NULL) ? 0 : -1;

Return value again.

> +static __inline int pthread_create(pthread_t *t, const void *unused, DWORD (__stdcall *start_routine)(LPVOID), void *arg)
> +{
> +	*t = CreateThread(NULL, 0, start_routine, arg, 0, NULL);
> +
> +	if (NULL == *t) {

	if (!*t)

> +		errno = err_win_to_posix(GetLastError());
> +		return -1;

Return value again. errno is not set.

> +	} else {
> +		errno = 0;
> +		return 0;
> +	}
> +}
> +
> +static __inline int pthread_join(pthread_t t, void **unused)
> +{
> ...
> +			errno = err_win_to_posix(GetLastError());
> +			return -1;

And again.

-- Hannes

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> Please do not cull Cc list when you resend a patch, if possible.

Ok, will do. I was sending patch using git send-email and I just
forgot to copy Cc there. Still trying to BTW is there a way to
"reformat-patch" with new amended commit and then "resend-email"?

> 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. I based this
implementation on ACE (Adaptive Communication Environment, large C++
library) implementation of the same concepts. All I removed from their
implementation is cond_broadcast, since it's not used by Git. I'm sure
that ACE does the best job when it comes to threading primitives.

On resubmit I'll give more credit to ACE.

> - 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.

> - 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. Here's
excerpt from pthread_cond_wait man page:
When using condition variables there is always a boolean predicate
involving shared variables associated with each condition wait that is
true if the thread should proceed. Spurious wakeups from the
pthread_cond_wait() or pthread_cond_timedwait() functions may occur.
Since the return from pthread_cond_wait() or pthread_cond_timedwait()
does not imply anything about the value of this predicate, the
predicate should be re-evaluated upon such return.

> 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).
>

Ok, thanks for pointing that out.

> 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

>
> -- Hannes
>

Thanks for awesome review, I'll fix all those returns and whitespaces
and resubmit.

--
Andrzej

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Johannes Schindelin]

Hi,

On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:


> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 02f9246..c96d293 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -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();
> +
> +#ifdef _WIN32

This flies in the face of our endeavors to enhance readability by reducing 
the number of #ifdef's, and at least guarding the #ifdef'ed parts behind 
meaningful names rather than platform specifiers.

See for example THREADED_DELTA_SEARCH: it does not read "HAS_PTHREADS" or 
some such.

Ciao,
Dscho

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:

>> +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++.

--
Andrew

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Erik Faye-Lund]

On Wed, Nov 4, 2009 at 3:14 PM, Andrzej K. Haczewski
<ahaczewski@gmail.com> wrote:
> 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
>
>>> +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++.

You don't need it:
compat/msvc.h:9:#define inline __inline


-- 
Erik "kusma" Faye-Lund

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Johannes Sixt]

Andrzej K. Haczewski schrieb:
> 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
>> 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.

Read again what I wrote: We are in agreement.

> On resubmit I'll give more credit to ACE.

Perhaps also a link to the source and, even better, to a discussion of the
implementation.

>> - 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.

It is not a requirement, but, yes, we do hold the mutex.

>> - 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.

Indeed.

>> 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. ;)

-- Hannes

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Andrzej K. Haczewski]

2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
>
> 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.

--
Andrzej

[PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

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.

Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>
---
 Makefile               |    8 ++-
 builtin-pack-objects.c |   41 +++++++++++++-
 compat/mingw.c         |    2 +-
 compat/mingw.h         |    5 ++
 compat/win32/pthread.h |  136 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h      |   10 ++++
 preload-index.c        |    4 +-
 7 files changed, 198 insertions(+), 8 deletions(-)
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index 521e8a5..14c371c 100644
--- a/Makefile
+++ b/Makefile
@@ -939,7 +939,8 @@ ifdef MSVC
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	NO_CURL = YesPlease
-	NO_PTHREADS = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
+	NO_STATIC_PTHREADS_INIT = YesPlease
 	BLK_SHA1 = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl
@@ -947,7 +948,7 @@ ifdef MSVC
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
@@ -1293,6 +1294,9 @@ ifdef THREADED_DELTA_SEARCH
 	BASIC_CFLAGS += -DTHREADED_DELTA_SEARCH
 	LIB_OBJS += thread-utils.o
 endif
+ifdef NO_STATIC_PTHREADS_INIT
+	COMPAT_CFLAGS += -DNO_STATIC_PTHREADS_INIT
+endif
 ifdef DIR_HAS_BSD_GROUP_SEMANTICS
 	COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
 endif
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..f297d82 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1592,7 +1592,34 @@ struct thread_params {
 
 static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
 
-static void *threaded_find_deltas(void *arg)
+/*
+ * For platforms that do support static pthreads initialization
+ * make these noops.
+ */
+#ifndef NO_STATIC_PTHREADS_INIT
+# define init_threaded_delta_search()
+# define cleanup_threaded_delta_search()
+#else
+static void init_threaded_delta_search()
+{
+	pthread_mutex_init(&read_mutex);
+	pthread_mutex_init(&cache_mutex);
+	pthread_mutex_init(&progress_mutex);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_delta_search()
+{
+	/* cleanup Windows threads thingies */
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+
+}
+#endif
+
+static THREAD_RET_TYPE threaded_find_deltas(void *arg)
 {
 	struct thread_params *me = arg;
 
@@ -1620,7 +1647,7 @@ static void *threaded_find_deltas(void *arg)
 		pthread_mutex_unlock(&me->mutex);
 	}
 	/* leave ->working 1 so that this doesn't get more work assigned */
-	return NULL;
+	return 0;
 }
 
 static void ll_find_deltas(struct object_entry **list, unsigned list_size,
@@ -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();
 #endif
 
 	prepare_packed_git();
@@ -2345,7 +2374,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	stop_progress(&progress_state);
 
 	if (non_empty && !nr_result)
-		return 0;
+		goto cleanup;
 	if (nr_result)
 		prepare_pack(window, depth);
 	write_pack_file();
@@ -2353,5 +2382,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "Total %"PRIu32" (delta %"PRIu32"),"
 			" reused %"PRIu32" (delta %"PRIu32")\n",
 			written, written_delta, reused, reused_delta);
+
+cleanup:
+#ifdef THREADED_DELTA_SEARCH
+	cleanup_threaded_delta_search();
+#endif
+
 	return 0;
 }
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 6907345..7e25fb5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -294,3 +294,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..a71b90f
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,136 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+#define PTHREAD_COND_INITIALIZER { 0, { 0 }, NULL }
+
+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)
+		return 0; /* POSIX do not allow pthread_cond_init to fail */
+	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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError();
+	else
+		return 0;
+}
+
+#define pthread_t HANDLE
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define PTHREAD_MUTEX_INITIALIZER { 0 }
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+static inline int pthread_create(pthread_t *thread, const void *unused,
+		DWORD (__stdcall *start_routine)(LPVOID), void *arg)
+{
+	*thread = CreateThread(NULL, 0, start_routine, arg, 0, NULL);
+
+	if (!*thread)
+		return err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
+
+static inline int pthread_join(pthread_t thread, void **unused)
+{
+	DWORD result = WaitForSingleObject(t, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+#endif /* PTHREAD_H */
diff --git a/git-compat-util.h b/git-compat-util.h
index ef60803..4311117 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -464,4 +464,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
  */
 int unlink_or_warn(const char *path);
 
+/*
+ * Define type of thread function return type to distinguish
+ * Windows and POSIX.
+ */
+#ifndef _WIN32
+# define THREAD_RET_TYPE void *
+#else
+# define THREAD_RET_TYPE DWORD __stdcall
+#endif
+
 #endif
diff --git a/preload-index.c b/preload-index.c
index 9289933..41b11a3 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -28,7 +28,7 @@ struct thread_data {
 	int offset, nr;
 };
 
-static void *preload_thread(void *_data)
+static THREAD_RET_TYPE preload_thread(void* _data)
 {
 	int nr;
 	struct thread_data *p = _data;
@@ -59,7 +59,7 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
-	return NULL;
+	return 0;
 }
 
 static void preload_index(struct index_state *index, const char **pathspec)
-- 
1.6.5.2

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:

> +	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.

> +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.

> @@ -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).


Nicolas

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Daniel Barkalow]

On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:

> 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> >
> > 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.

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.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/4 Nicolas Pitre <nico@fluxnic.net>:
> On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
>
>> +     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.
>

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?

>> +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).

>> @@ -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).

Sounds sensible, but I'd wait for the NO_STATIC_PTHREADS_INIT verdict.

--
Andrzej

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Nicolas Pitre]

On Wed, 4 Nov 2009, Daniel Barkalow wrote:

> On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
> 
> > 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> > >
> > > 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.
> 
> 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.

And while at it, we might just deinitialize them as soon as we're done 
with them saving resources (on Unix that might turn up to be a no op 
anyway) which is why I suggested doing this within ll_find_deltas() 
directly.


Nicolas

[PATCH] pack-objects: move thread autodetection closer to relevant code

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1920 bytes --]

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).

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---

> 2009/11/4 Nicolas Pitre <nico@fluxnic.net>:
> >> @@ -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).

So here it is.

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..4c91e94 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1629,6 +1629,8 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	struct thread_params *p;
 	int i, ret, active_threads = 0;
 
+	if (!delta_search_threads)	/* --threads=0 means autodetect */
+		delta_search_threads = online_cpus();
 	if (delta_search_threads <= 1) {
 		find_deltas(list, &list_size, window, depth, processed);
 		return;
@@ -2324,11 +2326,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (keep_unreachable && unpack_unreachable)
 		die("--keep-unreachable and --unpack-unreachable are incompatible.");
 
-#ifdef THREADED_DELTA_SEARCH
-	if (!delta_search_threads)	/* --threads=0 means autodetect */
-		delta_search_threads = online_cpus();
-#endif
-
 	prepare_packed_git();
 
 	if (progress)

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Erik Faye-Lund]

On Wed, Nov 4, 2009 at 10:16 PM, Andrzej K. Haczewski
<ahaczewski@gmail.com> wrote:
>>> +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 "kusma" Faye-Lund

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 849 bytes --]

On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:

> 2009/11/4 Nicolas Pitre <nico@fluxnic.net>:
> > On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
> >
> >> +     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.
> >
> 
> I don't like that approach either, but I was frighten of Junio being
> anal about static inits ;).

I think the alternative is much worse than loosing those 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?

Please do it and if anyone finds a problem with it then we'll start from 
there.


Nicolas

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Daniel Barkalow]

On Wed, 4 Nov 2009, Nicolas Pitre wrote:

> On Wed, 4 Nov 2009, Daniel Barkalow wrote:
> 
> > On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
> > 
> > > 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> > > >
> > > > 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.
> > 
> > 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.

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.

> 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.

Yeah, definitely best to have them match, whichever way we go.

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.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

Erik Faye-Lund pisze:
> 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*.

--
Andrzej

[PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

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.

Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>
---
 Makefile               |    4 +-
 builtin-pack-objects.c |   34 ++++++++++--
 compat/mingw.c         |    2 +-
 compat/mingw.h         |    5 ++
 compat/win32/pthread.h |  132 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h      |   10 ++++
 preload-index.c        |    4 +-
 7 files changed, 180 insertions(+), 11 deletions(-)
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index 94d44b0..0146ac7 100644
--- a/Makefile
+++ b/Makefile
@@ -975,7 +975,7 @@ 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
@@ -983,7 +983,7 @@ ifdef MSVC
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..e897b16 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1255,15 +1255,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1590,9 +1590,28 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
 
-static void *threaded_find_deltas(void *arg)
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex);
+	pthread_mutex_init(&cache_mutex);
+	pthread_mutex_init(&progress_mutex);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
+
+static THREAD_RET_TYPE threaded_find_deltas(void *arg)
 {
 	struct thread_params *me = arg;
 
@@ -1620,7 +1639,7 @@ static void *threaded_find_deltas(void *arg)
 		pthread_mutex_unlock(&me->mutex);
 	}
 	/* leave ->working 1 so that this doesn't get more work assigned */
-	return NULL;
+	return 0;
 }
 
 static void ll_find_deltas(struct object_entry **list, unsigned list_size,
@@ -1638,6 +1657,8 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 				delta_search_threads);
 	p = xcalloc(delta_search_threads, sizeof(*p));
 
+	init_threaded_search();
+
 	/* Partition the work amongst work threads. */
 	for (i = 0; i < delta_search_threads; i++) {
 		unsigned sub_size = list_size / (delta_search_threads - i);
@@ -1745,6 +1766,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 6907345..7e25fb5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -294,3 +294,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..0e43714
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,132 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+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)
+		return 0; /* POSIX do not allow pthread_cond_init to fail */
+	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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError();
+	else
+		return 0;
+}
+
+#define pthread_t HANDLE
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+static inline int pthread_create(pthread_t *thread, const void *unused,
+		DWORD (__stdcall *start_routine)(LPVOID), void *arg)
+{
+	*thread = CreateThread(NULL, 0, start_routine, arg, 0, NULL);
+
+	if (!*thread)
+		return err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
+
+static inline int pthread_join(pthread_t thread, void **unused)
+{
+	DWORD result = WaitForSingleObject(t, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+#endif /* PTHREAD_H */
diff --git a/git-compat-util.h b/git-compat-util.h
index ef60803..4311117 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -464,4 +464,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
  */
 int unlink_or_warn(const char *path);
 
+/*
+ * Define type of thread function return type to distinguish
+ * Windows and POSIX.
+ */
+#ifndef _WIN32
+# define THREAD_RET_TYPE void *
+#else
+# define THREAD_RET_TYPE DWORD __stdcall
+#endif
+
 #endif
diff --git a/preload-index.c b/preload-index.c
index 9289933..41b11a3 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -28,7 +28,7 @@ struct thread_data {
 	int offset, nr;
 };
 
-static void *preload_thread(void *_data)
+static THREAD_RET_TYPE preload_thread(void* _data)
 {
 	int nr;
 	struct thread_data *p = _data;
@@ -59,7 +59,7 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
-	return NULL;
+	return 0;
 }
 
 static void preload_index(struct index_state *index, const char **pathspec)
-- 
1.6.5.2

[PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

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.

Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>
---
Ignore previous patch, there was a compile error for POSIX platform
(pthread_mutex_init takes 2 arguments, MSVC doesn't complain if
1 argument is supplied to a macro call, but on POSIX it's function
call).

I haven't integrated Nicolas patch.

 Makefile               |    4 +-
 builtin-pack-objects.c |   34 ++++++++++--
 compat/mingw.c         |    2 +-
 compat/mingw.h         |    5 ++
 compat/win32/pthread.h |  132 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h      |   10 ++++
 preload-index.c        |    4 +-
 7 files changed, 180 insertions(+), 11 deletions(-)
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index 94d44b0..0146ac7 100644
--- a/Makefile
+++ b/Makefile
@@ -975,7 +975,7 @@ 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
@@ -983,7 +983,7 @@ ifdef MSVC
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..e897b16 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1255,15 +1255,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1590,9 +1590,28 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
 
-static void *threaded_find_deltas(void *arg)
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex, NULL);
+	pthread_mutex_init(&cache_mutex, NULL);
+	pthread_mutex_init(&progress_mutex, NULL);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
+
+static THREAD_RET_TYPE threaded_find_deltas(void *arg)
 {
 	struct thread_params *me = arg;
 
@@ -1620,7 +1639,7 @@ static void *threaded_find_deltas(void *arg)
 		pthread_mutex_unlock(&me->mutex);
 	}
 	/* leave ->working 1 so that this doesn't get more work assigned */
-	return NULL;
+	return 0;
 }
 
 static void ll_find_deltas(struct object_entry **list, unsigned list_size,
@@ -1638,6 +1657,8 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 				delta_search_threads);
 	p = xcalloc(delta_search_threads, sizeof(*p));
 
+	init_threaded_search();
+
 	/* Partition the work amongst work threads. */
 	for (i = 0; i < delta_search_threads; i++) {
 		unsigned sub_size = list_size / (delta_search_threads - i);
@@ -1745,6 +1766,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 6907345..7e25fb5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -294,3 +294,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..0e43714
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,132 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+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)
+		return 0; /* POSIX do not allow pthread_cond_init to fail */
+	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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError();
+	else
+		return 0;
+}
+
+#define pthread_t HANDLE
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+static inline int pthread_create(pthread_t *thread, const void *unused,
+		DWORD (__stdcall *start_routine)(LPVOID), void *arg)
+{
+	*thread = CreateThread(NULL, 0, start_routine, arg, 0, NULL);
+
+	if (!*thread)
+		return err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
+
+static inline int pthread_join(pthread_t thread, void **unused)
+{
+	DWORD result = WaitForSingleObject(t, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+#endif /* PTHREAD_H */
diff --git a/git-compat-util.h b/git-compat-util.h
index ef60803..4311117 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -464,4 +464,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
  */
 int unlink_or_warn(const char *path);
 
+/*
+ * Define type of thread function return type to distinguish
+ * Windows and POSIX.
+ */
+#ifndef _WIN32
+# define THREAD_RET_TYPE void *
+#else
+# define THREAD_RET_TYPE DWORD __stdcall
+#endif
+
 #endif
diff --git a/preload-index.c b/preload-index.c
index 9289933..41b11a3 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -28,7 +28,7 @@ struct thread_data {
 	int offset, nr;
 };
 
-static void *preload_thread(void *_data)
+static THREAD_RET_TYPE preload_thread(void* _data)
 {
 	int nr;
 	struct thread_data *p = _data;
@@ -59,7 +59,7 @@ static void *preload_thread(void *_data)
 			continue;
 		ce_mark_uptodate(ce);
 	} while (--nr > 0);
-	return NULL;
+	return 0;
 }
 
 static void preload_index(struct index_state *index, const char **pathspec)
-- 1.6.5.2 

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
>
>> +	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.

Hear hear.

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> @@ -1638,6 +1657,8 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
>  				delta_search_threads);
>  	p = xcalloc(delta_search_threads, sizeof(*p));
>  
> +	init_threaded_search();

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.


Nicolas

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Nicolas Pitre]

On Wed, 4 Nov 2009, Daniel Barkalow wrote:

> On Wed, 4 Nov 2009, Nicolas Pitre wrote:
> 
> > On Wed, 4 Nov 2009, Daniel Barkalow wrote:
> > 
> > > On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
> > > 
> > > > 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> > > > >
> > > > > 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.
> > > 
> > > 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.
> 
> 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.

This is still a bigger cost than not checking such flag at all.  
Especially if the check will be false on every call but the first one 
out of millions.  I agree this is not significant, but neither is a 
runtime initialization vs a static one.

> 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.

Maybe the static initializer can be turned into a global constructor on 
Windows?


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> +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)
> +		return 0; /* POSIX do not allow pthread_cond_init to fail */
> +	return 0;
> +}

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

However, my pthread_cond_init man page says:

[[[
RETURN VALUE
       If successful, the pthread_cond_destroy() and pthread_cond_init() func-
       tions  shall  return zero; otherwise, an error number shall be returned
       to indicate the error.

       The [EBUSY] and [EINVAL] error checks, if implemented, shall act as  if
       they  were performed immediately at the beginning of processing for the
       function and caused an error return prior to modifying the state of the
       condition variable specified by cond.

ERRORS
       The pthread_cond_destroy() function may fail if:

       EBUSY  The implementation has detected an attempt to destroy the object
              referenced by cond while it is referenced  (for  example,  while
              being used in a pthread_cond_wait() or pthread_cond_timedwait())
              by another thread.

       EINVAL The value specified by cond is invalid.

       The pthread_cond_init() function shall fail if:

       EAGAIN The system lacked the necessary resources (other than memory) to
              initialize another condition variable.

       ENOMEM Insufficient memory exists to initialize the condition variable.

       The pthread_cond_init() function may fail if:

       EBUSY  The implementation has detected an attempt to  reinitialize  the
              object referenced by cond, a previously initialized, but not yet
              destroyed, condition variable.

       EINVAL The value specified by attr is invalid.
]]]

I'm not advocating that you implement detailed error codes as we don't 
really care about specific errors.  This is just to disagree with the 
"POSIX do not allow pthread_cond_init to fail" assertion. In any case, 
using die() to keep it simple is certainly better than blindly returning 
0 on failure.  However you could simply return ENOMEM and use the die() 
in init_threaded_search() instead.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:

> Erik Faye-Lund pisze:
> > 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);
        [...]
}

?

Sure this will use 8 to 16 more bytes per thread, but we're dealing with 
a rather small number of threads anyway (more threads than the number of 
CPU cores is useless) making this extra memory usage rather 
insignificant compared to the many megabytes of RAM the rest of the code 
is using.  The advantage is full compatibility with the native pthread 
interface git is using at the source level while still being much 
lighter than a full blown pthread implementation.

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.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

Andrzej K. Haczewski schrieb:
> 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.

Looks quite good already.

In the next round, please squash this in.

diff --git a/Makefile b/Makefile
index bae1b40..6648d11 100644
--- a/Makefile
+++ b/Makefile
@@ -414,6 +414,7 @@ LIB_H += cache-tree.h
 LIB_H += commit.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

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> Please use die("CreateSemaphore() failed") in the failure case instead
> of returning success.

Sure, will do.

> 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.
]]]

--
Andrzej

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> 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.

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?

--
Andrzej

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
>
> 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);
>        [...]
> }

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

> 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.

Yeah, I'll factor that out to separate file.

--
Andrzej

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Erik Faye-Lund]

On Thu, Nov 5, 2009 at 10:00 AM, Andrzej K. Haczewski
<ahaczewski@gmail.com> wrote:
>
> That way we don't need allocations to simulate pthread init/join API

Yay! By the way, I love the work you're doing here. Getting threaded
delta-searching on Windows is something I'm looking forward to :)

-- 
Erik "kusma" Faye-Lund

[PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

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.

Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>
---
 Makefile               |    7 ++-
 builtin-pack-objects.c |   31 ++++++++++--
 compat/mingw.c         |    2 +-
 compat/mingw.h         |    5 ++
 compat/win32/pthread.c |   39 +++++++++++++++
 compat/win32/pthread.h |  125 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 201 insertions(+), 8 deletions(-)
 create mode 100644 compat/win32/pthread.c
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index bc039ac..30089a8 100644
--- a/Makefile
+++ b/Makefile
@@ -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
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..00594fd 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1255,15 +1255,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1590,7 +1590,26 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
+
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex, NULL);
+	pthread_mutex_init(&cache_mutex, NULL);
+	pthread_mutex_init(&progress_mutex, NULL);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
 
 static void *threaded_find_deltas(void *arg)
 {
@@ -1629,8 +1648,11 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	struct thread_params *p;
 	int i, ret, active_threads = 0;
 
+	init_threaded_search();
+
 	if (delta_search_threads <= 1) {
 		find_deltas(list, &list_size, window, depth, processed);
+		cleanup_threaded_search();
 		return;
 	}
 	if (progress > pack_to_stdout)
@@ -1745,6 +1767,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 6907345..7e25fb5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -294,3 +294,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
new file mode 100644
index 0000000..a7ab7af
--- /dev/null
+++ b/compat/win32/pthread.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#include "pthread.h"
+
+static DWORD __stdcall win32_start_routine(LPVOID arg)
+{
+	pthread_t *thread = arg;
+	thread->value = thread->start_routine(thread->arg);
+	return 0;
+}
+
+int pthread_create(pthread_t *thread, const void *unused,
+		   void *(*start_routine)(void*), void *arg)
+{
+	thread->arg = arg;
+	thread->handle = CreateThread(NULL, 0, win32_start_routine, thread, 0, NULL);
+
+	if (!thread->handle)
+		return err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
+
+int win32_pthread_join(pthread_t *thread, void **value_ptr)
+{
+	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			if (value_ptr)
+				*value_ptr = thread.value;
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..e50eb6b
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,125 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError();
+	else
+		return 0;
+}
+
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+typedef struct {
+	HANDLE handle;
+	void *(*start_routine)(void*);
+	void *arg;
+	void *value;
+} pthread_t;
+
+extern int pthread_create(pthread_t *thread, const void *unused,
+			  void *(*start_routine)(void*), void *arg);
+
+/*
+ * To avoid the need of allocating struct, we use small macro wrapper to pass
+ * pointer to win32_pthread_join instead of using typedef struct {} *pthread_t
+ */
+#define pthread_join(a, b) win32_pthread_join(&(a), (b))
+
+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
+
+#endif /* PTHREAD_H */
-- 
1.6.5.2

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

Andrzej K. Haczewski schrieb:
> diff --git a/Makefile b/Makefile
> index bc039ac..30089a8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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
>  	AR = compat/vcbuild/scripts/lib.pl
>  	CFLAGS =
>  	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
> -	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
> -	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
> +	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
> +	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
>  	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
>  	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
>  	lib =

What compiles compat/win32/pthread.c?

Please don't forget to add compat/win32/*.o to the clean target.

> +int pthread_create(pthread_t *thread, const void *unused,
> +		   void *(*start_routine)(void*), void *arg)
> +{
> +	thread->arg = arg;
> +	thread->handle = CreateThread(NULL, 0, win32_start_routine, thread, 0, NULL);

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

> +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);
> +
> +	/*
> +	 * Signal only when there are waiters
> +	 */
> +	if (have_waiters)
> +		return ReleaseSemaphore(cond->sema, 1, NULL) ?
> +			0 : err_win_to_posix(GetLastError();
> +	else
> +		return 0;
> +}

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

-- Hannes

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/5 Johannes Sixt <j.sixt@viscovery.net>:
> Elsewhere we use _beginthreadex(). What's the difference?

Oh my, I've just run through MSDN documentation and I'm wrongly using
CreateThread. I should have used _beginthread from the start, because
besides calling CreateThread it initializes local thread storage for
global C-runtime variables (errno etc.), which CreateThread does not
do. That could lead to very unpleasant consequences. I'll redo thread
creation to use _beginthreadex(). Thanks for that question!

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

Ok, will do.

--
Andrzej

Re: [PATCH] MSVC: port pthread code to native Windows threads

[Dmitry Potapov]

On Wed, Nov 04, 2009 at 02:47:09PM +0100, Andrzej K. Haczewski wrote:
> 2009/11/4 Johannes Sixt <j.sixt@viscovery.net>:
> 
> > - 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.

There is no such requirement in POSIX:

   The pthread_cond_broadcast() or pthread_cond_signal() functions may
   be called by a thread whether or not it currently owns the mutex that
   threads calling pthread_cond_wait() or pthread_cond_timedwait() have
   associated with the condition variable during their waits; however,
   if predictable scheduling behavior is required, then that mutex shall
   be locked by the thread calling pthread_cond_broadcast() or
   pthread_cond_signal().

http://www.opengroup.org/onlinepubs/009695399/functions/pthread_cond_signal.html



Dmitry

[PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

This patch implements native to Windows subset of pthreads API used by Git.
It allows to remove Pthreads for Win32 dependency for MSVC, 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.

Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>
---
Here is another round of that patch with all comments considered. There is
no workaround to make static initialization of mutexes and condition
variables work for Windows, that's why there's explicit initialization
added.

I hope I added to Cc all interested in this patch. Excuse me if I omitted
someone.

 Makefile               |    7 ++-
 builtin-pack-objects.c |   31 +++++++++++--
 compat/mingw.c         |    2 +-
 compat/mingw.h         |    5 ++
 compat/win32/pthread.c |  116 ++++++++++++++++++++++++++++++++++++++++++++++++
 compat/win32/pthread.h |   69 ++++++++++++++++++++++++++++
 6 files changed, 222 insertions(+), 8 deletions(-)
 create mode 100644 compat/win32/pthread.c
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index bc039ac..30089a8 100644
--- a/Makefile
+++ b/Makefile
@@ -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
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..00594fd 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1255,15 +1255,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1590,7 +1590,26 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
+
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex, NULL);
+	pthread_mutex_init(&cache_mutex, NULL);
+	pthread_mutex_init(&progress_mutex, NULL);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
 
 static void *threaded_find_deltas(void *arg)
 {
@@ -1629,8 +1648,11 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	struct thread_params *p;
 	int i, ret, active_threads = 0;
 
+	init_threaded_search();
+
 	if (delta_search_threads <= 1) {
 		find_deltas(list, &list_size, window, depth, processed);
+		cleanup_threaded_search();
 		return;
 	}
 	if (progress > pack_to_stdout)
@@ -1745,6 +1767,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 6907345..7e25fb5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -294,3 +294,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
new file mode 100644
index 0000000..e4d21bd
--- /dev/null
+++ b/compat/win32/pthread.c
@@ -0,0 +1,116 @@
+/*
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#include "../../git-compat-util.h"
+#include "pthread.h"
+
+#include <errno.h>
+#include <limits.h>
+
+static unsigned __stdcall win32_start_routine(void *arg)
+{
+	pthread_t *thread = arg;
+	thread->value = thread->start_routine(thread->arg);
+	return 0;
+}
+
+int pthread_create(pthread_t *thread, const void *unused,
+		   void *(*start_routine)(void*), void *arg)
+{
+	thread->arg = arg;
+	thread->start_routine = start_routine;
+	thread->value = NULL;
+	thread->handle =
+		_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
+
+	if (!thread->handle)
+		return errno;
+	else
+		return 0;
+}
+
+int win32_pthread_join(pthread_t *thread, void **value_ptr)
+{
+	DWORD result = WaitForSingleObject((HANDLE)thread->handle, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			if (value_ptr)
+				*value_ptr = thread->value;
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+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("CreateSemaphore() failed");
+	return 0;
+}
+
+int pthread_cond_destroy(pthread_cond_t *cond)
+{
+	CloseHandle(cond->sema);
+	cond->sema = NULL;
+
+	DeleteCriticalSection(&cond->waiters_lock);
+
+	return 0;
+}
+
+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 - ignore return value */
+	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;
+}
+
+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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..a7594cc
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,69 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Defines that adapt Windows API threads to pthreads API
+ */
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+extern int pthread_cond_init(pthread_cond_t *cond, const void *unused);
+
+extern int pthread_cond_destroy(pthread_cond_t *cond);
+
+extern int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex);
+
+extern int pthread_cond_signal(pthread_cond_t *cond);
+
+/*
+ * Simple thread creation implementation using pthread API
+ */
+typedef struct {
+	uintptr_t handle;
+	void *(*start_routine)(void*);
+	void *arg;
+	void *value;
+} pthread_t;
+
+extern int pthread_create(pthread_t *thread, const void *unused,
+			  void *(*start_routine)(void*), void *arg);
+
+/*
+ * To avoid the need of allocating struct, we use small macro wrapper to pass
+ * pointer to win32_pthread_join instead of using typedef struct {} *pthread_t
+ */
+#define pthread_join(a, b) win32_pthread_join(&(a), (b))
+
+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
+
+#endif /* PTHREAD_H */
-- 
1.6.5.2

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

Thanks.

I pushed this to

  git://repo.or.cz/git/mingw/j6t.git pthreads-for-windows

together with Nico's patch and another patch to enable pthreads on
MinGW unconditionally. Feedback welcome!

Andrzej K. Haczewski (1):
      MSVC: Windows-native implementation for subset of Pthreads API

Johannes Sixt (1):
      MinGW: enable pthreads

Nicolas Pitre (1):
      pack-objects: move thread autodetection closer to relevant code

This is my patch:

--- >8 ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] MinGW: enable pthreads

If the MinGW build was built as part of the msysgit build environment,
then threading was already enabled because the pthreads package from
GNU-Win32 is available in msysgit.

The previous patch added a minimal pthreads implementation for Windows.
Therefore, we can now enable code that uses pthreads unconditionally.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 Makefile |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index db7ffb0..4b8603a 100644
--- a/Makefile
+++ b/Makefile
@@ -986,9 +986,11 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
+	THREADED_DELTA_SEARCH = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o
+	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
+		compat/win32/pthread.o
 	EXTLIBS += -lws2_32
 	X = .exe
 ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
@@ -998,10 +1000,8 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 	EXTLIBS += /mingw/lib/libz.a
 	NO_R_TO_GCC_LINKER = YesPlease
 	INTERNAL_QSORT = YesPlease
-	THREADED_DELTA_SEARCH = YesPlease
 else
 	NO_CURL = YesPlease
-	NO_PTHREADS = YesPlease
 endif
 endif
 endif
-- 
1.6.5.2.1198.ge698c

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> > 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.
> ]]]

Maybe that's for a particular implementation.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1074 bytes --]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> 2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> > 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.
> 
> 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?

It is already factored out when thread support is not enabled.

When thread support is enabled but there is only one thread, there was 
no point duplicating the code just to have a path without any mutexes, 
especially on Linux where no performance difference could be measured.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1498 bytes --]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> 2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> > On Wed, 4 Nov 2009, Andrzej K. Haczewski wrote:
> >
> > 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);
> >        [...]
> > }
> 
> 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...

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

> 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

Right.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> +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.

> +int win32_pthread_join(pthread_t *thread, void **value_ptr)
> +{
> +	DWORD result = WaitForSingleObject((HANDLE)thread->handle, INFINITE);

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

Otherwise this looks pretty good now.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:
>
>> +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?

> 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 ;).

--
Andrzej

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1095 bytes --]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> 2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> > On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:
> >
> >> +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?

Just post a replacement patch.

> > 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 ;).

Wonderful.  One could wonder why Windows can't have coherent 
interfaces...

Well, given that all existing usages in the tree (run-command.c and 
compat/mingw.c) already cast the _beginthreadex() return value instead, 
then that might be a good idea to follow the same model.


Nicolas

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> 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.

--
Andrzej

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 990 bytes --]

On Thu, 5 Nov 2009, Andrzej K. Haczewski wrote:

> 2009/11/5 Nicolas Pitre <nico@fluxnic.net>:
> > Why not?  At least gcc is quite happy with such a construct.  It
> > probably makes a copy of the stack before passing it though.

"a copy of the struct" I meant here.

> 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 :)

Welcome back !  ;-)

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

Yep, just what I said.  Normally if you pass a structure to a function, 
it will be copied beforehand so modifications by the callee won't be 
seen by the caller.  But in this case we don't care, hence passing the 
original structure address is more efficient.


Nicolas

Re: [PATCH] pack-objects: move thread autodetection closer to relevant code

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> 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).

Nice.  The ll_find_delta() function itself will disappear and becomes the
single-threaded find_deltas() when THREADED_DELTA_SEARCH is not defined,
and the variable in question is used only inside the function anyway, so
this works beautifully.

Very nice.

[PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Andrzej K. Haczewski]

This patch implements native to Windows subset of pthreads API used by Git.
It allows to remove Pthreads for Win32 dependency for MSVC, 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.

Signed-off-by: Andrzej K. Haczewski <ahaczewski@gmail.com>
---
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?

 Makefile               |    7 ++-
 builtin-pack-objects.c |   31 +++++++++++--
 compat/mingw.c         |    2 +-
 compat/mingw.h         |    5 ++
 compat/win32/pthread.c |  120 ++++++++++++++++++++++++++++++++++++++++++++++++
 compat/win32/pthread.h |   68 +++++++++++++++++++++++++++
 6 files changed, 225 insertions(+), 8 deletions(-)
 create mode 100644 compat/win32/pthread.c
 create mode 100644 compat/win32/pthread.h

diff --git a/Makefile b/Makefile
index bc039ac..30089a8 100644
--- a/Makefile
+++ b/Makefile
@@ -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
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..00594fd 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1255,15 +1255,15 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 
 #ifdef THREADED_DELTA_SEARCH
 
-static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t read_mutex;
 #define read_lock()		pthread_mutex_lock(&read_mutex)
 #define read_unlock()		pthread_mutex_unlock(&read_mutex)
 
-static pthread_mutex_t cache_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
 #define cache_unlock()		pthread_mutex_unlock(&cache_mutex)
 
-static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t progress_mutex;
 #define progress_lock()		pthread_mutex_lock(&progress_mutex)
 #define progress_unlock()	pthread_mutex_unlock(&progress_mutex)
 
@@ -1590,7 +1590,26 @@ struct thread_params {
 	unsigned *processed;
 };
 
-static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t progress_cond;
+
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_threaded_search()
+{
+	pthread_mutex_init(&read_mutex, NULL);
+	pthread_mutex_init(&cache_mutex, NULL);
+	pthread_mutex_init(&progress_mutex, NULL);
+	pthread_cond_init(&progress_cond, NULL);
+}
+
+static void cleanup_threaded_search()
+{
+	pthread_cond_destroy(&progress_cond);
+	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&cache_mutex);
+	pthread_mutex_destroy(&progress_mutex);
+}
 
 static void *threaded_find_deltas(void *arg)
 {
@@ -1629,8 +1648,11 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 	struct thread_params *p;
 	int i, ret, active_threads = 0;
 
+	init_threaded_search();
+
 	if (delta_search_threads <= 1) {
 		find_deltas(list, &list_size, window, depth, processed);
+		cleanup_threaded_search();
 		return;
 	}
 	if (progress > pack_to_stdout)
@@ -1745,6 +1767,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 			active_threads--;
 		}
 	}
+	cleanup_threaded_search();
 	free(p);
 }
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..f2e9f02 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,7 @@
 
 #include <shellapi.h>
 
-static int err_win_to_posix(DWORD winerr)
+int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
 	switch(winerr) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 6907345..7e25fb5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -294,3 +294,8 @@ struct mingw_dirent
 #define readdir(x) mingw_readdir(x)
 struct dirent *mingw_readdir(DIR *dir);
 #endif // !NO_MINGW_REPLACE_READDIR
+
+/*
+ * Used by Pthread API implementation for Windows
+ */
+extern int err_win_to_posix(DWORD winerr);
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
new file mode 100644
index 0000000..652d7b4
--- /dev/null
+++ b/compat/win32/pthread.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ *
+ * DISCLAMER: The implementation is Git-specific, it is subset of original
+ * Pthreads API, without lots of other features that Git doesn't use.
+ * Git also makes sure that the passed arguments are valid, so there's
+ * no need for double-checking.
+ */
+
+#include "../../git-compat-util.h"
+#include "pthread.h"
+
+#include <errno.h>
+#include <limits.h>
+
+static unsigned __stdcall win32_start_routine(void *arg)
+{
+	pthread_t *thread = arg;
+	thread->arg = thread->start_routine(thread->arg);
+	return 0;
+}
+
+int pthread_create(pthread_t *thread, const void *unused,
+		   void *(*start_routine)(void*), void *arg)
+{
+	thread->arg = arg;
+	thread->start_routine = start_routine;
+	thread->handle = (HANDLE)
+		_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
+
+	if (!thread->handle)
+		return errno;
+	else
+		return 0;
+}
+
+int win32_pthread_join(pthread_t *thread, void **value_ptr)
+{
+	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
+	switch (result) {
+		case WAIT_OBJECT_0:
+			if (value_ptr)
+				*value_ptr = thread->arg;
+			return 0;
+		case WAIT_ABANDONED:
+			return EINVAL;
+		default:
+			return err_win_to_posix(GetLastError());
+	}
+}
+
+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("CreateSemaphore() failed");
+	return 0;
+}
+
+int pthread_cond_destroy(pthread_cond_t *cond)
+{
+	CloseHandle(cond->sema);
+	cond->sema = NULL;
+
+	DeleteCriticalSection(&cond->waiters_lock);
+
+	return 0;
+}
+
+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 - ignore return value */
+	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;
+}
+
+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);
+
+	/*
+	 * Signal only when there are waiters
+	 */
+	if (have_waiters)
+		return ReleaseSemaphore(cond->sema, 1, NULL) ?
+			0 : err_win_to_posix(GetLastError());
+	else
+		return 0;
+}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
new file mode 100644
index 0000000..63293ad
--- /dev/null
+++ b/compat/win32/pthread.h
@@ -0,0 +1,68 @@
+/*
+ * Header used to adapt pthread-based POSIX code to Windows API threads.
+ *
+ * Copyright (C) 2009 Andrzej K. Haczewski <ahaczewski@gmail.com>
+ */
+
+#ifndef PTHREAD_H
+#define PTHREAD_H
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+
+#include <windows.h>
+
+/*
+ * Defines that adapt Windows API threads to pthreads API
+ */
+#define pthread_mutex_t CRITICAL_SECTION
+
+#define pthread_mutex_init(a,b) InitializeCriticalSection((a))
+#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
+#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_unlock LeaveCriticalSection
+
+/*
+ * Implement simple condition variable for Windows threads, based on ACE
+ * implementation.
+ *
+ * See original implementation: http://bit.ly/1vkDjo
+ * ACE homepage: http://www.cse.wustl.edu/~schmidt/ACE.html
+ * See also: http://www.cse.wustl.edu/~schmidt/win32-cv-1.html
+ */
+typedef struct {
+	LONG waiters;
+	CRITICAL_SECTION waiters_lock;
+	HANDLE sema;
+} pthread_cond_t;
+
+extern int pthread_cond_init(pthread_cond_t *cond, const void *unused);
+
+extern int pthread_cond_destroy(pthread_cond_t *cond);
+
+extern int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex);
+
+extern int pthread_cond_signal(pthread_cond_t *cond);
+
+/*
+ * Simple thread creation implementation using pthread API
+ */
+typedef struct {
+	HANDLE handle;
+	void *(*start_routine)(void*);
+	void *arg;
+} pthread_t;
+
+extern int pthread_create(pthread_t *thread, const void *unused,
+			  void *(*start_routine)(void*), void *arg);
+
+/*
+ * To avoid the need of copying a struct, we use small macro wrapper to pass
+ * pointer to win32_pthread_join instead.
+ */
+#define pthread_join(a, b) win32_pthread_join(&(a), (b))
+
+extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
+
+#endif /* PTHREAD_H */
-- 
1.6.5.2

Re: [PATCH] MSVC: Windows-native implementation for subset of Pthreads API

[Johannes Sixt]

Andrzej K. Haczewski schrieb:
> 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?

Thanks; the result is in

  git://repo.or.cz/git/mingw/j6t.git pthreads-for-windows

-- Hannes