threads / patch / 22666

patchfix threaded grep for machines with only one cpu

Subject: [PATCH] fix threaded grep for machines with only one cpu

## tl;dr

26 messages between Feb 15, 2010 and Feb 18, 2010. Diffs are folded; open one to read it.

replies: 25people: 7as markdown or json

Heiko Voigt· Feb 15, 2010, 22:50 UTC · lore

In case the machine has only one cpu the initialization was skipped.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin-grep.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Show changes to builtin-grep.c +6 −6
diff --git a/builtin-grep.c b/builtin-grep.c
index 26d4deb..644051c 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -220,12 +220,6 @@ static void start_threads(struct grep_opt *opt)
 {
 	int i;
 
-	pthread_mutex_init(&grep_mutex, NULL);
-	pthread_mutex_init(&read_sha1_mutex, NULL);
-	pthread_cond_init(&cond_add, NULL);
-	pthread_cond_init(&cond_write, NULL);
-	pthread_cond_init(&cond_result, NULL);
-
 	for (i = 0; i < ARRAY_SIZE(todo); i++) {
 		strbuf_init(&todo[i].out, 0);
 	}
@@ -880,6 +874,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (online_cpus() == 1 || !grep_threads_ok(&opt))
 		use_threads = 0;
 
+	pthread_mutex_init(&grep_mutex, NULL);
+	pthread_mutex_init(&read_sha1_mutex, NULL);
+	pthread_cond_init(&cond_add, NULL);
+	pthread_cond_init(&cond_write, NULL);
+	pthread_cond_init(&cond_result, NULL);
+
 	if (use_threads)
 		start_threads(&opt);
 #else

-- 
1.7.0.rc1.7.gc0da5
Johannes Schindelin· Feb 15, 2010, 23:09 UTC · re: Heiko Voigt · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Hi,
On Mon, 15 Feb 2010, Heiko Voigt wrote:
Show 5 quoted lines
> In case the machine has only one cpu the initialization was
> skipped.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---

Thank you, Heiko, for fixing that nasty bug! It is already in 4msysgit's 'devel' branch.

Ciao, Dscho

Junio C Hamano· Feb 16, 2010, 01:07 UTC · re: Heiko Voigt · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Heiko Voigt <hvoigt@hvoigt.net> writes:
> In case the machine has only one cpu the initialization was
> skipped.

An obvious question that forces reviewers to go "Huh?" is why this matters.

If use_threads is false, we do not call start_threads(), and I think at least the intent of the threaded grep code in 5b594f4 (Threaded grep, 2010-01-25) is that uninitialized mutexes matter at all in that case.

For example, "grep_mutex" is touched by calling grep_lock() and grep_unlock() macros, or pthread_* functions on it directly. There are five functions that touch this mutex: add_work(), get_work(), work_done(), start_threads(), wait_all().

 - add_work() is called from grep_sha1_async() and grep_file_async().
   They are both called _only_ when use_threads is true.
 - get_work() and work_done() are only called from run() and that is the
   function threads run from start_threads().  They cannot be reached
   unless use_threads is true.
 - wait_all() has three call sites in cmd_grep(); all of them trigger only
   when use_threads is true.

So at least for grep_mutex your patch shouldn't matter. Please explain use of which mutex is broken and how your patch fixes it.

I think the fix also assumes that an initialized mutex that is used but not destroyed is not a programming error, as wait_all() is the ONLY function that destroys these mutexes and it is called ONLY when we are using threads by calling start_threads(). But that assumption surely smells like it is sweeping a bug under a rug rather than fixing it.

Is there some programming rule I am not aware of, like "if you define a mutex or cond, you must initialize it, even if you are not going to use it at all?" If that is the case, I think your patch is necessary, but I somehow find it a bit hard to believe.

Junio C Hamano· Feb 16, 2010, 01:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
>> In case the machine has only one cpu the initialization was
>> skipped.
>
> An obvious question that forces reviewers to go "Huh?" is why this
> matters.
>
> If use_threads is false, we do not call start_threads(), and I think at
> least the intent of the threaded grep code in 5b594f4 (Threaded grep,
> 2010-01-25) is that uninitialized mutexes matter at all in that case.
s/matter/do not &/; obviously.
Junio C Hamano· Feb 16, 2010, 02:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Junio C Hamano <gitster@pobox.com> writes:
> So at least for grep_mutex your patch shouldn't matter.  Please explain
> use of which mutex is broken and how your patch fixes it.

Is this the answer to my question? With your patch, your single-threaded program will end up locking and unlocking this mutex needlessly, and the mutex is left undestroyed after you are done, I think.

 builtin-grep.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin-grep.c +2 −2
diff --git a/builtin-grep.c b/builtin-grep.c
index 26d4deb..5c1545e 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
 
 #define grep_lock() pthread_mutex_lock(&grep_mutex)
 #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
-#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
-#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
+#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
+#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
 
 /* Signalled when a new work_item is added to todo. */
 static pthread_cond_t cond_add;
Junio C Hamano· Feb 16, 2010, 02:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Junio C Hamano <gitster@pobox.com> writes:
> Is this the answer to my question?

IOW, please try this patch. I am planning to queue it to 'maint' as part of 1.7.0.1 if this is the right solution (which I obviously think it is).

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 15 Feb 2010 18:34:28 -0800
Subject: [PATCH] Fix use of mutex in threaded grep

The program can decide at runtime not to use threading even if the support is compiled in. In such a case, mutexes are not necessary and left uninitialized. But the code incorrectly tried to take and release the read_sha1_mutex unconditionally.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-grep.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin-grep.c +2 −2
diff --git a/builtin-grep.c b/builtin-grep.c
index 26d4deb..5c1545e 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
 
 #define grep_lock() pthread_mutex_lock(&grep_mutex)
 #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
-#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
-#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
+#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
+#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
 
 /* Signalled when a new work_item is added to todo. */
 static pthread_cond_t cond_add;

-- 
1.7.0.17.g7e5eb
Fredrik Kuivinen· Feb 16, 2010, 07:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, Feb 16, 2010 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:
Show 37 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Is this the answer to my question?
>
> IOW, please try this patch.  I am planning to queue it to 'maint' as part
> of 1.7.0.1 if this is the right solution (which I obviously think it is).
>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 15 Feb 2010 18:34:28 -0800
> Subject: [PATCH] Fix use of mutex in threaded grep
>
> The program can decide at runtime not to use threading even if the
> support is compiled in.  In such a case, mutexes are not necessary
> and left uninitialized.  But the code incorrectly tried to take and
> release the read_sha1_mutex unconditionally.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin-grep.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin-grep.c b/builtin-grep.c
> index 26d4deb..5c1545e 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
>
>  #define grep_lock() pthread_mutex_lock(&grep_mutex)
>  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
>
>  /* Signalled when a new work_item is added to todo. */
>  static pthread_cond_t cond_add;
This is the correct fix. Thanks.
Acked-by: Fredrik Kuivinen <frekui@gmail.com>
- Fredrik
Johannes Schindelin· Feb 16, 2010, 08:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Hi,
On Mon, 15 Feb 2010, Junio C Hamano wrote:
Show 12 quoted lines
> diff --git a/builtin-grep.c b/builtin-grep.c
> index 26d4deb..5c1545e 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
>  
>  #define grep_lock() pthread_mutex_lock(&grep_mutex)
>  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)

