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

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

[Heiko Voigt]

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

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

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

[Johannes Schindelin]

Hi,

On Mon, 15 Feb 2010, Heiko Voigt wrote:

> 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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

[Junio C Hamano]

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

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;

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

[Junio C Hamano]

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;
-- 
1.7.0.17.g7e5eb

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

[Fredrik Kuivinen]

On Tue, Feb 16, 2010 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Johannes Schindelin]

Hi,

On Mon, 15 Feb 2010, Junio C Hamano wrote:

> 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

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

[Heiko Voigt]

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)

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

cheers Heiko

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

[Nicolas Pitre]

On Tue, 16 Feb 2010, Heiko Voigt wrote:

> 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

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

>> 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.

>> +#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?

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

[Sverre Rabbelier]

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

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

[Nicolas Pitre]

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

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Nicolas Pitre]

On Tue, 16 Feb 2010, Johannes Schindelin wrote:

> 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

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

[Heiko Voigt]

On Tue, Feb 16, 2010 at 03:00:47PM -0500, 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.

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

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

[Nicolas Pitre]

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

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

[Junio C Hamano]

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.

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.

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

[Junio C Hamano]

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

 builtin-grep.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

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

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

[Johannes Schindelin]

Hi,

On Tue, 16 Feb 2010, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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.

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

[Nicolas Pitre]

On Tue, 16 Feb 2010, Junio C Hamano wrote:

> 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

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

[Paolo Bonzini]

On 02/16/2010 09:00 PM, 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.

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

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

[Heiko Voigt]

On Tue, Feb 16, 2010 at 02:54:29PM -0800, Junio C Hamano wrote:
> 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

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

[Heiko Voigt]

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>

cheers Heiko

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

[Johannes Schindelin]

Hi,

On Wed, 17 Feb 2010, Heiko Voigt wrote:

> 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