threads / patch / 45715

v2, 8 partsgrep threading cleanup & tests

Subject: [PATCH v2 0/8] grep threading cleanup & tests

## tl;dr

9 messages between Apr 16, 2017 and Apr 16, 2017. Diffs are folded; open one to read it.

replies: 8people: 1as markdown or json

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:20 UTC · lore

This is the spiritual successor to the "grep: add ability to disable threading with --threads=0 or grep.threads=0" patch I submitted as part of my PCRE series (<20170408132506.5415-2-avarab@gmail.com>).

There's a long back & forth thread between me and Jeff King as a follow-up to that which I'll summarize: I'd misunderstood what --threads=1 does, because I was tracing how often a pattern was compiled, and observing that even with --threads=1 we'd compile the pattern twice.

This series fixes that redundant needless compilation in 3/8, skips pthread overhead as Jeff suggested kin 3/8. The rest of the patches are a bunch of small related fixes I noticed along the way.

Ævar Arnfjörð Bjarmason (8):
  grep: assert that threading is enabled when calling grep_{lock,unlock}
  grep: add tests for --threads=N and grep.threads
  grep: don't redundantly compile throwaway patterns under threading
  grep: skip pthreads overhead when using one thread
  tests: add a PTHREADS prerequisite
  pack-object & index-pack: add test for --threads warning under
    NO_PTHREADS
  pack-objects: fix buggy warning about threads under
    NO_PTHREADS=YesPlease
  grep: given --threads with NO_PTHREADS=YesPlease, warn

 Makefile               |  1 +
 builtin/grep.c         | 32 +++++++++++++++++++++++++-------
 builtin/pack-objects.c |  4 +++-
 t/README               |  4 ++++
 t/t5300-pack-object.sh | 33 +++++++++++++++++++++++++++++++++
 t/t7810-grep.sh        | 34 ++++++++++++++++++++++++++++++++++
 t/test-lib.sh          |  1 +
 7 files changed, 101 insertions(+), 8 deletions(-)

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 1/8] grep: assert that threading is enabled when calling grep_{lock,unlock}

Change the grep_{lock,unlock} functions to assert that num_threads is true, instead of only locking & unlocking the pthread mutex lock when it is.

These functions are never called when num_threads isn't true, this logic has gone through multiple iterations since the initial introduction of grep threading in commit 5b594f457a ("Threaded grep", 2010-01-25), but ever since then they'd only be called if num_threads was true, so this check made the code confusing to read.

Replace the check with an assertion, so that it's clear to the reader that this code path is never taken unless we're spawning threads.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to builtin/grep.c +4 −4

diff --git a/builtin/grep.c b/builtin/grep.c
index 65070c52fc..3aa7836a04 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -73,14 +73,14 @@ static pthread_mutex_t grep_mutex;
 
 static inline void grep_lock(void)
 {
-	if (num_threads)
-		pthread_mutex_lock(&grep_mutex);
+	assert(num_threads);
+	pthread_mutex_lock(&grep_mutex);
 }
 
 static inline void grep_unlock(void)
 {
-	if (num_threads)
-		pthread_mutex_unlock(&grep_mutex);
+	assert(num_threads);
+	pthread_mutex_unlock(&grep_mutex);
 }
 
 /* Signalled when a new work_item is added to todo. */

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/8] grep: add tests for --threads=N and grep.threads

Add tests for when --threads=N is supplied on the command-line or when grep.threads is supplied in the configuration.

When the threading support was made run-time configurable in commit 89f09dd34e ("grep: add --threads=<num> option and grep.threads configuration", 2015-12-15) no tests were added for it.

In developing a change to grep I was able to make '--threads=1 <pat>` segfault, while the test suite still passed. This change fixes that blind spot in the tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7810-grep.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Show changes to t/t7810-grep.sh +16 −0

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index cee42097b0..4523ca926b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -771,6 +771,22 @@ test_expect_success 'grep -W with userdiff' '
 	test_cmp expected actual
 '
 