This is inconsistent. Just look at the code above and tell me why it is so different.

Ciao, Dscho

Junio C Hamano· Feb 16, 2010, 23:59 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 15 quoted lines
>> diff --git a/builtin-grep.c b/builtin-grep.c
>> index 26d4deb..5c1545e 100644
>> --- a/builtin-grep.c
>> +++ b/builtin-grep.c
>> @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
>>  
>>  #define grep_lock() pthread_mutex_lock(&grep_mutex)
>>  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
>> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
>> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
>> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
>> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
>
> This is inconsistent. Just look at the code above and tell me why it is so 
> different.

It is because grep_mutex is protected by "use_threads" very high in the callchain and do not need nor want extra if().

But I think this is much cleaner. The patch replaces the one you are replying to (i.e. read_sha1_{lock,unlock}() are unconditional).

-- >8 --
Subject: Fix use of mutex in threaded grep

The program can decide at runtime not to use threading even if the support is compiled in. In such a case, mutexes are not necessary and left uninitialized. But the code incorrectly tried to take and release the read_sha1_mutex unconditionally.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Acked-by: Fredrik Kuivinen <frekui@gmail.com>
---
 builtin-grep.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)
Show changes to builtin-grep.c +16 −9
diff --git a/builtin-grep.c b/builtin-grep.c
index 26d4deb..8cec8b6 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -408,15 +408,25 @@ static int pathspec_matches(const char **paths, const char *name, int max_depth)
 	return 0;
 }
 
