diff mbox series

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

Message ID 20181027173008.18852-7-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series Reduce #ifdef NO_PTHREADS | expand

Commit Message

Duy Nguyen Oct. 27, 2018, 5:30 p.m. UTC 
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

Comments

Ben Peart Oct. 29, 2018, 5:21 p.m. UTC | #1 
On 10/27/2018 1:30 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   preload-index.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/preload-index.c b/preload-index.c
> index 9e7152ab14..0e24886aca 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -7,17 +7,7 @@
>   #include "fsmonitor.h"
>   #include "config.h"
>   #include "progress.h"
> -
> -#ifdef NO_PTHREADS
> -static void preload_index(struct index_state *index,
> -			  const struct pathspec *pathspec,
> -			  unsigned int refresh_flags)
> -{
> -	; /* nothing */
> -}
> -#else
> -
> -#include <pthread.h>
> +#include "thread-utils.h"
>   
>   /*
>    * Mostly randomly chosen maximum thread counts: we
> @@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,
>   	struct thread_data data[MAX_PARALLEL];
>   	struct progress_data pd;
>   
> -	if (!core_preload_index)
> +	if (!HAVE_THREADS || !core_preload_index)
>   		return;
>   
>   	threads = index->cache_nr / THREAD_COST;
> @@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,
>   
>   	trace_performance_leave("preload index");
>   }
> -#endif
>   
>   int read_index_preload(struct index_state *index,
>   		       const struct pathspec *pathspec,
> 


In the theory that code speaks louder than comments, here is the patch I 
used when testing out the idea of getting rid of NO_PTHREADS:


  git diff head~1 preload-index.c
diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..266bc9d8d7 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@
  #include "fsmonitor.h"
  #include "config.h"
  #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
-                         const struct pathspec *pathspec,
-                         unsigned int refresh_flags)
-{
-       ; /* nothing */
-}
-#else
-
-#include <pthread.h>
+#include "thread-utils.h"

  /*
   * Mostly randomly chosen maximum thread counts: we
@@ -104,7 +94,7 @@ static void preload_index(struct index_state *index,
                           const struct pathspec *pathspec,
                           unsigned int refresh_flags)
  {
-       int threads, i, work, offset;
+       int cpus, threads, i, work, offset;
         struct thread_data data[MAX_PARALLEL];
         struct progress_data pd;

@@ -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();
@@ -151,7 +144,6 @@ static void preload_index(struct index_state *index,

         trace_performance_leave("preload index");
  }
-#endif

  int read_index_preload(struct index_state *index,
                        const struct pathspec *pathspec,
Duy Nguyen Oct. 29, 2018, 5:26 p.m. UTC | #2 
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.
Ben Peart Oct. 29, 2018, 6:05 p.m. UTC | #3 
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. :-)
Jeff King Oct. 29, 2018, 8:11 p.m. UTC | #4 
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
diff mbox series

Patch

diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,17 +7,7 @@ 
 #include "fsmonitor.h"
 #include "config.h"
 #include "progress.h"
-
-#ifdef NO_PTHREADS
-static void preload_index(struct index_state *index,
-			  const struct pathspec *pathspec,
-			  unsigned int refresh_flags)
-{
-	; /* nothing */
-}
-#else
-
-#include <pthread.h>
+#include "thread-utils.h"
 
 /*
  * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@  static void preload_index(struct index_state *index,
 	struct thread_data data[MAX_PARALLEL];
 	struct progress_data pd;
 
-	if (!core_preload_index)
+	if (!HAVE_THREADS || !core_preload_index)
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
@@ -151,7 +141,6 @@  static void preload_index(struct index_state *index,
 
 	trace_performance_leave("preload index");
 }
-#endif
 
 int read_index_preload(struct index_state *index,
 		       const struct pathspec *pathspec,