Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
- Date: Mon, 29 Oct 2018 11:16:39 +0900
- From: Junio C Hamano <gitster@xxxxxxxxx>
- Subject: Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Jeff King <peff@xxxxxxxx> writes:
> Cases like this are kind of weird. I'd expect to see wait_all() return
> immediately when !HAVE_THREADS. But we don't need to, because later we
> do this:
>> - if (num_threads)
>> + if (HAVE_THREADS && num_threads)
>> hit |= wait_all();
> Which I think works, but it feels like we're introducing some subtle
> dependencies about which functions get called in which cases. I'd hoped
> in general that the non-threaded code paths could mostly just collapse
> down into the same as the "threads == 1" cases, and we wouldn't have to
> ask "are threads even supported" in a lot of places.
True, but the way "grep" code works with threads is already strange,
and I suspect that the subtle strangeness you feel mostly comes from
that. The single threaded code signaled "hit" with return value of
individual functions like grep_file(), but the meaning of return
value from them is changed to "does not matter--we do not have
meaningful result yet at this point" when threading is used.
In the new world order where "threading is the norm but
single-threaded is still supported", I wonder if we would want to
drive the single threaded case the same way with the add_work(run)
interface and return the "did we see hits?" etc. from the same (or
at lesat "more similar than today's") interface, so that the flow of
data smells the same in both cases.
> I dunno. My comment isn't really an objection exactly, but mostly just
> an indication that I'm still thinking through what the best approach is,
> and what end state we want to end up in.