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

From: Stefan Zager <szager@chromium.org>
Date: Mar 19, 2014, 16:57 UTC
Message-ID: <CAHOQ7J9c_ZfzYEmO861Oa64YZeArQQBMnah1yWAkChME7dA+TA@mail.gmail.com>
In-Reply-To: <CACsJy8A7ESSjfHqr96_yYjNsE-A1Sf=8+rmRfGrjML0+fCWTTg@mail.gmail.com>

On Wed, Mar 19, 2014 at 3:28 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 19 quoted lines

> On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager <szager@chromium.org> wrote:
>>
>> I suppose it would be possible to fix the immediate problem just by
>> using one fd per thread, without a new pread implementation.  But it
>> seems better overall to have a pread() implementation that is
>> thread-safe as long as read() and pread() aren't interspersed; and
>> then convert all existing read() calls to pread().  That would be a
>> good follow-up patch...
>
> I still don't understand how compat/pread.c does not work with pack_fd
> per thread. I don't have Windows to test, but I forced compat/pread.c
> on on Linux with similar pack_fd changes and it worked fine, helgrind
> only complained about progress.c.
>
> A pread() implementation that is thread-safe with condition sounds
> like an invite for trouble later. And I don't think converting read()
> to pread() is a good idea. Platforms that rely on pread() will hit
> first because of more use of compat/pread.c. read() seeks while
> pread() does not, so we have to audit more code..

Using one fd per thread is all well and good for something like index-pack, which only accesses a single pack file. But using that heuristic to add threading elsewhere is probably not going to work. For example, I have a patch in progress to add threading to checkout, and another one planned to add threading to status. In both cases, we would need one fd per thread per pack file, which is pretty ridiculous.

There really aren't very many calls to read() in the code. I don't think it would be very difficult to eliminate the remaining ones. The more interesting question, I think is: what platforms still don't have a thread-safe pread implementation?