[PATCH] Enable index-pack threading in msysgit.

[PATCH] Enable index-pack threading in msysgit.

[Stefan Zager]

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.

To accomodate that fact, this change also incorporates:

http://article.gmane.org/gmane.comp.version-control.git/196042

... which gives each index-pack thread its own file descriptor.

Signed-off-by: Stefan Zager <szager@chromium.org>
---
 builtin/index-pack.c | 30 ++++++++++++++++++++----------
 compat/mingw.c       | 37 ++++++++++++++++++++++++++++++++++++-
 compat/mingw.h       |  3 +++
 config.mak.uname     |  1 -
 4 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38..63b8b0e 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -40,17 +40,17 @@ struct base_data {
 	int ofs_first, ofs_last;
 };
 
-#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
-/* pread() emulation is not thread-safe. Disable threading. */
-#define NO_PTHREADS
-#endif
-
 struct thread_local {
 #ifndef NO_PTHREADS
 	pthread_t thread;
 #endif
 	struct base_data *base_cache;
 	size_t base_cache_used;
+    /*
+     * To accomodate platforms that have pthreads, but don't have a
+     * thread-safe pread, give each thread its own file descriptor.
+     */
+	int pack_fd;
 };
 
 /*
@@ -91,7 +91,8 @@ static off_t consumed_bytes;
 static unsigned deepest_delta;
 static git_SHA_CTX input_ctx;
 static uint32_t input_crc32;
-static int input_fd, output_fd, pack_fd;
+static const char *curr_pack;
+static int input_fd, output_fd;
 
 #ifndef NO_PTHREADS
 
@@ -134,6 +135,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
  */
 static void init_thread(void)
 {
+	int i;
 	init_recursive_mutex(&read_mutex);
 	pthread_mutex_init(&counter_mutex, NULL);
 	pthread_mutex_init(&work_mutex, NULL);
@@ -141,11 +143,17 @@ static void init_thread(void)
 		pthread_mutex_init(&deepest_delta_mutex, NULL);
 	pthread_key_create(&key, NULL);
 	thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+	for (i = 0; i < nr_threads; i++) {
+		thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
+		if (thread_data[i].pack_fd == -1)
+			die_errno("unable to open %s", curr_pack);
+	}
 	threads_active = 1;
 }
 
 static void cleanup_thread(void)
 {
+	int i;
 	if (!threads_active)
 		return;
 	threads_active = 0;
@@ -155,6 +163,8 @@ static void cleanup_thread(void)
 	if (show_stat)
 		pthread_mutex_destroy(&deepest_delta_mutex);
 	pthread_key_delete(key);
+	for (i = 0; i < nr_threads; i++)
+		close(thread_data[i].pack_fd);
 	free(thread_data);
 }
 
@@ -288,13 +298,13 @@ static const char *open_pack_file(const char *pack_name)
 			output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
 		if (output_fd < 0)
 			die_errno(_("unable to create '%s'"), pack_name);
-		pack_fd = output_fd;
+		nothread_data.pack_fd = output_fd;
 	} else {
 		input_fd = open(pack_name, O_RDONLY);
 		if (input_fd < 0)
 			die_errno(_("cannot open packfile '%s'"), pack_name);
 		output_fd = -1;
-		pack_fd = input_fd;
+		nothread_data.pack_fd = input_fd;
 	}
 	git_SHA1_Init(&input_ctx);
 	return pack_name;
