[RFC][PATCH] grep: enable threading for context line printing

This patch makes work_done() in builtin/grep.c print hunk marks between
files if threading is used, while show_line() in grep.c still does the
same in the other case.  The latter is  controlled by the struct grep_opt
member show_hunk_mark: 0 means show_line() prints no hunk marks between
files, 1 means it prints them before the next file with matches and 2
means it prints them before matches from the current file.

Having two places for this is a bit ugly but it enables parallel grep
for the common -ABC options.  Locking should be fine in add_work().

Comments?

René

---
 builtin/grep.c |   17 ++++++++++++++++-
 grep.c         |   14 +++-----------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 40b9a93..8d58bb9 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -96,6 +96,9 @@ static pthread_cond_t cond_write;
 /* Signalled when we are finished with everything. */
 static pthread_cond_t cond_result;
 
+static int print_hunk_marks_between_files;
+static int printed_something;
+
 static void add_work(enum work_type type, char *name, void *id)
 {
 	grep_lock();

@@ -159,7 +162,12 @@ static void work_done(struct work_item *w)
 	for(; todo[todo_done].done && todo_done != todo_start;
 	    todo_done = (todo_done+1) % ARRAY_SIZE(todo)) {
 		w = &todo[todo_done];
-		write_or_die(1, w->out.buf, w->out.len);
+		if (w->out.len) {
+			if (print_hunk_marks_between_files && printed_something)
+				write_or_die(1, "--\n", 3);
+			write_or_die(1, w->out.buf, w->out.len);
+			printed_something = 1;
+		}
 		free(w->name);
 		free(w->identifier);
 	}

@@ -932,6 +940,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	use_threads = 0;
 #endif
 
+	if (opt.pre_context || opt.post_context) {
+		if (use_threads)
+			print_hunk_marks_between_files = 1;
+		else
+			opt.show_hunk_mark = 1;
+	}
+
 	compile_grep_patterns(&opt);
 
 	/* Check revs and then paths */

diff --git a/grep.c b/grep.c
index 90a063a..251c70c 100644
--- a/grep.c
+++ b/grep.c

@@ -549,10 +549,10 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	sign_str[0] = sign;
 	if (opt->pre_context || opt->post_context) {
 		if (opt->last_shown == 0) {
-			if (opt->show_hunk_mark)
+			if (opt->show_hunk_mark == 2)
 				opt->output(opt, "--\n", 3);
-			else
-				opt->show_hunk_mark = 1;
+			else if (opt->show_hunk_mark == 1)
+				opt->show_hunk_mark = 2;
 		} else if (lno > opt->last_shown + 1)
 			opt->output(opt, "--\n", 3);
 	}

@@ -750,14 +750,6 @@ int grep_threads_ok(const struct grep_opt *opt)
 	    !opt->name_only)
 		return 0;
 
-	/* If we are showing hunk marks, we should not do it for the
-	 * first match. The synchronization problem we get for this
-	 * constraint is not yet solved, so we disable threading in
-	 * this case.
-	 */
-	if (opt->pre_context || opt->post_context)
-		return 0;
-
 	return 1;
 }
 

On Sun, Mar 14, 2010 at 19:18, René Scharfe [off-list ref] wrote:

This patch makes work_done() in builtin/grep.c print hunk marks between
files if threading is used, while show_line() in grep.c still does the
same in the other case.  The latter is  controlled by the struct grep_opt
member show_hunk_mark: 0 means show_line() prints no hunk marks between
files, 1 means it prints them before the next file with matches and 2
means it prints them before matches from the current file.

Having two places for this is a bit ugly but it enables parallel grep
for the common -ABC options.  Locking should be fine in add_work().

Comments?

The implementation looks correct. As you say it is a bit ugly, but I
think it is worth it anyway. (The solutions I managed to come up with
when I wrote the original threaded grep patch were even uglier, that
is why I simply disabled the threading in that case.)

Symbolic constants for the magic values 0, 1, and 2 would make the
code more readable.

- Fredrik

Am 14.03.2010 21:03, schrieb Fredrik Kuivinen:

On Sun, Mar 14, 2010 at 19:18, René Scharfe [off-list ref] wrote:

quoted

This patch makes work_done() in builtin/grep.c print hunk marks between
files if threading is used, while show_line() in grep.c still does the
same in the other case.  The latter is  controlled by the struct grep_opt
member show_hunk_mark: 0 means show_line() prints no hunk marks between
files, 1 means it prints them before the next file with matches and 2
means it prints them before matches from the current file.

Having two places for this is a bit ugly but it enables parallel grep
for the common -ABC options.  Locking should be fine in add_work().

Comments?

The implementation looks correct. As you say it is a bit ugly, but I
think it is worth it anyway. (The solutions I managed to come up with
when I wrote the original threaded grep patch were even uglier, that
is why I simply disabled the threading in that case.)

