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@xxxxxxxxx>
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@xxxxxxxxx>
> + */
> +
> +#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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html