@@ -542,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
 
 	do {
 		ssize_t n = (len < 64*1024) ? len : 64*1024;
-		n = pread(pack_fd, inbuf, n, from);
+		n = pread(get_thread_data()->pack_fd, inbuf, n, from);
 		if (n < 0)
 			die_errno(_("cannot pread pack file"));
 		if (!n)
@@ -1490,7 +1500,7 @@ static void show_pack_info(int stat_only)
 int cmd_index_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
-	const char *curr_pack, *curr_index;
+	const char *curr_index;
 	const char *index_name = NULL, *pack_name = NULL;
 	const char *keep_name = NULL, *keep_msg = NULL;
 	char *index_name_buf = NULL, *keep_name_buf = NULL;
diff --git a/compat/mingw.c b/compat/mingw.c
index 383cafe..0efc570 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -329,7 +329,42 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
-int mingw_open (const char *filename, int oflags, ...)
+
+/*
+ * Warning: contrary to the specificiation, when ReadFile() is called
+ * with an 'overlapped' argument, it *will* modify the implict position
+ * pointer for the file descriptor.  As a result, it is *not* safe to
+ * intersperse calls to read() and pread() on a single file descriptor.
+ */
+ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
+{
+	HANDLE hand = (HANDLE)_get_osfhandle(fd);
+	if (hand == INVALID_HANDLE_VALUE) {
+		errno = EBADF;
+		return -1;
+	}
+
+	LARGE_INTEGER offset_value;
+	offset_value.QuadPart = offset;
+
+	DWORD bytes_read = 0;
+	OVERLAPPED overlapped = {0};
+	overlapped.Offset = offset_value.LowPart;
+	overlapped.OffsetHigh = offset_value.HighPart;
+	BOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);
+
+	ssize_t ret = bytes_read;
+
+	if (!result && GetLastError() != ERROR_HANDLE_EOF)
+	{
+		errno = err_win_to_posix(GetLastError());
+		ret = -1;
+	}
+
+	return ret;
+}
+
+int mingw_open(const char *filename, int oflags, ...)
 {
 	va_list args;
 	unsigned mode;
diff --git a/compat/mingw.h b/compat/mingw.h
index 08b83fe..377ba50 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);
 int mingw_rmdir(const char *path);
 #define rmdir mingw_rmdir
 
+ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);
+#define pread mingw_pread
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
diff --git a/config.mak.uname b/config.mak.uname
index e8acc39..b405524 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 endif
 ifneq (,$(findstring MINGW,$(uname_S)))
 	pathsep = ;
-	NO_PREAD = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
-- 
1.9.0.279.gdc9e3eb

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

[Junio C Hamano]

szager@chromium.org (Stefan Zager) writes:

> 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.
>
> To accomodate that fact, this change also incorporates:
>
> http://article.gmane.org/gmane.comp.version-control.git/196042
>
> ... which gives each index-pack thread its own file descriptor.
>
> Signed-off-by: Stefan Zager <szager@chromium.org>
> ---

I'll queue it on 'pu' until I hear from Windows folks.
There were a few things I tweaked while queuing, tho.

 - the indentation of the new comment inside struct thread_local
   declaration looked strange;

 - there was one new if () statement whose block was opened on the
   next line, not on the same line as if () itself.

Thanks.

