[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 <[hidden email]>
+ */
+
+#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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Johannes Sixt-2]

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Andrzej K. Haczewski]

2009/11/4 Johannes Schindelin <[hidden email]>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Johannes Sixt-2]

[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Michael Wookey]

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

> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 <[hidden email]>
---
 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 <[hidden email]>
+ */
+
+#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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Erik Faye-Lund]

On Wed, Nov 4, 2009 at 11:37 AM, Andrzej K. Haczewski
<[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Andrzej K. Haczewski]

2009/11/4 Johannes Sixt <[hidden email]>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Andrzej K. Haczewski]

2009/11/4 Erik Faye-Lund <[hidden email]>:

>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Paolo Bonzini-2]

On 11/04/2009 11:50 AM, Erik Faye-Lund wrote:

> On Wed, Nov 4, 2009 at 11:37 AM, Andrzej K. Haczewski
> <[hidden email]>  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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Paolo Bonzini-2]

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Johannes Sixt-2]

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 <[hidden email]>

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 <[hidden email]>
> + */
> +
> +#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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Andrzej K. Haczewski]

2009/11/4 Johannes Sixt <[hidden email]>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Andrzej K. Haczewski]

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

>> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html