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

Junio C Hamano [off-list ref] 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 <redacted>
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 <redacted>
---
 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

On Tue, Feb 16, 2010 at 03:39, Junio C Hamano [off-list ref] wrote:

This is the correct fix. Thanks.

Acked-by: Fredrik Kuivinen <redacted>

- Fredrik

Hi,

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

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

Ciao,
Dscho

On Mon, Feb 15, 2010 at 06:39:48PM -0800, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

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 <redacted>
Date: Mon, 15 Feb 2010 18:34:28 -0800
Subject: [PATCH] Fix use of mutex in threaded grep

[...]

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

On Tue, 16 Feb 2010, Heiko Voigt wrote:

On Mon, Feb 15, 2010 at 06:39:48PM -0800, Junio C Hamano wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

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

quoted

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.

quoted

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

[...]

quoted

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

Heiko Voigt [off-list ref] writes:

quoted

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.

quoted

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

Heya,

On Tue, Feb 16, 2010 at 20:20, Junio C Hamano [off-list ref] 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

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

Hi,

On Tue, 16 Feb 2010, Nicolas Pitre wrote:

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

[...]

quoted

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

On Tue, 16 Feb 2010, Johannes Schindelin wrote:

Hi,

On Tue, 16 Feb 2010, Nicolas Pitre wrote:

quoted

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

[...]

quoted

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

On Tue, Feb 16, 2010 at 03:00:47PM -0500, Nicolas Pitre wrote:

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

[...]

quoted

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.

On Tue, 16 Feb 2010, Heiko Voigt wrote:

On Tue, Feb 16, 2010 at 03:00:47PM -0500, Nicolas Pitre wrote:

quoted

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

Johannes Schindelin [off-list ref] writes:

quoted

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 <redacted>
Acked-by: Fredrik Kuivinen <redacted>
---

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

Hi,

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

Johannes Schindelin [off-list ref] writes:

quoted

quoted

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 <redacted>
Acked-by: Fredrik Kuivinen <redacted>
---

Yes, this one looks much, much nicer.

Ciao,
Dscho

On 02/16/2010 09:00 PM, Nicolas Pitre wrote:

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

[...]

quoted

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

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 <redacted>
Acked-by: Fredrik Kuivinen <redacted>

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

Tested-by: Heiko Voigt <redacted>

cheers Heiko

Hi,

On Wed, 17 Feb 2010, Heiko Voigt wrote:

On Tue, Feb 16, 2010 at 03:59:43PM -0800, Junio C Hamano wrote:

quoted

-- >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 <redacted>
Acked-by: Fredrik Kuivinen <redacted>

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

Tested-by: Heiko Voigt <redacted>

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