>  builtin/index-pack.c | 30 ++++++++++++++++++++----------
>  compat/mingw.c       | 37 ++++++++++++++++++++++++++++++++++++-
>  compat/mingw.h       |  3 +++
>  config.mak.uname     |  1 -
>  4 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 2f37a38..63b8b0e 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -40,17 +40,17 @@ struct base_data {
>  	int ofs_first, ofs_last;
>  };
>  
> -#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
> -/* pread() emulation is not thread-safe. Disable threading. */
> -#define NO_PTHREADS
> -#endif
> -
>  struct thread_local {
>  #ifndef NO_PTHREADS
>  	pthread_t thread;
>  #endif
>  	struct base_data *base_cache;
>  	size_t base_cache_used;
> +    /*
> +     * To accomodate platforms that have pthreads, but don't have a
> +     * thread-safe pread, give each thread its own file descriptor.
> +     */
> +	int pack_fd;
>  };
>  
>  /*
> @@ -91,7 +91,8 @@ static off_t consumed_bytes;
>  static unsigned deepest_delta;
>  static git_SHA_CTX input_ctx;
>  static uint32_t input_crc32;
> -static int input_fd, output_fd, pack_fd;
> +static const char *curr_pack;
> +static int input_fd, output_fd;
>  
>  #ifndef NO_PTHREADS
>  
> @@ -134,6 +135,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
>   */
>  static void init_thread(void)
>  {
> +	int i;
>  	init_recursive_mutex(&read_mutex);
>  	pthread_mutex_init(&counter_mutex, NULL);
>  	pthread_mutex_init(&work_mutex, NULL);
> @@ -141,11 +143,17 @@ static void init_thread(void)
>  		pthread_mutex_init(&deepest_delta_mutex, NULL);
>  	pthread_key_create(&key, NULL);
>  	thread_data = xcalloc(nr_threads, sizeof(*thread_data));
> +	for (i = 0; i < nr_threads; i++) {
> +		thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
> +		if (thread_data[i].pack_fd == -1)
> +			die_errno("unable to open %s", curr_pack);
> +	}
>  	threads_active = 1;
>  }
>  
>  static void cleanup_thread(void)
>  {
> +	int i;
>  	if (!threads_active)
>  		return;
>  	threads_active = 0;
> @@ -155,6 +163,8 @@ static void cleanup_thread(void)
>  	if (show_stat)
>  		pthread_mutex_destroy(&deepest_delta_mutex);
>  	pthread_key_delete(key);
> +	for (i = 0; i < nr_threads; i++)
> +		close(thread_data[i].pack_fd);
>  	free(thread_data);
>  }
>  
> @@ -288,13 +298,13 @@ static const char *open_pack_file(const char *pack_name)
>  			output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
>  		if (output_fd < 0)
>  			die_errno(_("unable to create '%s'"), pack_name);
> -		pack_fd = output_fd;
> +		nothread_data.pack_fd = output_fd;
>  	} else {
>  		input_fd = open(pack_name, O_RDONLY);
>  		if (input_fd < 0)
>  			die_errno(_("cannot open packfile '%s'"), pack_name);
>  		output_fd = -1;
> -		pack_fd = input_fd;
> +		nothread_data.pack_fd = input_fd;
>  	}
>  	git_SHA1_Init(&input_ctx);
>  	return pack_name;
> @@ -542,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
>  
>  	do {
>  		ssize_t n = (len < 64*1024) ? len : 64*1024;
> -		n = pread(pack_fd, inbuf, n, from);
> +		n = pread(get_thread_data()->pack_fd, inbuf, n, from);
>  		if (n < 0)
>  			die_errno(_("cannot pread pack file"));
>  		if (!n)
> @@ -1490,7 +1500,7 @@ static void show_pack_info(int stat_only)
>  int cmd_index_pack(int argc, const char **argv, const char *prefix)
>  {
>  	int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
> -	const char *curr_pack, *curr_index;
> +	const char *curr_index;
>  	const char *index_name = NULL, *pack_name = NULL;
>  	const char *keep_name = NULL, *keep_msg = NULL;
>  	char *index_name_buf = NULL, *keep_name_buf = NULL;
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 383cafe..0efc570 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -329,7 +329,42 @@ int mingw_mkdir(const char *path, int mode)
>  	return ret;
>  }
>  
> -int mingw_open (const char *filename, int oflags, ...)
> +
> +/*
> + * Warning: contrary to the specificiation, when ReadFile() is called
> + * with an 'overlapped' argument, it *will* modify the implict position
> + * pointer for the file descriptor.  As a result, it is *not* safe to
> + * intersperse calls to read() and pread() on a single file descriptor.
> + */
> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
> +{
> +	HANDLE hand = (HANDLE)_get_osfhandle(fd);
> +	if (hand == INVALID_HANDLE_VALUE) {
> +		errno = EBADF;
> +		return -1;
> +	}
> +
> +	LARGE_INTEGER offset_value;
> +	offset_value.QuadPart = offset;
> +
> +	DWORD bytes_read = 0;
> +	OVERLAPPED overlapped = {0};
> +	overlapped.Offset = offset_value.LowPart;
> +	overlapped.OffsetHigh = offset_value.HighPart;
> +	BOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);
> +
> +	ssize_t ret = bytes_read;
> +
> +	if (!result && GetLastError() != ERROR_HANDLE_EOF)
> +	{
> +		errno = err_win_to_posix(GetLastError());
> +		ret = -1;
> +	}
> +
> +	return ret;
> +}
> +
> +int mingw_open(const char *filename, int oflags, ...)
>  {
>  	va_list args;
>  	unsigned mode;
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 08b83fe..377ba50 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);
>  int mingw_rmdir(const char *path);
>  #define rmdir mingw_rmdir
>  
> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);
> +#define pread mingw_pread
> +
>  int mingw_open (const char *filename, int oflags, ...);
>  #define open mingw_open
>  
> diff --git a/config.mak.uname b/config.mak.uname
> index e8acc39..b405524 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  endif
>  ifneq (,$(findstring MINGW,$(uname_S)))
>  	pathsep = ;
> -	NO_PREAD = YesPlease
>  	NEEDS_CRYPTO_WITH_SSL = YesPlease
>  	NO_LIBGEN_H = YesPlease
>  	NO_POLL = YesPlease

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