+static void *read_sha1_file_locked(const unsigned char *sha1, enum object_type *type, unsigned long *size)
+{
+	void *data;
+
+	if (use_threads) {
+		read_sha1_lock();
+		data = read_sha1_file(sha1, type, size);
+		read_sha1_unlock();
+	} else {
+		data = read_sha1_file(sha1, type, size);
+	}
+	return data;
+}
+
 static void *load_sha1(const unsigned char *sha1, unsigned long *size,
 		       const char *name)
 {
 	enum object_type type;
-	char *data;
-
-	read_sha1_lock();
-	data = read_sha1_file(sha1, &type, size);
-	read_sha1_unlock();
+	void *data = read_sha1_file_locked(sha1, &type, size);
 
 	if (!data)
 		error("'%s': unable to read %s", name, sha1_to_hex(sha1));
@@ -605,10 +615,7 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
 			void *data;
 			unsigned long size;
 
-			read_sha1_lock();
-			data = read_sha1_file(entry.sha1, &type, &size);
-			read_sha1_unlock();
-
+			data = read_sha1_file_locked(entry.sha1, &type, &size);
 			if (!data)
 				die("unable to read tree (%s)",
 				    sha1_to_hex(entry.sha1));
Johannes Schindelin· Feb 17, 2010, 01:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Hi,
On Tue, 16 Feb 2010, Junio C Hamano wrote:
Show 35 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> diff --git a/builtin-grep.c b/builtin-grep.c
> >> index 26d4deb..5c1545e 100644
> >> --- a/builtin-grep.c
> >> +++ b/builtin-grep.c
> >> @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
> >>  
> >>  #define grep_lock() pthread_mutex_lock(&grep_mutex)
> >>  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> >> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> >> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> >> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
> >> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
> >
> > This is inconsistent. Just look at the code above and tell me why it is so 
> > different.
> 
> It is because grep_mutex is protected by "use_threads" very high in the
> callchain and do not need nor want extra if().
> 
> But I think this is much cleaner.  The patch replaces the one you are
> replying to (i.e. read_sha1_{lock,unlock}() are unconditional).
> 
> -- >8 --
> Subject: Fix use of mutex in threaded grep
> 
> The program can decide at runtime not to use threading even if the support
> is compiled in.  In such a case, mutexes are not necessary and left
> uninitialized.  But the code incorrectly tried to take and release the
> read_sha1_mutex unconditionally.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Acked-by: Fredrik Kuivinen <frekui@gmail.com>
> ---
Yes, this one looks much, much nicer.

Ciao, Dscho

Junio C Hamano· Feb 17, 2010, 01:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
>> The program can decide at runtime not to use threading even if the support
>> is compiled in.  In such a case, mutexes are not necessary and left
>> uninitialized.  But the code incorrectly tried to take and release the
>> read_sha1_mutex unconditionally.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Acked-by: Fredrik Kuivinen <frekui@gmail.com>
>> ---
>
> Yes, this one looks much, much nicer.

The structure may be much nicer, but one remaining thing is that I do not think foo_locked() is a good name; IIRC, kernel folks use _locked() suffix when the caller is expected to already hold the lock. So a typical naming convention goes like this:

	foo()
        {
        	lock();
                foo_locked();
                unlock();
        }
but what the patch did was the other way around:
	read_sha1_file_locked()
        {
        	lock();
                read_sha1_file();
                unlock();
	}

which is probably against the convention many readers of our codebase are already familiar with.

We need a better name to unconfuse people, I think.
Nicolas Pitre· Feb 17, 2010, 02:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, 16 Feb 2010, Junio C Hamano wrote:
Show 38 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> The program can decide at runtime not to use threading even if the support
> >> is compiled in.  In such a case, mutexes are not necessary and left
> >> uninitialized.  But the code incorrectly tried to take and release the
> >> read_sha1_mutex unconditionally.
> >> 
> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >> Acked-by: Fredrik Kuivinen <frekui@gmail.com>
> >> ---
> >
> > Yes, this one looks much, much nicer.
> 
> The structure may be much nicer, but one remaining thing is that I do not
> think foo_locked() is a good name; IIRC, kernel folks use _locked() suffix
> when the caller is expected to already hold the lock.  So a typical naming
> convention goes like this:
> 
> 	foo()
>         {
>         	lock();
>                 foo_locked();
>                 unlock();
>         }
> 
> but what the patch did was the other way around:
> 
> 	read_sha1_file_locked()
>         {
>         	lock();
>                 read_sha1_file();
>                 unlock();
> 	}
> 
> which is probably against the convention many readers of our codebase are
> already familiar with.
> 
> We need a better name to unconfuse people, I think.
lock_and_read_sha1_file()
Nicolas
Heiko Voigt· Feb 17, 2010, 22:43 UTC · re: Junio C Hamano · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, Feb 16, 2010 at 03:59:43PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> -- >8 --
> Subject: Fix use of mutex in threaded grep
> 
> The program can decide at runtime not to use threading even if the support
> is compiled in.  In such a case, mutexes are not necessary and left
> uninitialized.  But the code incorrectly tried to take and release the
> read_sha1_mutex unconditionally.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Acked-by: Fredrik Kuivinen <frekui@gmail.com>

Just to be sure I just tested this one as well. If you like you can add a:

Tested-by: Heiko Voigt <hvoigt@hvoigt.net>
cheers Heiko
Johannes Schindelin· Feb 18, 2010, 09:10 UTC · re: Heiko Voigt · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

Hi,
On Wed, 17 Feb 2010, Heiko Voigt wrote:
Show 16 quoted lines
> On Tue, Feb 16, 2010 at 03:59:43PM -0800, Junio C Hamano wrote:
> > -- >8 --
> > Subject: Fix use of mutex in threaded grep
> > 
> > The program can decide at runtime not to use threading even if the support
> > is compiled in.  In such a case, mutexes are not necessary and left
> > uninitialized.  But the code incorrectly tried to take and release the
> > read_sha1_mutex unconditionally.
> > 
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > Acked-by: Fredrik Kuivinen <frekui@gmail.com>
> 
> Just to be sure I just tested this one as well. If you like you can add 
> a:
> 
> Tested-by: Heiko Voigt <hvoigt@hvoigt.net>

I have pushed the commit (at least the second-last :-) to 4msysgit's devel already (in the vain hope to release a Git for Windows on Tuesday...)

Ciao, Dscho

Heiko Voigt· Feb 16, 2010, 18:02 UTC · re: Junio C Hamano · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

On Mon, Feb 15, 2010 at 06:39:48PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Is this the answer to my question?

Sorry I forgot to add a reference to the thread from which the patch originated: http://groups.google.com/group/msysgit/msg/5da53cf1ccf417cf

> IOW, please try this patch.  I am planning to queue it to 'maint' as part
> of 1.7.0.1 if this is the right solution (which I obviously think it is).

Yes your patch does it correctly I just verified that the segfaults are gone as well. I think your solution is even nicer than mine. Thanks.

> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 15 Feb 2010 18:34:28 -0800
> Subject: [PATCH] Fix use of mutex in threaded grep
[...]
Show 12 quoted lines
> diff --git a/builtin-grep.c b/builtin-grep.c
> index 26d4deb..5c1545e 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
>  
>  #define grep_lock() pthread_mutex_lock(&grep_mutex)
>  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)

