Re: [PATCH] Enable index-pack threading in msysgit.

From: Stefan Zager <szager@chromium.org>
Date: Mar 20, 2014, 16:08 UTC
Message-ID: <CAHOQ7J9drXwcTt4b0Tcyw97KTGcifwsO5rtFNQYf7CVr3WD7zQ@mail.gmail.com>
In-Reply-To: <532AF304.7040301@gmail.com>

On Thu, Mar 20, 2014 at 6:54 AM, Karsten Blees <karsten.blees@gmail.com> wrote:

Show 10 quoted lines

> Am 19.03.2014 01:46, schrieb szager@chromium.org:
>> This adds a Windows implementation of pread.  Note that it is NOT
>> safe to intersperse calls to read() and pread() on a file
>> descriptor.
>
> This is a bad idea. You're basically fixing the multi-threaded issue twice, while at the same time breaking single-threaded read/pread interop on the mingw and msvc platform. Users of pread already have to take care that its not thread-safe on some platforms, now you're adding another breakage that has to be considered in future development.
>
> The mingw_pread implementation in [1] is both thread-safe and allows mixing read/pread in single-threaded scenarios, why not use this instead?
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/242120

That's not thread-safe. There is, presumably, a point between the first and second calls to lseek64 when the implicit position pointer is incorrect. If another thread executes its first call to lseek64 at that time, then the file descriptor may end up with an incorrect position pointer after all threads have finished.

> Duy's patch alone enables multi-threaded index-pack on all platforms (including cygwin), so IMO this should be a separate patch.

Fair enough, and it's a good first step. I would love to see it landed soon. On the Chrome project, we're currently distributing a patched version of msysgit; I would very much like for us to stop doing that, but the performance penalty is too significant right now.

Duy, would you like to re-post your patch without the new pread implementation?

Going forward, there is still a lot of performance that gets left on the table when you rule out threaded file access. There are not so many calls to read, mmap, and pread in the code; it should be possible to rationalize them and make them thread-safe -- at least, thread-safe for posix-compliant systems and msysgit, which covers the great majority of git users, I would hope.