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

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Nov 4, 2009, 14:34 UTC
Message-ID: <4AF190F1.3020607@viscovery.net>
In-Reply-To: <16cee31f0911040547m69e5b9cbi30e20d2a7790bd6f@mail.gmail.com>

Andrzej K. Haczewski schrieb:

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 10 quoted lines

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