One minor thing: Would it not be even nicer having the while loop inside the if clause? E.g like this

#define read_sha1_lock() if (use_threads) do { pthread_mutex_lock(&read_sha1_mutex); } while (0) #define read_sha1_unlock() if (use_threads) do { pthread_mutex_unlock(&read_sha1_mutex); } while (0)

If the purpose was to force a thread switch it is not necessary when not using threads.

cheers Heiko
Nicolas Pitre· Feb 16, 2010, 18:59 UTC · re: Heiko Voigt · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, 16 Feb 2010, Heiko Voigt wrote:
Show 37 quoted lines
> On Mon, Feb 15, 2010 at 06:39:48PM -0800, Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > > Is this the answer to my question?
> 
> Sorry I forgot to add a reference to the thread from which the patch
> originated: http://groups.google.com/group/msysgit/msg/5da53cf1ccf417cf
> 
> > IOW, please try this patch.  I am planning to queue it to 'maint' as part
> > of 1.7.0.1 if this is the right solution (which I obviously think it is).
> 
> Yes your patch does it correctly I just verified that the segfaults are
> gone as well. I think your solution is even nicer than mine. Thanks.
> 
> > -- >8 --
> > From: Junio C Hamano <gitster@pobox.com>
> > Date: Mon, 15 Feb 2010 18:34:28 -0800
> > Subject: [PATCH] Fix use of mutex in threaded grep
> [...]
> > diff --git a/builtin-grep.c b/builtin-grep.c
> > index 26d4deb..5c1545e 100644
> > --- a/builtin-grep.c
> > +++ b/builtin-grep.c
> > @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
> >  
> >  #define grep_lock() pthread_mutex_lock(&grep_mutex)
> >  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> > -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> > -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> > +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
> > +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
> 
> One minor thing: Would it not be even nicer having the while loop inside
> the if clause? E.g like this
> 
> #define read_sha1_lock() if (use_threads) do { pthread_mutex_lock(&read_sha1_mutex); } while (0)
> #define read_sha1_unlock() if (use_threads) do { pthread_mutex_unlock(&read_sha1_mutex); } while (0)
No.  Think what happens if you have code like this:
	if (foo == 1)
		read_sha1_lock();
	else
		baz();
