git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0

From
Jeff King <peff@peff.net>
Date
Apr 14, 2017, 21:23 UTC
Message-ID
<20170414212325.fefrl3qdjigwyitd@sigill.intra.peff.net>
In-Reply-To
<CACBZZX7Xi2OWqHQd7jTGBEZyqcWk59oXbPJOjuYrYAFzd5huCA@mail.gmail.com>
On Tue, Apr 11, 2017 at 10:56:01PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 10 quoted lines
> > Right, my suggestion was to teach "grep" to treat --threads=1 as "do not
> > spawn any other threads". I.e., to make it like the "0" case you were
> > proposing, and then leave "0" as "auto-detect". There would be no way to
> > spawn a _single_ thread and feed it. But why would you want to do that?
> > It's always going to be strictly worse than not threading at all.
> 
> I understand, but given the two profiles we've posted it seems clear
> that there's cases where if we did that, we'd be locking people out of
> their optimal thread configuration, which would be --thread=1 with my
> patch, but wouldn't exist with this proposed change.

Maybe I don't understand your profiles. For a single-core machine you probably want fewer threads, right? There is no such thing as "0" threads, as you always have the original main thread in which we would do the work. So the lowest you can go is "1" (it's a separate question of what --threads=0 should "mean"; I think we should keep it as "auto-detect" for compatibility).

We could implement the single-thread case by spawning off one worker thread (and effectively having 2 threads, but one is just sitting in pthread_join()). And I think that's how it's implemented now in git-grep. But we can optimize out the creation of the second thread entirely, and just do the work in the main thread.

That saves a little bit of thread-spawning overhead, and it also makes debugging much more pleasant. For --threads=2, you'd always have to kick in the thread-spawning code, and you'd spawn two worker threads (and the main thread just sits there).

IOW, I am just proposing something like this:
diff --git a/builtin/grep.c b/builtin/grep.c
index 65070c52f..76ce38404 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -326,7 +326,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	}
 
 #ifndef NO_PTHREADS
-	if (num_threads) {
+	if (num_threads > 1) {
 		add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
 		strbuf_release(&pathbuf);
 		return 0;
@@ -360,7 +360,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	}
 
 #ifndef NO_PTHREADS
-	if (num_threads) {
+	if (num_threads > 1) {
 		add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
 		strbuf_release(&buf);
 		return 0;

where we fall back to the same code that NO_PTHREADS uses when there is
only a single thread requested.

> Anyway, I really don't care about this feature much, I just wanted a
> way to disable threading, but looking at the perf profiles I wonder if
> doing your proposed change would cause a regression in some cases
> where someone really wanted /one/ thread.

I'm not sure what would regress. Right now --threads=1 only does
work in a single worker thread. And --threads=2 does it in 2, and so on.
In all cases, the original main-thread is just farming out work and
waiting on pthread_join() (let's call that the controller thread).  So
why would you ever want the "controller plus one worker" setup? It's
strictly worse than "controller just does the work".

> But of course my patch breaks the long documented grep.threads=0 for
> "give me threads that you auto detect" to now mean "you get none".

Right, that's what I'm concerned about.

> Also doesn't --thread=1 right now mean "one thread, but two workers?".
> I haven't dug into the grep worker/thread code, but it compiles the
> the pattern twice, so isn't both the non-thread main process & the
> sole thread it spawns on --thread=1 doing work, so in some other
> universe it's synonymous with --workers=2?

I think --threads=1 right now means "one worker thread". I think the
main program calls compile_grep_patterns() to make sure they are sane,
before it even considers whether and how to thread.  And then each
worker thread duplicates them and re-compiles them itself (IIRC, this is
because the regex code may not be thread-safe).

> If so do pack-objects & index-pack also behave like that? If so this
> whole thing is very confusing for users, because some will read 1
> thread and think "one worker", whereas it really means "two workers,
> one using a thread, if you want three workers spawn two threads".

No, I think --threads is "this many workers". If you have more than one
worker, you may have an extra thread farming out work to them, but that
isn't counted (and is mostly dormant).

-Peff
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 7 of 43 in “PCREv2 & more”
  1. 00/12 PCREv2 & moreÆvar Arnfjörð Bjarmason, Apr 8, 2017
  2. 01/12 grep: add ability to disable threading with --threads=0 or grep.threads=0Ævar Arnfjörð Bjarmason, Apr 8, 2017
  3. Jeff KingApr 11, 2017
  4. Ævar Arnfjörð BjarmasonApr 11, 2017
  5. Jeff KingApr 11, 2017
  6. Ævar Arnfjörð BjarmasonApr 11, 2017
  7. Jeff KingApr 14, 2017
  8. Ævar Arnfjörð BjarmasonApr 16, 2017
  9. 02/12 grep: remove redundant regflags assignment under PCREÆvar Arnfjörð Bjarmason, Apr 8, 2017
  10. Jeff KingApr 11, 2017
  11. 03/12 Makefile & configure: reword outdated comment about PCREÆvar Arnfjörð Bjarmason, Apr 8, 2017
  12. Jeff KingApr 11, 2017
  13. Ævar Arnfjörð BjarmasonApr 15, 2017
  14. 04/12 grep: add a test for backreferences in PCRE patternsÆvar Arnfjörð Bjarmason, Apr 8, 2017
  15. 05/12 log: add exhaustive tests for pattern style options & configÆvar Arnfjörð Bjarmason, Apr 8, 2017
  16. Jeff KingApr 11, 2017
  17. Ævar Arnfjörð BjarmasonApr 11, 2017
  18. Jeff KingApr 11, 2017
  19. 06/12 log: add -P as a synonym for --perl-regexpÆvar Arnfjörð Bjarmason, Apr 8, 2017
  20. Junio C HamanoApr 10, 2017
  21. Jeff KingApr 11, 2017
  22. 08/12 grep: make grep.patternType=[pcre|pcre1] a synonym for "perl"Ævar Arnfjörð Bjarmason, Apr 8, 2017
  23. Jeff KingApr 11, 2017
  24. 07/12 grep & rev-list doc: stop promising libpcre for --perl-regexpÆvar Arnfjörð Bjarmason, Apr 8, 2017
  25. 09/12 test-lib: rename the LIBPCRE prerequisite to PCREÆvar Arnfjörð Bjarmason, Apr 8, 2017
  26. Jeff KingApr 11, 2017
  27. 10/12 grep: change the internal PCRE macro names to be PCRE1Ævar Arnfjörð Bjarmason, Apr 8, 2017
  28. Jeff KingApr 11, 2017
  29. Ævar Arnfjörð BjarmasonApr 11, 2017
  30. Jeff KingApr 11, 2017
  31. 12/12 grep: add support for PCRE v2Ævar Arnfjörð Bjarmason, Apr 8, 2017
  32. Jeff KingApr 11, 2017
  33. 11/12 grep: change the internal PCRE code & header names to be PCRE1Ævar Arnfjörð Bjarmason, Apr 8, 2017
  34. Jeff KingApr 11, 2017
  35. Ævar Arnfjörð BjarmasonApr 11, 2017
  36. Jeff KingApr 11, 2017
  37. Ævar Arnfjörð BjarmasonApr 11, 2017
  38. Jeff KingApr 11, 2017
  39. Brandon WilliamsApr 11, 2017
  40. Ævar Arnfjörð BjarmasonApr 11, 2017
  41. Jeff KingApr 11, 2017
  42. Junio C HamanoApr 15, 2017
  43. Ævar Arnfjörð BjarmasonApr 15, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.