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

2014-03-26 Thread Johannes Sixt 
Am 3/19/2014 1:46, schrieb sza...@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.  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.
 ---
  builtin/index-pack.c | 21 -
  compat/mingw.c   | 31 ++-
  compat/mingw.h   |  3 +++
  config.mak.uname |  1 -
  4 files changed, 49 insertions(+), 7 deletions(-)

t5302 does not pass with this patch (sz/mingw-index-pack-threaded).
It fails like this:

+ eval 'git index-pack --index-version=1 --stdin  test-1-${pack1}.pack 
 git prune-packed 
 git count-objects | ( read nr rest  test $nr -eq 1 ) 
 cmp test-1-${pack1}.pack .git/objects/pack/pack-${pack1}.pack 
 cmp test-1-${pack1}.idx  .git/objects/pack/pack-${pack1}.idx'
++ git index-pack --index-version=1 --stdin
pack1c54d893dd9bf6645ecee2886ea72f2c2030bea1
++ git prune-packed
error: packfile 
.git/objects/pack/pack-1c54d893dd9bf6645ecee2886ea72f2c2030bea1.pack does not 
match index
warning: packfile 
.git/objects/pack/pack-1c54d893dd9bf6645ecee2886ea72f2c2030bea1.pack cannot be 
accessed
[... these 2 messages repeat ~250 times ...]
++ git count-objects
++ read nr rest
++ test 303 -eq 1

I haven't tested Duy's latest patch (index-pack: work around
thread-unsafe pread() yesterday), yet.