Nicolas
Junio C Hamano· Feb 16, 2010, 19:20 UTC · re: Heiko Voigt · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Heiko Voigt <hvoigt@hvoigt.net> writes:
Show 5 quoted lines
>> IOW, please try this patch.  I am planning to queue it to 'maint' as part
>> of 1.7.0.1 if this is the right solution (which I obviously think it is).
>
> Yes your patch does it correctly I just verified that the segfaults are
> gone as well. I think your solution is even nicer than mine. Thanks.
Show 8 quoted lines
>> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
>> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
>
> One minor thing: Would it not be even nicer having the while loop inside
> the if clause? E.g like this
>
> #define read_sha1_lock() if (use_threads) do { pthread_mutex_lock(&read_sha1_mutex); } while (0)
> #define read_sha1_unlock() if (use_threads) do { pthread_mutex_unlock(&read_sha1_mutex); } while (0)
No.
	#define frotz() do { this compound stmt; } while (0)

is a common idiom to make a macro that expands to a compound stmt behave as if it is a simple function call to avoid bugs when it is expanded, regardless of in which context it is used by an unsuspecting caller.

Your rewrite is pointless because it is the same as saying
	#define read_sha1_lock() if (use_threads) p_m_l(&r_s_m)

and that is exactly what the idiom's use of "do { } while (0)" is all about.

Try this simple program.

-- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 -- #include <stdio.h>

#define frotz() do { if (flag) printf("frotz"); } while (0) #define xyzzy() if (flag) do { printf("xyzzy"); } while (0) #define yomin() if (flag) printf("yomin")

void test(int foo, int bar, int baz, int flag)
{
        if (foo)
                frotz();
        else if (bar)
                xyzzy();
        else if (baz)
                yomin();
        else
                printf("huh?");
        printf("\n");
}
int main(int ac, char **av)
{
        int foo, bar, baz;
        for (foo = 0; foo < 2; foo++)
                for (bar = 0; bar < 2; bar++)
                        for (baz = 0; baz < 2; baz++) {
                                printf("%d %d %d ", foo, bar, baz);
                                test(foo, bar, baz, 1);
                        }
        return 0;
}
-- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 --
Textually the "test" function expands to this:
        void test(int foo, int bar, int baz, int flag)
        {
         if (foo)
          do { if (flag) printf("frotz"); } while (0);
         else if (bar)
          if (flag) do { printf("xyzzy"); } while (0);
         else if (baz)
          if (flag) printf("yomin");
         else
          printf("huh?");
         printf("\n");
        }
but if you properly indent it, it looks like this:
        void test(int foo, int bar, int baz, int flag)
        {
                if (foo)
                        do {
                                if (flag)
                                        printf("frotz");
                        } while (0);
                else if (bar)
                        if (flag)
                                do {
                                        printf("xyzzy");
                                } while (0);
                        else if (baz)
                                if (flag)
                                        printf("yomin");
                                else
                                        printf("huh?");
                printf("\n");
        }