+for threads in $(test_seq 0 10)
+do
+	test_expect_success "grep --threads=$threads & -c grep.threads=$threads" "
+		git grep --threads=$threads . >actual.$threads &&
+		if test $threads -ge 1
+		then
+			test_cmp actual.\$(($threads - 1)) actual.$threads
+		fi &&
+		git -c grep.threads=$threads grep . >actual.$threads &&
+		if test $threads -ge 1
+		then
+			test_cmp actual.\$(($threads - 1)) actual.$threads
+		fi
+	"
+done
+
 test_expect_success 'grep from a subdirectory to search wider area (1)' '
 	mkdir -p s &&
 	(

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 3/8] grep: don't redundantly compile throwaway patterns under threading

Change the pattern compilation logic under threading so that grep doesn't compile a pattern it never ends up using on the non-threaded code path, only to compile it again N times for N threads which will each use their own copy, ignoring the initially compiled pattern.

This redundant compilation dates back to the initial introduction of the threaded grep in commit 5b594f457a ("Threaded grep", 2010-01-25).

There was never any reason for doing this redundant work other than an oversight in the initial commit. Jeff King suggested on-list in <20170414212325.fefrl3qdjigwyitd@sigill.intra.peff.net> that this might be needed to check the pattern for sanity before threaded execution commences.

That's not the case. The pattern is compiled under threading in start_threads() before any concurrent execution has started by calling pthread_create(), so if the pattern contains an error we still do the right thing. I.e. die with one error before any threaded execution has commenced, instead of e.g. spewing out an error for each N threads, which could be a regression a change like this might inadvertently introduce.

The undocumented --debug mode added in commit 17bf35a3c7 ("grep: teach --debug option to dump the parse tree", 2012-09-13) still works properly with this change. It only emits debugging info during pattern compilation, which is now dumped by the pattern compiled just before the first thread is started.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Show changes to builtin/grep.c +11 −3

diff --git a/builtin/grep.c b/builtin/grep.c
index 3aa7836a04..a3d380551b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -224,7 +224,8 @@ static void start_threads(struct grep_opt *opt)
 		int err;
 		struct grep_opt *o = grep_opt_dup(opt);
 		o->output = strbuf_out;
-		o->debug = 0;
+		if (i)
+			o->debug = 0;
 		compile_grep_patterns(o);
 		err = pthread_create(&threads[i], NULL, run, o);
 
@@ -1154,8 +1155,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!opt.fixed && opt.ignore_case)
 		opt.regflags |= REG_ICASE;
 
-	compile_grep_patterns(&opt);
-
 	/*
 	 * We have to find "--" in a separate pass, because its presence
 	 * influences how we will parse arguments that come before it.
@@ -1230,6 +1229,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	num_threads = 0;
 #endif
 
+	if (!num_threads)
+		/*
+		 * The compiled patterns on the main path are only
+		 * used when not using threading. Otherwise
+		 * start_threads() below calls compile_grep_patterns()
+		 * for each thread.
+		 */
+		compile_grep_patterns(&opt);
+
 #ifndef NO_PTHREADS
 	if (num_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 4/8] grep: skip pthreads overhead when using one thread

Skip the administrative overhead of using pthreads when only using one thread. Instead take the non-threaded path which would be taken under NO_PTHREADS.

The threading support was initially added in commit 5b594f457a ("Threaded grep", 2010-01-25) with a hardcoded compile-time number of 8 threads. Later the number of threads was made configurable in commit 89f09dd34e ("grep: add --threads=<num> option and grep.threads configuration", 2015-12-15).

That change did not add any special handling for --threads=1. Now we take a slightly faster path by skipping thread handling entirely when 1 thread is requested.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 2 ++
 1 file changed, 2 insertions(+)

Show changes to builtin/grep.c +2 −0

diff --git a/builtin/grep.c b/builtin/grep.c
index a3d380551b..cb3323060e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1225,6 +1225,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		num_threads = GREP_NUM_THREADS_DEFAULT;
 	else if (num_threads < 0)
 		die(_("invalid number of threads specified (%d)"), num_threads);
+	if (num_threads == 1)
+		num_threads = 0;
 #else
 	num_threads = 0;
 #endif

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 5/8] tests: add a PTHREADS prerequisite

