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

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Nguyễn Thái Ngọc Duy wrote:
[snipped]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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< --