Notice how your version (xyzzy) broke the cascade of if..elseif..else.
Don't they teach this in schools anymore?
Sverre Rabbelier· Feb 16, 2010, 19:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Heya,
On Tue, Feb 16, 2010 at 20:20, Junio C Hamano <gitster@pobox.com> wrote:
> Don't they teach this in schools anymore?

Have they ever? And no, they don't. At least in the Netherlands (and I suspect it to be the same in other European countries, and perhaps even in the US) don't spend a whole lot of time teaching C. At my uni they start out with Java, and stick with that most of the time. There are only brief (and often optional) ventures into other languages.

-- 
Cheers,

Sverre Rabbelier
Nicolas Pitre· Feb 16, 2010, 20:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, 16 Feb 2010, Junio C Hamano wrote:
[...]
> Notice how your version (xyzzy) broke the cascade of if..elseif..else.
> 
> Don't they teach this in schools anymore?

What do you expect from academia? School and real life are still too often disconnected.

Nicolas
Johannes Schindelin· Feb 16, 2010, 20:19 UTC · re: Nicolas Pitre · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Hi,
On Tue, 16 Feb 2010, Nicolas Pitre wrote:
Show 9 quoted lines
> On Tue, 16 Feb 2010, Junio C Hamano wrote:
> 
> [...]
> > Notice how your version (xyzzy) broke the cascade of if..elseif..else.
> > 
> > Don't they teach this in schools anymore?
> 
> What do you expect from academia?  School and real life are still too 
> often disconnected.

Actually, you know, I am quite happy that they do not teach _that_ particular code template. It is enough that I have to suffer this ugliness from oldtimers, no need for newtimers piling onto this particular pile.

Ciao, Dscho

Nicolas Pitre· Feb 16, 2010, 20:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, 16 Feb 2010, Johannes Schindelin wrote:
Show 17 quoted lines
> Hi,
> 
> On Tue, 16 Feb 2010, Nicolas Pitre wrote:
> 
> > On Tue, 16 Feb 2010, Junio C Hamano wrote:
> > 
> > [...]
> > > Notice how your version (xyzzy) broke the cascade of if..elseif..else.
> > > 
> > > Don't they teach this in schools anymore?
> > 
> > What do you expect from academia?  School and real life are still too 
> > often disconnected.
> 
> Actually, you know, I am quite happy that they do not teach _that_ 
> particular code template. It is enough that I have to suffer this ugliness 
> from oldtimers, no need for newtimers piling onto this particular pile.

Unfortunately the real world is not without its share of ugliness. And newtimers need to be better prepared to cope when they get loose.

I guess that's one of the reasons I skipped many lectures at Uni... to hack on Linux 1.0.x instead.

Nicolas
Heiko Voigt· Feb 16, 2010, 20:57 UTC · re: Nicolas Pitre · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, Feb 16, 2010 at 03:00:47PM -0500, Nicolas Pitre wrote:
Show 9 quoted lines
> On Tue, 16 Feb 2010, Junio C Hamano wrote:
> 
> [...]
> > Notice how your version (xyzzy) broke the cascade of if..elseif..else.
> > 
> > Don't they teach this in schools anymore?
> 
> What do you expect from academia?  School and real life are still too 
> often disconnected.

If you want to offend people that try to help out in real life. This is exactly the kind of comment that does it.

Nicolas Pitre· Feb 16, 2010, 21:20 UTC · re: Heiko Voigt · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, 16 Feb 2010, Heiko Voigt wrote:
Show 6 quoted lines
> On Tue, Feb 16, 2010 at 03:00:47PM -0500, Nicolas Pitre wrote:
> > What do you expect from academia?  School and real life are still too 
> > often disconnected.
> 
> If you want to offend people that try to help out in real life. This is
> exactly the kind of comment that does it.

Because you think that I'm not giving my own time helping people solving real life issues? If so I'd suggest you do a quick background check on myself.

I do get offence, though, when theoricians try to tell me how to do the things they never were able to do themselves because they get so blinded by concepts over practicalities. Dont get me wrong -- there are a good bunch of individuals with practical sense in academia, but they usually aren't those who get most credits.

Nicolas
Junio C Hamano· Feb 16, 2010, 22:54 UTC · re: Nicolas Pitre · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