-- Hannes

 
 diff --git a/builtin/index-pack.c b/builtin/index-pack.c
 index 2f37a38..c02dd4c 100644
 --- a/builtin/index-pack.c
 +++ b/builtin/index-pack.c
 @@ -51,6 +51,7 @@ struct thread_local {
  #endif
   struct base_data *base_cache;
   size_t base_cache_used;
 + int pack_fd;
  };
  
  /*
 @@ -91,7 +92,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 +136,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 +144,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 +164,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 +299,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 +553,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 +1501,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

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

2014-03-21 Thread Karsten Blees 
Am 20.03.2014 02:25, schrieb Duy Nguyen:
 On Thu, Mar 20, 2014 at 4:35 AM, Stefan Zager sza...@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).

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-21 Thread Karsten Blees 
Am 21.03.2014 06:35, schrieb Stefan Zager:
 On Thu, Mar 20, 2014 at 10:21 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Fri, Mar 21, 2014 at 08:51:18AM +0700, Duy Nguyen wrote:
 On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager sza...@chromium.org wrote:
 Duy, would you like to re-post your patch without the new pread 
 implementation?

 I will but let me try out the sliding window idea first. My quick
 tests on git.git show me we may only need 21k mmap instead of 177k
 pread. That hints some potential performance improvement.

 The patch at the bottom reuses (un)use_pack() instead of pread(). The
 results on linux-2.6 do not look any different. I guess we can drop
 the idea.

 It makes me wonder, though, what's wrong a simple patch like this to
 make pread in index-pack thread-safe? It does not look any different
 either from the performance point of view, perhaps because
 unpack_data() reads small deltas most of the time
 
 When you serialize disk access in this way, the effect on performance
 is really dependent on the behavior of the OS, as well as the locality
 of the read offsets.  Assuming -- fairly, I think -- that the reads
 will be pretty randomly distributed (i.e., no locality to speak of),
 then your best bet is get as many read operations in flight as
 possible, and let the disk scheduler optimize the seek time.
 

The read() implementation in MSVCRT.DLL is synchronized anyway, and I strongly 
suspect that this is also true for ReadFile() (at least for synchronous file 
handles, i.e. opened without FILE_FLAG_OVERLAPPED). So I guess separate file 
descriptors would help with parallel IO as well.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-21 Thread Karsten Blees 
Am 20.03.2014 22:56, schrieb Stefan Zager:
 On Thu, Mar 20, 2014 at 2:35 PM, Karsten Blees karsten.bl...@gmail.com 
 wrote:
 Am 20.03.2014 17:08, schrieb Stefan Zager:

 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.
 
 Does that mean you would endorse the (N threads) * (M pack files)
 approach to threading checkout and status?  That seems kind of
 crazy-town to me.  Not to mention that pack windows are not shared, so
 this approach to multi-threading can have the side-effect of blowing
 out memory consumption.  We have already had to dial back settings for
 pack.threads and core.deltaBaseCacheLimit, because threaded index-pack
 was causing OOM errors on 32-bit platforms.
 

Opening more file descriptors doesn't significantly increase the memory 
footprint, so it shouldn't matter whether the threads read data via shared or 
private descriptors.

git-status with core.preloadindex is already multithreaded (at least the first 
part), and AFAIK doesn't read pack files at all.

I'm still not convinced that multi-threaded git-checkout is a good idea. 
According to my tests this is actually slower than sequential checkout. You'd 
have to be very careful to only multi-thread the parts that don't do any IO, 
such as unpacking / undeltifying.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Karsten Blees 
Am 19.03.2014 01:46, schrieb sza...@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

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

Duy's patch alone enables multi-threaded index-pack on all platforms (including 
cygwin), so IMO this should be a separate patch.

 + if (hand == INVALID_HANDLE_VALUE) {
 + errno = EBADF;
 + return -1;
 + }

This check is redundant, ReadFile already ckecks for invalid handles and 
err_win_to_posix converts to EBADF.

 +
 + 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)

According to MSDN docs, ReadFile never fails with ERROR_HANDLE_EOF, or is this 
another case where the documentation is wrong?

When a synchronous read operation reaches the end of a file, ReadFile returns 
TRUE and sets *lpNumberOfBytesRead to zero.

Karsten
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Stefan Zager 
On Thu, Mar 20, 2014 at 6:54 AM, Karsten Blees karsten.bl...@gmail.com wrote:
 Am 19.03.2014 01:46, schrieb sza...@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.

 Duy's patch alone enables multi-threaded index-pack on all platforms 
 (including cygwin), so IMO this should be a separate patch.

Fair enough, and it's a good first step.  I would love to see it
landed soon.  On the Chrome project, we're currently distributing a
patched version of msysgit; I would very much like for us to stop
doing that, but the performance penalty is too significant right now.

Duy, would you like to re-post your patch without the new pread implementation?

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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Karsten Blees 
Am 20.03.2014 17:08, schrieb Stefan Zager:
 On Thu, Mar 20, 2014 at 6:54 AM, Karsten Blees karsten.bl...@gmail.com 
 wrote:
 Am 19.03.2014 01:46, schrieb sza...@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.

 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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Stefan Zager 
On Thu, Mar 20, 2014 at 2:35 PM, Karsten Blees karsten.bl...@gmail.com wrote:
 Am 20.03.2014 17:08, schrieb Stefan Zager:

 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.

Does that mean you would endorse the (N threads) * (M pack files)
approach to threading checkout and status?  That seems kind of
crazy-town to me.  Not to mention that pack windows are not shared, so
this approach to multi-threading can have the side-effect of blowing
out memory consumption.  We have already had to dial back settings for
pack.threads and core.deltaBaseCacheLimit, because threaded index-pack
was causing OOM errors on 32-bit platforms.

Cygwin (and MSVC) should be able to share a mostly compliant pread
implementation.  I don't have any insight into NonstopKernel; does is
really not have a thread-safe pread implementation?  If so, then I
suppose we have to #ifdef NO_PREAD, just as we do now.

I realize that these are deep changes.  However, the performance of
msysgit on the chromium repositories is pretty awful, enough so to
motivate this work.

Stefan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Duy Nguyen 
On Fri, Mar 21, 2014 at 4:56 AM, Stefan Zager sza...@chromium.org wrote:
 Considering all that, Duy's solution of opening separate file descriptors 
 per thread seems to be the best pattern for future multi-threaded work.

 Does that mean you would endorse the (N threads) * (M pack files)
 approach to threading checkout and status?  That seems kind of
 crazy-town to me.  Not to mention that pack windows are not shared, so
 this approach to multi-threading can have the side-effect of blowing
 out memory consumption.

Maybe we could protect and share the delta cache. Pack windows are
mmap'd so we should not need to worry about their memory consumption.

 We have already had to dial back settings for
 pack.threads and core.deltaBaseCacheLimit, because threaded index-pack
 was causing OOM errors on 32-bit platforms.

Hm.. I don't think index-pack uses sha1_file.c heavily. Local pack
access is only needed for verifying identical objects (and that should
never happen often). Something is fishy with these OOM errors.

 Cygwin (and MSVC) should be able to share a mostly compliant pread
 implementation.  I don't have any insight into NonstopKernel; does is
 really not have a thread-safe pread implementation?  If so, then I
 suppose we have to #ifdef NO_PREAD, just as we do now.

 I realize that these are deep changes.  However, the performance of
 msysgit on the chromium repositories is pretty awful, enough so to
 motivate this work.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Duy Nguyen 
On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager sza...@chromium.org wrote:
 Duy, would you like to re-post your patch without the new pread 
 implementation?

I will but let me try out the sliding window idea first. My quick
tests on git.git show me we may only need 21k mmap instead of 177k
pread. That hints some potential performance improvement.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-20 Thread Duy Nguyen 
On Fri, Mar 21, 2014 at 08:51:18AM +0700, Duy Nguyen wrote:
 On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager sza...@chromium.org wrote:
  Duy, would you like to re-post your patch without the new pread 
  implementation?
 
 I will but let me try out the sliding window idea first. My quick
 tests on git.git show me we may only need 21k mmap instead of 177k
 pread. That hints some potential performance improvement.

The patch at the bottom reuses (un)use_pack() instead of pread(). The
results on linux-2.6 do not look any different. I guess we can drop
the idea.

It makes me wonder, though, what's wrong a simple patch like this to
make pread in index-pack thread-safe? It does not look any different
either from the performance point of view, perhaps because
unpack_data() reads small deltas most of the time

-- 8 --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a6b1c17..b91f4f8 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -40,11 +40,6 @@ 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;
@@ -175,6 +170,22 @@ static void cleanup_thread(void)
 #endif
 
 
+#if defined(NO_THREAD_SAFE_PREAD)
+static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)
+{
+   int ret;
+   read_lock();
+   ret = pread(fd, buf, count, off);
+   read_unlock();
+   return ret;
+}
+#else
+static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)
+{
+   return pread(fd, buf, count, off);
+}
+#endif
+
 static int mark_link(struct object *obj, int type, void *data)
 {
if (!obj)
@@ -542,7 +553,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_safe(pack_fd, inbuf, n, from);
if (n  0)
die_errno(_(cannot pread pack file));
if (!n)
-- 8 --

And the sliding window patch for the list archive

-- 8 --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a6b1c17..6f5c6d9 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -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 int input_fd, output_fd;
+static struct packed_git *pack;
 
 #ifndef NO_PTHREADS
 
@@ -224,8 +225,10 @@ static unsigned check_objects(void)
 static void flush(void)
 {
if (input_offset) {
-   if (output_fd = 0)
+   if (output_fd = 0) {
write_or_die(output_fd, input_buffer, input_offset);
+   pack-pack_size += input_offset;
+   }
git_SHA1_Update(input_ctx, input_buffer, input_offset);
memmove(input_buffer, input_buffer + input_offset, input_len);
input_offset = 0;
@@ -277,6 +280,10 @@ static void use(int bytes)
 
 static const char *open_pack_file(const char *pack_name)
 {
+   pack = xmalloc(sizeof(*pack) + 1);
+   memset(pack, 0, sizeof(*pack));
+   pack-pack_name[0] = '\0';
+
if (from_stdin) {
input_fd = 0;
if (!pack_name) {
@@ -288,13 +295,17 @@ 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;
+   pack-pack_fd = output_fd;
} else {
+   struct stat st;
input_fd = open(pack_name, O_RDONLY);
if (input_fd  0)
die_errno(_(cannot open packfile '%s'), pack_name);
+   if (lstat(pack_name, st))
+   die_errno(_(cannot stat packfile '%s'), pack_name);
output_fd = -1;
-   pack_fd = input_fd;
+   pack-pack_fd = input_fd;
+   pack-pack_size = st.st_size;
}
git_SHA1_Init(input_ctx);
return pack_name;
@@ -531,9 +542,15 @@ static void *unpack_data(struct object_entry *obj,
unsigned char *data, *inbuf;
git_zstream stream;
int status;
+   struct pack_window *w_cursor = NULL;
+
+   if (from + len  pack-pack_size)
+   die(Q_(premature end of pack file, %lu byte missing,
+  premature end of pack file, %lu bytes missing,
+  from + len - pack-pack_size),
+   (unsigned long)(from + len - pack-pack_size));
 
data = xmalloc(consume ? 64*1024 : obj-size);
-   inbuf = xmalloc((len  64*1024) ? len :

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

2014-03-20 Thread Stefan Zager 
On Thu, Mar 20, 2014 at 10:21 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Fri, Mar 21, 2014 at 08:51:18AM +0700, Duy Nguyen wrote:
 On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager sza...@chromium.org wrote:
  Duy, would you like to re-post your patch without the new pread 
  implementation?

 I will but let me try out the sliding window idea first. My quick
 tests on git.git show me we may only need 21k mmap instead of 177k
 pread. That hints some potential performance improvement.

 The patch at the bottom reuses (un)use_pack() instead of pread(). The
 results on linux-2.6 do not look any different. I guess we can drop
 the idea.

 It makes me wonder, though, what's wrong a simple patch like this to
 make pread in index-pack thread-safe? It does not look any different
 either from the performance point of view, perhaps because
 unpack_data() reads small deltas most of the time

When you serialize disk access in this way, the effect on performance
is really dependent on the behavior of the OS, as well as the locality
of the read offsets.  Assuming -- fairly, I think -- that the reads
will be pretty randomly distributed (i.e., no locality to speak of),
then your best bet is get as many read operations in flight as
possible, and let the disk scheduler optimize the seek time.

If I have a chance, I will try out this patch in msysgit on the
chromium repositories.


 -- 8 --
 diff --git a/builtin/index-pack.c b/builtin/index-pack.c
 index a6b1c17..b91f4f8 100644
 --- a/builtin/index-pack.c
 +++ b/builtin/index-pack.c
 @@ -40,11 +40,6 @@ 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;
 @@ -175,6 +170,22 @@ static void cleanup_thread(void)
  #endif


 +#if defined(NO_THREAD_SAFE_PREAD)
 +static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)
 +{
 +   int ret;
 +   read_lock();
 +   ret = pread(fd, buf, count, off);
 +   read_unlock();
 +   return ret;
 +}
 +#else
 +static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)
 +{
 +   return pread(fd, buf, count, off);
 +}
 +#endif
 +
  static int mark_link(struct object *obj, int type, void *data)
  {
 if (!obj)
 @@ -542,7 +553,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_safe(pack_fd, inbuf, n, from);
 if (n  0)
 die_errno(_(cannot pread pack file));
 if (!n)
 -- 8 --

 And the sliding window patch for the list archive

 -- 8 --
 diff --git a/builtin/index-pack.c b/builtin/index-pack.c
 index a6b1c17..6f5c6d9 100644
 --- a/builtin/index-pack.c
 +++ b/builtin/index-pack.c
 @@ -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 int input_fd, output_fd;
 +static struct packed_git *pack;

  #ifndef NO_PTHREADS

 @@ -224,8 +225,10 @@ static unsigned check_objects(void)
  static void flush(void)
  {
 if (input_offset) {
 -   if (output_fd = 0)
 +   if (output_fd = 0) {
 write_or_die(output_fd, input_buffer, input_offset);
 +   pack-pack_size += input_offset;
 +   }
 git_SHA1_Update(input_ctx, input_buffer, input_offset);
 memmove(input_buffer, input_buffer + input_offset, input_len);
 input_offset = 0;
 @@ -277,6 +280,10 @@ static void use(int bytes)

  static const char *open_pack_file(const char *pack_name)
  {
 +   pack = xmalloc(sizeof(*pack) + 1);
 +   memset(pack, 0, sizeof(*pack));
 +   pack-pack_name[0] = '\0';
 +
 if (from_stdin) {
 input_fd = 0;
 if (!pack_name) {
 @@ -288,13 +295,17 @@ 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;
 +   pack-pack_fd = output_fd;
 } else {
 +   struct stat st;
 input_fd = open(pack_name, O_RDONLY);
 if (input_fd  0)
 die_errno(_(cannot open packfile '%s'), pack_name);
 +   if (lstat(pack_name, st))
 +   die_errno(_(cannot stat packfile '%s'), pack_name);
 output_fd = -1;
 -   pack_fd = input_fd;
 +   pack-pack_fd = input_fd;
 +   pack-pack_size = st.st_size;
 }

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

2014-03-19 Thread Duy Nguyen 
On Wed, Mar 19, 2014 at 7:46 AM,  sza...@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()..)


 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.
 ---
  builtin/index-pack.c | 21 -
  compat/mingw.c   | 31 ++-
  compat/mingw.h   |  3 +++
  config.mak.uname |  1 -
  4 files changed, 49 insertions(+), 7 deletions(-)

 diff --git a/builtin/index-pack.c b/builtin/index-pack.c
 index 2f37a38..c02dd4c 100644
 --- a/builtin/index-pack.c
 +++ b/builtin/index-pack.c
 @@ -51,6 +51,7 @@ struct thread_local {
  #endif
 struct base_data *base_cache;
 size_t base_cache_used;
 +   int pack_fd;
  };

  /*
 @@ -91,7 +92,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 +136,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 +144,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 +164,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 +299,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 +553,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 +1501,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..6cc85d6 100644
 --- a/compat/mingw.c
 +++ b/compat/mingw.c
 @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)
 return ret;
  }

 -int mingw_open (const char *filename, int oflags, ...)
 +
 +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 =

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

2014-03-19 Thread Stefan Zager 
On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Wed, Mar 19, 2014 at 7:46 AM,  sza...@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...




 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.
 ---
  builtin/index-pack.c | 21 -
  compat/mingw.c   | 31 ++-
  compat/mingw.h   |  3 +++
  config.mak.uname |  1 -
  4 files changed, 49 insertions(+), 7 deletions(-)

 diff --git a/builtin/index-pack.c b/builtin/index-pack.c
 index 2f37a38..c02dd4c 100644
 --- a/builtin/index-pack.c
 +++ b/builtin/index-pack.c
 @@ -51,6 +51,7 @@ struct thread_local {
  #endif
 struct base_data *base_cache;
 size_t base_cache_used;
 +   int pack_fd;
  };

  /*
 @@ -91,7 +92,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 +136,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 +144,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 +164,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 +299,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 +553,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 +1501,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..6cc85d6

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

2014-03-19 Thread Duy Nguyen 
On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager sza...@chromium.org wrote:
 On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Wed, Mar 19, 2014 at 7:46 AM,  sza...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-19 Thread Stefan Zager 
On Wed, Mar 19, 2014 at 3:28 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager sza...@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-19 Thread Stefan Zager 
On Wed, Mar 19, 2014 at 9:57 AM, Stefan Zager sza...@chromium.org wrote:

 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?

I don't want to go too deep down the rabbit hole here.  We don't have
to solve the read() vs. pread() issue once for all right now; that can
wait for another day.  The pread() implementation in this patch is
certainly no worse than the one in compat/pread.c.

Stefan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-03-19 Thread Junio C Hamano 
sza...@chromium.org 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.
 ---

Sign-off?

The new per-thread file descriptors to the same thing in a generic
codepath is a bit of eyesore.  For index-pack, keeping as many file
descritors open to the current pack as the worker threads are should
not be too bad, but could we have some comment next to the field
definition please (e.g. Windows emulation of pread() needs separate
fd per thread; see $URL for details or something)?

  builtin/index-pack.c | 21 -
  compat/mingw.c   | 31 ++-
  compat/mingw.h   |  3 +++
  config.mak.uname |  1 -
  4 files changed, 49 insertions(+), 7 deletions(-)

 diff --git a/builtin/index-pack.c b/builtin/index-pack.c
 index 2f37a38..c02dd4c 100644
 --- a/builtin/index-pack.c
 +++ b/builtin/index-pack.c
 @@ -51,6 +51,7 @@ struct thread_local {
  #endif
   struct base_data *base_cache;
   size_t base_cache_used;
 + int pack_fd;
  };
  
  /*
 @@ -91,7 +92,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 +136,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 +144,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 +164,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 +299,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 +553,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 +1501,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..6cc85d6 100644
 --- a/compat/mingw.c
 +++ b/compat/mingw.c
 @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)
   return ret;
  }
  
 -int mingw_open (const char *filename, int oflags, ...)
 +
 +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;
 +

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

2014-03-19 Thread Junio C Hamano 
sza...@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 sza...@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
 +++

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

2014-03-19 Thread Duy Nguyen 
On Thu, Mar 20, 2014 at 4:35 AM, Stefan Zager sza...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html