Symbolic constants for the magic values 0, 1, and 2 would make the
code more readable.

Yes, that was a bit too complicated.  I shuffled the code around a bit,
so the patch is now a bit smaller and avoids introducing value 2 for
show_hunk_mark.  Better?

-- >8 --
If context lines are to be printed, grep separates them with hunk marks
("--\n").  These marks are printed between matches from different files,
too.  They are not printed before the first file, though.

Threading was disabled when context line printing was enabled because
avoiding to print the mark before the first line was an unsolved
synchronisation problem.  This patch separates the code for printing
hunk marks for the threaded and the unthreaded case, allowing threading
to be turned on together with the common -ABC options.

->show_hunk_mark, which controls printing of hunk marks between files in
show_line(), is now set in grep_buffer_1(), but only if some results
have already been printed and threading is disabled.  The threaded case
is handled in work_done().

Signed-off-by: Rene Scharfe <redacted>
---
 builtin/grep.c |   15 +++++++++++++--
 grep.c         |   17 +++++------------
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 40b9a93..f427d55 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -96,6 +96,9 @@ static pthread_cond_t cond_write;
 /* Signalled when we are finished with everything. */
 static pthread_cond_t cond_result;
 
+static int print_hunk_marks_between_files;
+static int printed_something;
+
 static void add_work(enum work_type type, char *name, void *id)
 {
 	grep_lock();

@@ -159,7 +162,12 @@ static void work_done(struct work_item *w)
 	for(; todo[todo_done].done && todo_done != todo_start;
 	    todo_done = (todo_done+1) % ARRAY_SIZE(todo)) {
 		w = &todo[todo_done];
-		write_or_die(1, w->out.buf, w->out.len);
+		if (w->out.len) {
+			if (print_hunk_marks_between_files && printed_something)
+				write_or_die(1, "--\n", 3);
+			write_or_die(1, w->out.buf, w->out.len);
+			printed_something = 1;
+		}
 		free(w->name);
 		free(w->identifier);
 	}

@@ -926,8 +934,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (online_cpus() == 1 || !grep_threads_ok(&opt))
 		use_threads = 0;
 
-	if (use_threads)
+	if (use_threads) {
+		if (opt.pre_context || opt.post_context)
+			print_hunk_marks_between_files = 1;
 		start_threads(&opt);
+	}
 #else
 	use_threads = 0;
 #endif

diff --git a/grep.c b/grep.c
index 90a063a..e5f06e4 100644
--- a/grep.c
+++ b/grep.c

@@ -551,8 +551,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		if (opt->last_shown == 0) {
 			if (opt->show_hunk_mark)
 				opt->output(opt, "--\n", 3);
-			else
-				opt->show_hunk_mark = 1;
 		} else if (lno > opt->last_shown + 1)
 			opt->output(opt, "--\n", 3);
 	}

@@ -750,14 +748,6 @@ int grep_threads_ok(const struct grep_opt *opt)
 	    !opt->name_only)
 		return 0;
 
-	/* If we are showing hunk marks, we should not do it for the
-	 * first match. The synchronization problem we get for this
-	 * constraint is not yet solved, so we disable threading in
-	 * this case.
-	 */
-	if (opt->pre_context || opt->post_context)
-		return 0;
-
 	return 1;
 }
 

@@ -779,11 +769,14 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	enum grep_context ctx = GREP_CONTEXT_HEAD;
 	xdemitconf_t xecfg;
 
-	opt->last_shown = 0;
-
 	if (!opt->output)
 		opt->output = std_output;
 
+	if (opt->last_shown && (opt->pre_context || opt->post_context) &&
+	    opt->output == std_output)
+		opt->show_hunk_mark = 1;
+	opt->last_shown = 0;
+
 	if (buffer_is_binary(buf, size)) {
 		switch (opt->binary) {
 		case GREP_BINARY_DEFAULT:

René Scharfe [off-list ref] writes:

quoted

Symbolic constants for the magic values 0, 1, and 2 would make the
code more readable.

Yes, that was a bit too complicated.  I shuffled the code around a bit,
so the patch is now a bit smaller and avoids introducing value 2 for
show_hunk_mark.  Better?

I guess.

I still cannot shake off this nagging feeling that the complication mainly
comes from the way threaded support was written as if the control flow is
the same as that of the single-threaded implementation (e.g. grep_object()
and friends are called from a one-path-at-a-time loop and pretend as if
they have returned some meaningful results indicating they have found a
match), and the logic may become more clear if the flow was restructured.

But I haven't tried, so I dunno.  Your patch does not make the situation
worse, and looks like the right thing to do at the moment.

On Mon, Mar 15, 2010 at 17:21, René Scharfe [off-list ref] wrote:

Yes, that was a bit too complicated.  I shuffled the code around a bit,
so the patch is now a bit smaller and avoids introducing value 2 for
show_hunk_mark.  Better?

In my opinion, yes.

- Fredrik