Add a PTHREADS prerequisite which is false when git is compiled with NO_PTHREADS=YesPlease.

There's lots of custom code that runs when threading isn't available, but before this prerequisite there was no way to test it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile      | 1 +
 t/README      | 4 ++++
 t/test-lib.sh | 1 +
 3 files changed, 6 insertions(+)

Show changes to 3 files +6 −0

Makefile, t/README, t/test-lib.sh

diff --git a/Makefile b/Makefile
index 9b36068ac5..9753281acd 100644
--- a/Makefile
+++ b/Makefile
@@ -2235,6 +2235,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_EXPAT=\''$(subst ','\'',$(subst ','\'',$(NO_EXPAT)))'\' >>$@+
 	@echo USE_LIBPCRE=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE)))'\' >>$@+
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
+	@echo NO_PTHREADS=\''$(subst ','\'',$(subst ','\'',$(NO_PTHREADS)))'\' >>$@+
 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
diff --git a/t/README b/t/README
index ab386c3681..6775ced582 100644
--- a/t/README
+++ b/t/README
@@ -817,6 +817,10 @@ use these, and "test_set_prereq" for how to define your own.
    Test is run on a filesystem which converts decomposed utf-8 (nfd)
    to precomposed utf-8 (nfc).
 
+ - PTHREADS
+
+   Git wasn't compiled with NO_PTHREADS=YesPlease.
+
 Tips for Writing Tests
 ----------------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 13b5696822..fba91bad3a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1009,6 +1009,7 @@ esac
 
 ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
 test -z "$NO_PERL" && test_set_prereq PERL
+test -z "$NO_PTHREADS" && test_set_prereq PTHREADS
 test -z "$NO_PYTHON" && test_set_prereq PYTHON
 test -n "$USE_LIBPCRE" && test_set_prereq LIBPCRE
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 8/8] grep: given --threads with NO_PTHREADS=YesPlease, warn

Add a warning about missing thread support when grep.threads or --threads is set to a non 0 (default) or 1 (no parallelism) value under NO_PTHREADS=YesPlease.

This is for consistency with the index-pack & pack-objects commands, which also take a --threads option & are configurable via pack.threads, and have long warned about the same under NO_PTHREADS=YesPlease.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c  |  8 ++++++++
 t/t7810-grep.sh | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+)

Show changes to 2 files +26 −0

builtin/grep.c, t/t7810-grep.sh

diff --git a/builtin/grep.c b/builtin/grep.c
index cb3323060e..c746cdd33c 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -290,6 +290,12 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 		if (num_threads < 0)
 			die(_("invalid number of threads specified (%d) for %s"),
 			    num_threads, var);
+#ifdef NO_PTHREADS
+		else if (num_threads && num_threads != 1) {
+			warning(_("no threads support, ignoring %s"), var);
+			num_threads = 0;
+		}
+#endif
 	}
 
 	return st;
@@ -1228,6 +1234,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (num_threads == 1)
 		num_threads = 0;
 #else
+	if (num_threads)
+		warning(_("no threads support, ignoring --threads"));
 	num_threads = 0;
 #endif
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 4523ca926b..f4c225b410 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -787,6 +787,24 @@ do
 	"
 done
 
+test_expect_success !PTHREADS,!GETTEXT_POISON 'grep --threads=N or pack.threads=N warns when no pthreads' '
+	git grep --threads=2 Hello hello_world 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 1 warnings &&
+	grep "no threads support, ignoring --threads" err &&
+	git -c grep.threads=2 grep Hello hello_world 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 1 warnings &&
+	grep "no threads support, ignoring grep\.threads" err &&
+	git -c grep.threads=2 grep --threads=4 Hello hello_world 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 2 warnings &&
+	grep "no threads support, ignoring --threads" err &&
+	grep "no threads support, ignoring grep\.threads" err &&
+	git -c grep.threads=0 grep --threads=0 Hello hello_world 2>err &&
+	test_line_count = 0 err
+'
+
 test_expect_success 'grep from a subdirectory to search wider area (1)' '
 	mkdir -p s &&
 	(

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 7/8] pack-objects: fix buggy warning about threads under NO_PTHREADS=YesPlease

