[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] Enable index-pack threading in msysgit.
From:       Stefan Zager <szager () chromium ! org>
Date:       2014-03-20 16:08:04
Message-ID: CAHOQ7J9drXwcTt4b0Tcyw97KTGcifwsO5rtFNQYf7CVr3WD7zQ () mail ! gmail ! com
[Download RAW message or body]

On Thu, Mar 20, 2014 at 6:54 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]