Re: [PATCH v2 2/2] index-pack: support multithreaded delta resolving

Nguyễn Thái Ngọc Duy wrote:
[snipped]

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 I changed Ramsay's mutex patch a little bit and incorporate it here.
 Ramsay, it'd be great if you could try it again on MinGW

Hmm, well do you want the good news, or the bad news ... :-D

First, I should say that I feel like I'm doing a very bad job of
communicating, so let me apologize for that and hope that this time
I make a better job of it!

This patch breaks the build on MinGW, since the emulation code has not
(thus far) included an implementation of pthread_key_delete(). I simply
commented out the call to that function, in cleanup_thread(), so that I
could test the remainder of the patch.

Although this patch is an improvement on previous patches, it still fails
in *exactly* the same way as earlier attempts.

I probably didn't make clear before that 'nr_threads' has been given too
many duties, which is the main reason for me introducing a new variable
'threads_active'. For example, ...

 builtin/index-pack.c             |  198 ++++++++++++++++++++++++++++++++++----
 3 files changed, 192 insertions(+), 18 deletions(-)

[snipped]

+static inline void lock_mutex(pthread_mutex_t *mutex)
+{
+	if (nr_threads > 1)
+		pthread_mutex_lock(mutex);
+}

What is this condition testing (ie. what does it mean)? Does it mean:

    1. there are some threads currently running ?
    2. the mutex variables are in a usable state ?

Does this expression always express the same invariant?

The answer, of course, is *no*.

Let us consider the call to parse_pack_objects() at line 1367. Let us
suppose that we have been asked to use threads (from the config file,
the command-line, or simply !NO_PTHREADS), so that when we call the
parse_pack_objects() function nr_threads > 1.

Note that, at this point, no threads are active and the mutex variables
have not been initialised.

Now, at the beginning of parse_pack_objects(), we find some 'first pass'
processing [for (i = 0; i < nr_objects; i++) ... lines 839-851], which
includes a call to sha1_object() at line 848. sha1_object() in turn has
an invocation of the read_lock() macro (line 552), which in turn calls
lock_mutex() with a pointer to the read_mutex.

Note that, at this point, no threads are active and the mutex variables
have not been initialised.

Also note that "nr_threads > 1" is true. At this point, nr_threads is still
playing the "this is how many threads I have been requested to create" role.
But again, no threads have been created yet, the mutex variables haven't been
initialised, and ... well, *boom*.

So, in order to get it to work on MinGW (and this time I only tested on MinGW),
I had to apply the patch below (look familiar?).

[I ran the same four tests as before, five times in a row. On *one* occasion
t5300.22 (verify-pack catches a corrupted type/size of the 1st packed object data)
failed because the 'dd' command crashed! So, maybe there is a problem lurking.]

ATB,
Ramsay Jones

-- >8 --

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 7e3b287..6679734 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -69,6 +69,7 @@ static int nr_processed;
 static int nr_deltas;
 static int nr_resolved_deltas;
 static int nr_threads;
+static int threads_active;
 
 static int from_stdin;
 static int strict;

@@ -105,13 +106,13 @@ static pthread_key_t key;
 
 static inline void lock_mutex(pthread_mutex_t *mutex)
 {
-	if (nr_threads > 1)
+	if (threads_active)
 		pthread_mutex_lock(mutex);
 }
 
 static inline void unlock_mutex(pthread_mutex_t *mutex)
 {
-	if (nr_threads > 1)
+	if (threads_active)
 		pthread_mutex_unlock(mutex);
 }
 

@@ -125,14 +126,16 @@ static void init_thread(void)
 	pthread_mutex_init(&work_mutex, NULL);
 	pthread_key_create(&key, NULL);
 	thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+	threads_active = 1;
 }
 
 static void cleanup_thread(void)
 {
+	threads_active = 0;
 	pthread_mutex_destroy(&read_mutex);
 	pthread_mutex_destroy(&counter_mutex);
 	pthread_mutex_destroy(&work_mutex);
-	pthread_key_delete(key);
+	/*pthread_key_delete(key);*/
 	nr_threads = 1;
 	free(thread_data);
 }
-- 8< --