[Duy Nguyen]

On Thu, Mar 20, 2014 at 4:35 AM, Stefan Zager <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.
>
> To accomodate that fact, this change also incorporates:
>
> http://article.gmane.org/gmane.comp.version-control.git/196042
>
> ... which gives each index-pack thread its own file descriptor.

If the problem is mixing read() and pread() then perhaps it's enough to do

output_fd = dup(output_fd);

after pack_fd is set in open_pack_file(), to make sure that
fixup_pack_header_footer() has its own file handle. If that works, we
don't need one pack_fd per thread.

compat/mmap.c uses pread() and its bad interaction with read() could
turn it into a nightmare. Fortunately Windows (except Cygwin) does not
use this implementation. Not sure if we should make a note about this.

It makes me wonder if sliding mmap window (like we do for pack access
in sha1_file.c) would be better than pread(). index-pack used to do
mmap() [1] in the past with poor performance but I don't think sliding
window was mentioned.

[1] http://thread.gmane.org/gmane.comp.version-control.git/34741/focus=34832

> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  endif
>  ifneq (,$(findstring MINGW,$(uname_S)))
>         pathsep = ;
> -       NO_PREAD = YesPlease
>         NEEDS_CRYPTO_WITH_SSL = YesPlease
>         NO_LIBGEN_H = YesPlease
>         NO_POLL = YesPlease

What about the "ifeq ($(uname_S),Windows)" section? I think MSVC and
MinGW builds share a lot of code.
-- 
Duy

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

[Karsten Blees]

Am 20.03.2014 02:25, schrieb Duy Nguyen:
> On Thu, Mar 20, 2014 at 4:35 AM, Stefan Zager <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.
>>
>> To accomodate that fact, this change also incorporates:
>>
>> http://article.gmane.org/gmane.comp.version-control.git/196042
>>
>> ... which gives each index-pack thread its own file descriptor.
> 
> If the problem is mixing read() and pread() then perhaps it's enough to do
> 
> output_fd = dup(output_fd);
> 

Unfortunately not, dup() / DuplicateHandle() just opens another handle to the same file object (i.e. sharing the same file position).

[BUG] 'pread' : macro redefinition

[Marat Radchenko]

Stefan Zager <szager <at> chromium.org> writes:

> 
> This adds a Windows implementation of pread.
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 08b83fe..377ba50 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);
> +#define pread mingw_pread

This result in tons of following warnings in MSVC=1 build:

git-compat-util.h(401) : warning C4005: 'pread' : macro redefinition
mingw.h(181) : see previous definition of 'pread'