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

From: Andrzej K. Haczewski <ahaczewski@gmail.com>
Date: Nov 4, 2009, 13:47 UTC
Message-ID: <16cee31f0911040547m69e5b9cbi30e20d2a7790bd6f@mail.gmail.com>
In-Reply-To: <4AF175E8.7020400@viscovery.net>

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.

Show 10 quoted lines

> 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