Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

[Jeff King <peff@peff.net>]

On Mon, Oct 29, 2018 at 02:05:58PM -0400, Ben Peart wrote:

> On 10/29/2018 1:26 PM, Duy Nguyen wrote:
> > On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
> > > @@ -114,6 +104,9 @@ static void preload_index(struct index_state *index,
> > >           threads = index->cache_nr / THREAD_COST;
> > >           if ((index->cache_nr > 1) && (threads < 2) &&
> > > git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
> > >                   threads = 2;
> > > +       cpus = online_cpus();
> > > +       if (threads > cpus)
> > > +               threads = cpus;
> > >           if (threads < 2)
> > >                   return;
> > >           trace_performance_enter();
> > 
> > Capping the number of threads to online_cpus() does not always make
> > sense. In this case (or at least the original use case where we stat()
> > over nfs) we want to issue as many requests to nfs server as possible
> > to reduce latency and it has nothing to do with how many cores we
> > have. Using more threads than cores might make sense since threads are
> > blocked by nfs client, but we still want to send more to the server.
> > 
> 
> That makes sense.  Some test will be necessary then.
> 
> I guess HAVE_THREADS is functionally the same as online_cpus() == 1. For
> some reason, I prefer the latter - probably because I'm typically already
> calling it and so it doesn't feel like a 'special' value/test I have to
> remember. I just know I need to make sure the logic works correctly with any
> number of cps greater than zero. :-)

I don't think they're functionally the same. If you see online_cpus()
== 1, you are in one of two cases:

  - the system supports threads, but CPU-bound tasks should stick to a
    single thread

  - the system does not even support threading, and calling
    pthread_create() will give you ENOSYS

In the first case, if your task is _not_ CPU bound (i.e., the stat()
thing), then you want to distinguish those cases.

I'm not sure if I'm violently agreeing, but it just wasn't clear to me
from the above discussion that we all agreed that HAVE_THREADS is still
necessary in some cases.

(I do think in cases where it is not, that we would do just as well to
avoid it; if that is all you were saying, then I agree. ;) ).

-Peff