Re: [PATCH] Enable index-pack threading in msysgit.
Am 20.03.2014 17:08, schrieb Stefan Zager:
Show 19 quoted lines
> 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.
Show 7 quoted lines
> 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
Karsten Blees <karsten.blees@gmail.com>