Consider resurrecting the mingw_pread patches

[sschuberth]

Chromium uses a Windows pread performance patch since quite a while, and Stefan Zager had proposed an index-pack threading patch on top of it.

I believe it makes sense to look at integrating this again, and this issue exists so we do not forget about it.

@kblees Would you have time to look at the patch again?

[dscho]

According to https://msdn.microsoft.com/en-us/library/windows/desktop/aa365497%28v=vs.85%29.aspx, the ReOpenFile function is only available starting with Vista, and according to these two issues, we still have a contributor who uses XP himself...

[dscho]

For reference, I cherry-picked Zach's patch: master...pread

[dscho]

I have no physical setup right now to do proper performance tests, but we would really need to do them first. And then, if it is worth it, we can always play games with lazy-loading the ReOpenFile symbol and falling back to the malloc-based emulation.

[kblees]

The NO_THREAD_SAFE_PREAD setting is gone from the Makefile, thus I suspect the problem has already been fixed upstream (e.g. by using Duy's patch [1]).

For reference, upstream discussion is at [2] (gmane seems to be lacking the concluding messages). I also provided a thread-safe mingw_pread based on ReadFile + OVERLAPPED that doesn't require Windows Vista [3].

[1] http://article.gmane.org/gmane.comp.version-control.git/196042
[2] http://git.661346.n2.nabble.com/Make-the-git-codebase-thread-safe-td7603504.html
[3] http://article.gmane.org/gmane.comp.version-control.git/242120

[kblees]

...indeed: 3953949 "index-pack: work around thread-unsafe pread()" merged on 2014-03-25 (53f52cd)

[dscho]

@kblees but we still define NO_PREAD = YesPlease, should we stop that?

[sschuberth]

but we still define NO_PREAD = YesPlease, should we stop that?

I don't think we can stop that yet as there's still no mingw_pread() (neither Karsten's nor Stefan's) in our code as far as I can see. @kblees Or are you saying we do not need a mingw_pread() anymore?

For reference, I cherry-picked Zach's patch

Who's Zach? :-) Also, we probably would want to fix the indentation in that patch before merging it.

[kblees]

but we still define NO_PREAD = YesPlease, should we stop that?

No, we still need NO_PREAD to get git_pread() from compat/pread.c.

We had this discussion last year because the multi-threaded index-pack algorithm required a thread-safe pread() at the time, and git_pread() is not thread-safe.

One possible solution was to provide a thread-safe mingw_pread()...until Duy proposed to change the algorithm so that it no longer requires a thread-safe implementation.

In other words: the git_pread() implentation we get with NO_PREAD is perfectly fine.

[sschuberth]

And we also would not gain any performance in some other area by still providing a thread-safe mingw_pread(), correct?

[kblees]

And we also would not gain any performance in some other area by still providing a thread-safe mingw_pread(), correct?

I don't think so. My version does essentially the same thing as git_pread() (just using Win32 ReadFile() instead of read()). I would think that the ReOpenFile() variant is even slower (due to the performance penalty of opening a new file handle on each call).