Re: [PATCH v2] Make xmalloc and xrealloc thread-safe

From: Nicolas Pitre <nico@fluxnic.net>
Date: Apr 7, 2010, 13:17 UTC
Message-ID: <alpine.LFD.2.00.1004070859540.7232@xanadu.home>
In-Reply-To: <r2xec874dac1004070529p3d21d23z533e471636194c00@mail.gmail.com>

On Wed, 7 Apr 2010, Shawn Pearce wrote:

Show 9 quoted lines

> On Tue, Apr 6, 2010 at 9:51 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > In practice memset() will optimize the memory access by using words and
> > no bytes.  But in theory this is not guaranteed.  The solution for this
> > would be to have yet another mutex just to protect the read_mutex
> > hownership information modifications in order to make it atomic to
> > potential readers.  That is becoming ugly for a feature (the freeing of
> > pack data) that is not supposed to be the common case.
> 
> Multi-threaded programming is hard.  Its never easy to get it right.

Indeed. But in this case what makes it harder is the willingness to stick to standard APIs.

Show 5 quoted lines

> We had really excellent reasons for avoiding multiple threads in the
> early days of Git.  We still have those excellent reasons, but we have
> been pushing more and more into these async threads to support
> windows, and now its making us realize we never really thought about
> this stuff very much.

Well, pack-objects was "broken" even without Windows in the picture. What I'm trying to fix here is not Windows specific (although the thread API limitations are).

Show 5 quoted lines

> You mentioned avoiding a recursive mutex only because windows
> emulation doesn't have support for it.  But that's exactly what we
> need here.  Shouldn't windows have a recursive mutex object that can
> just be used inside of the emulation layer when we really need a
> recursive mutex?

Maybe. That would in fact just mean pushing the double mutex issue into the pthread emulation instead of having it outside it. This would impact performances for all mutexes although only one instance of them currently require a recursive behavior.

Yet, the memset() issue comes up only because pthread_t is meant to be an opaque type. The only information we would need here is the actual thread ID as returned by gettid() on Linux or GetCurrentThreadId() on Windows, and then the read_mutex_owner could be a simple atomically modifiable integer. But what about other pthread-capable non Linux systems?

Nicolas