Nicolas Pitre <nico@fluxnic.net> writes:
Show 12 quoted lines
> On Tue, 16 Feb 2010, Heiko Voigt wrote:
>
>> On Tue, Feb 16, 2010 at 03:00:47PM -0500, Nicolas Pitre wrote:
>> > What do you expect from academia?  School and real life are still too 
>> > often disconnected.
>> 
>> If you want to offend people that try to help out in real life. This is
>> exactly the kind of comment that does it.
>
> Because you think that I'm not giving my own time helping people solving 
> real life issues? If so I'd suggest you do a quick background check on 
> myself.

I don't think Heiko is saying he was offended by you, but is saying he was offended by me saying "Don't they teach this in schools anymore?" I was simply curious about the answer to that question. If they don't teach C, it is not Heiko's fault---no need to get offended. I admit that I did go "Huh? Never repeat? What's the point?" when I first saw the idiom.

By learning and sticking to idioms people get more efficient, because there is no need to think if it is better to have "do { ... } while()" inside or outside yourself; cleverer people have already figured out the best way and that is how idioms came about. Once you learn them, your eyes will coast over the outer cruft and focus on the essential part of the macro "..." without even spending any extra effort to actively squelch the surrounding "do {" and "} while (0)". These literally become part of background noise and do not bother you anymore to the point that they are not even ugly.

Think of this as a mutual/collective learning experience. As Dscho said on this topic on msysgit list, Heiko _is_ the hero in figuring out what was originally broken, even though the proposed solution might have been suboptimal. I know figuring out what is broken have taken a lot more effort than my looking over his solution and reaching a better solution. Correct diagnosis is often more than 80% of solving a problem.

Heiko Voigt· Feb 17, 2010, 17:56 UTC · re: Junio C Hamano · lore

Re: Re: [PATCH] fix threaded grep for machines with only one cpu

On Tue, Feb 16, 2010 at 02:54:29PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > On Tue, 16 Feb 2010, Heiko Voigt wrote:
> >
> >> On Tue, Feb 16, 2010 at 03:00:47PM -0500, Nicolas Pitre wrote:
> >> > What do you expect from academia?  School and real life are still too 
> >> > often disconnected.
> >> 
> >> If you want to offend people that try to help out in real life. This is
> >> exactly the kind of comment that does it.
> >
> > Because you think that I'm not giving my own time helping people solving 
> > real life issues? If so I'd suggest you do a quick background check on 
> > myself.
> 
> I don't think Heiko is saying he was offended by you, but is saying he was
> offended by me saying "Don't they teach this in schools anymore?"  I was
> simply curious about the answer to that question.  If they don't teach C,
> it is not Heiko's fault---no need to get offended.  I admit that I did go
> "Huh?  Never repeat?  What's the point?" when I first saw the idiom.

It was actually the combination of both ;) But I am glad we could sort this out. I simply had the same thought as you when I saw this first and then confused it with the typical sleep(0) you use for forcing thread switches.

BTW, we had a course of C but for macros they stopped at "type them uppercase and try to avoid them when possible". Everything else is learning by experience.

cheers Heiko
Paolo Bonzini· Feb 17, 2010, 15:29 UTC · re: Nicolas Pitre · lore

Re: [PATCH] fix threaded grep for machines with only one cpu

On 02/16/2010 09:00 PM, Nicolas Pitre wrote:
Show 9 quoted lines
> On Tue, 16 Feb 2010, Junio C Hamano wrote:
>
> [...]
>> Notice how your version (xyzzy) broke the cascade of if..elseif..else.
>>
>> Don't they teach this in schools anymore?
>
> What do you expect from academia?  School and real life are still too
> often disconnected.

When I taught C to second-year bachelor students, I think I did a pretty good course (*) but it totally lacked time to get into macros, except for simple constants. I did have a student later on that was doing his final project with me and came asking what it was.

Another guy I know is teaching an elective "portable programming" course that includes pretty much everything you'd expect (including bit-twiddling tricks, basic Autoconf, shared libraries, blah blah) but that's a graduate-level course.

    (*) and not too disconnected from reality.  One year their final
    one-week project was using cairo for graphics, had a server that
    talked to multiple clients using poll, and I forced them to support
    IPv6.  Shameless plug: http://github.com/bonzini/netrobots
Paolo

← back to recent threads