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

List:       git
Subject:    Re: [PATCH] Enable index-pack threading in msysgit.
From:       Karsten Blees <karsten.blees () gmail ! com>
Date:       2014-03-20 21:35:09
Message-ID: 532B5F0D.2070300 () gmail ! com
[Download RAW message or body]

Am 20.03.2014 17:08, schrieb Stefan Zager:
> 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.
> 

Correct, a multi-threaded code section using pread has the effect of randomizing the \
file position (btw., this is also true for your pread implementation). This can be \
easily fixed by resetting the file position after pthread_join, if necessary. \
Currently there's just six callers of pthread_join.

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

IMO a "mostly" XSI compliant pread (or even the git_pread() emulation) is still \
better than forbidding the use of read() entirely. Switching from read to pread \
everywhere requires that all callers have to keep track of the file position, which \
means a _lot_ of code changes (read/xread/strbuf_read is used in ~70 places \
throughout git). And how do you plan to deal with platforms that don't have a \
thread-safe pread (HP, Cygwin)?

Considering all that, Duy's solution of opening separate file descriptors per thread \
seems to be the best pattern for future multi-threaded work.

Karsten

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