On Mon, Nov 09, 2015 at 08:34:27AM -0800, Victor Leschuk wrote:
> > 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;
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html