Fix a buggy warning about threads under NO_PTHREADS=YesPlease. Due to re-using the delta_search_threads variable for both the state of the "pack.threads" config & the --threads option setting "pack.threads" but not supplying --threads would trigger the warning for both "pack.threads" & --threads.

Solve this bug by resetting the delta_search_threads variable in git_pack_config(), it might then be set by --threads again and be subsequently warned about, as the test I'm changing here asserts.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 4 +++-
 t/t5300-pack-object.sh | 3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

Show changes to 2 files +4 −3

builtin/pack-objects.c, t/t5300-pack-object.sh

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 84af7c2324..905465e91f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2472,8 +2472,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 			die("invalid number of threads specified (%d)",
 			    delta_search_threads);
 #ifdef NO_PTHREADS
-		if (delta_search_threads != 1)
+		if (delta_search_threads != 1) {
 			warning("no threads support, ignoring %s", k);
+			delta_search_threads = 0;
+		}
 #endif
 		return 0;
 	}
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 6bb6a8981b..50e1ae87a4 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -445,8 +445,7 @@ test_expect_success !PTHREADS,!GETTEXT_POISON 'pack-objects --threads=N or pack.
 	git -c pack.threads=2 pack-objects --stdout --all </dev/null >/dev/null 2>err &&
 	cat err &&
 	grep ^warning: err >warnings &&
-	test_line_count = 2 warnings &&
-	grep "no threads support, ignoring --threads" err &&
+	test_line_count = 1 warnings &&
 	grep "no threads support, ignoring pack\.threads" err &&
 	git -c pack.threads=2 pack-objects --threads=4 --stdout --all </dev/null >/dev/null 2>err &&
 	grep ^warning: err >warnings &&

-- 
2.11.0

Ævar Arnfjörð Bjarmason· Apr 16, 2017, 22:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 6/8] pack-object & index-pack: add test for --threads warning under NO_PTHREADS

Add a test for the warning that's emitted when --threads or pack.threads is provided under NO_PTHREADS. This uses the new PTHREADS prerequisite.

The assertion for !GETTEXT_POISON in the latter test is currently redundant, since unlike index-pack the pack-objects warnings aren't i18n'd. However they might be changed to be i18n'd in the future, and there's no harm in future-proofing the test.

There's an existing bug in the implementation of pack-objects which this test currently tests for as-is. Details about the bug & the fix are included in a follow-up change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5300-pack-object.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Show changes to t/t5300-pack-object.sh +34 −0

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 43a672c345..6bb6a8981b 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -421,6 +421,40 @@ test_expect_success 'index-pack <pack> works in non-repo' '
 	test_path_is_file foo.idx
 '
 
+test_expect_success !PTHREADS,!GETTEXT_POISON 'index-pack --threads=N or pack.threads=N warns when no pthreads' '
+	test_must_fail git index-pack --threads=2 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 1 warnings &&
+	grep "no threads support, ignoring --threads=2" err &&
+	test_must_fail git -c pack.threads=2 index-pack 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 1 warnings &&
+	grep "no threads support, ignoring pack\.threads" err &&
+	test_must_fail git -c pack.threads=2 index-pack --threads=4 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 2 warnings &&
+	grep "no threads support, ignoring --threads=4" err &&
+	grep "no threads support, ignoring pack\.threads" err
+'
+
+test_expect_success !PTHREADS,!GETTEXT_POISON 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
+	git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 1 warnings &&
+	grep "no threads support, ignoring --threads" err &&
+	git -c pack.threads=2 pack-objects --stdout --all </dev/null >/dev/null 2>err &&
+	cat err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 2 warnings &&
+	grep "no threads support, ignoring --threads" err &&
+	grep "no threads support, ignoring pack\.threads" err &&
+	git -c pack.threads=2 pack-objects --threads=4 --stdout --all </dev/null >/dev/null 2>err &&
+	grep ^warning: err >warnings &&
+	test_line_count = 2 warnings &&
+	grep "no threads support, ignoring --threads" err &&
+	grep "no threads support, ignoring pack\.threads" err
+'
+
 #
 # WARNING!
 #

-- 
2.11.0