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

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 19, 2014, 10:28 UTC
Message-ID: <CACsJy8A7ESSjfHqr96_yYjNsE-A1Sf=8+rmRfGrjML0+fCWTTg@mail.gmail.com>
In-Reply-To: <CAHOQ7J_Wmjo6AJRQra2UDWX3WRboD+-4SaGCHYOUgRR+NyUX4A@mail.gmail.com>

On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager <szager@chromium.org> wrote:

Show 21 quoted lines

> On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, Mar 19, 2014 at 7:46 AM,  <szager@chromium.org> wrote:
>>> This adds a Windows implementation of pread.  Note that it is NOT
>>> safe to intersperse calls to read() and pread() on a file
>>> descriptor.  According to the ReadFile spec, using the 'overlapped'
>>> argument should not affect the implicit position pointer of the
>>> descriptor.  Experiments have shown that this is, in fact, a lie.
>>
>> If I understand it correctly, new pread() is added because
>> compat/pread.c does not work because of some other read() in between?
>> Where are those read() (I can only see one in index-pack.c, but there
>> could be some hidden read()..)
>
> I *think* it's the call to fixup_pack_header_footer(), but I'm not 100% sure.
>
> 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..

-- 
Duy