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

[Johannes Sixt <j.sixt@viscovery.net>]

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