Please pay attention to your title. It no longer matches what the
patch does.
It may also be beneficial to study recent titles and log messages
from other developers (you can find them in "git log --no-merges"
and "git shortlog --no-merges") and learn and imitate their format,
style and tone. We want our log to tell a story in a consistent
voice, no matter who the authors of individual commits are.
Victor Leschuk <vleschuk@xxxxxxxxx> writes:
> It's a follow up to "[PATCH] Add git-grep threads-num param":
Do you think anybody wants to see this line in the output from "git
log" six months from now? I doubt it. The previous one will not be
committed to my tree anyway, so the readers would not know (nor
care) what other patch you are talking about.
> @@ -832,7 +836,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> }
>
> #ifndef NO_PTHREADS
> - if (list.nr || cached || online_cpus() == 1)
> + if (list.nr || cached || online_cpus() == 1 || opt.num_threads <= 1)
> use_threads = 0;
This avoid --threads=0 to take the threading codepath and spawning
no threads, which would have happend in the previous patch.
But it makes me wonder if the logic should be more like this:
- Because the code is not prepared to go multi-thread when
searching in the object data (not working tree), we always
disable threading if 'list' is not empty or 'cached' is given;
otherwise
- If the user explicitly said that she wants N threads, we use that
many threads; otherwise
- If there is only one CPU, we do not do multi-thread; otherwise
- We use the default number of threads.
IOW, I'd suggest making opt.num_threads an "int" (not "unsigned"),
initialize it to -1 (unspecified), and then make this part more like
this, perhaps?
if (!opt.num_threads)
use_threads = 0; /* the user tells us not to use threads */
else if (list.nr || cached)
use_threads = 0; /* cannot multi-thread object lookup */
else if (opt.num_threads >= 1)
use_threads = 1; /* the user explicitly wants this many */
else if (online_cpus() <= 1)
use_threads = 0;
else {
use_threads = 1;
opt.num_threads = GREP_NUM_THREADS_DEFAULT;
}
Something like this code structure makes it very clear what needs to
be changed when we want to add some sort of auto-scaling (instead of
assigning the DEFAULT constant, you'd see how many cores you have,
how many files you will be grepping in, etc. and come up with a good
number dynamically).
> @@ -150,6 +159,8 @@ void grep_init(struct grep_opt *opt, const char *prefix)
> opt->pathname = def->pathname;
> opt->regflags = def->regflags;
> opt->relative = def->relative;
> + if(!opt->num_threads)
You forgot a required SP between a keyword for a syntactic construct
and its open parenthesis.
Thanks.
--
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