Re: [PATCH v4] Add git-grep threads param

From: Jeff King <peff@peff.net>
Date: Nov 9, 2015, 16:53 UTC
Message-ID: <20151109165343.GA29179@sigill.intra.peff.net>
In-Reply-To: <6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA17@mail.accesssoftek.com>

On Mon, Nov 09, 2015 at 08:34:27AM -0800, Victor Leschuk wrote:

Show 12 quoted lines

> > Why don't we leave it at 8, then? That's the conservative choice, and
> > once we have --threads, people can easily experiment with different
> > values and we can follow-up with a change to the default if need be.
>
> I'd propose the following:
>
>     if (list.nr || cached) {
>         num_threads = 0; /* Can not multi-thread object lookup */
>     }
>     else if (num_threads < 0 && online_cpus() <= 1) {
>         num_threads = 0; /* User didn't set threading option and we have <= 1 of hardware cores */
>     }

OK, so you are presumably initializing:

  static int num_threads = -1;

Show 6 quoted lines

>     else if (num_threads == 0) {
>         num_threads = GREP_NUM_THREADS_DEFAULT; /* User explicitly choose default behavior */
>     }
>     else if (num_threads < 0) {  /* Actually this one should be checked earlier so no need to double check here */
>         die(_("Ivalid number of threads specified (%d)"), num_threads)
>     }

What happens if the user has not specified a value (nr_threads == -1)? Here you die, but shouldn't you take the default thread value?

I wonder if it would be simpler to just default to 0, and then treat negative values the same as 0 (which is what pack.threads does). Like:

  if (list.nr || cached)
	num_threads = 1;
  if (!num_threads)
	num_threads = GREP_NUM_THREADS_DEFAULT;

and then later, instead of use_threads, do:

  if (num_threads <= 1) {
	... do single-threaded version ...
  } else {
        ... do multi-threaded version ...
  }

That matches the logic in builtin/pack-objects.c.

-Peff