diff mbox series

[v2,08/10] read-cache.c: remove #ifdef NO_PTHREADS

Message ID 20181027173008.18852-9-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>
---
 read-cache.c | 49 ++++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

Comments

Jeff King Oct. 29, 2018, 2:30 p.m. UTC | #1 
On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:

> -#ifndef NO_PTHREADS
> -	nr_threads = git_config_get_index_threads();
> +	if (HAVE_THREADS) {
> +		nr_threads = git_config_get_index_threads();
>  
> -	/* TODO: does creating more threads than cores help? */
> -	if (!nr_threads) {
> -		nr_threads = istate->cache_nr / THREAD_COST;
> -		cpus = online_cpus();
> -		if (nr_threads > cpus)
> -			nr_threads = cpus;
> +		/* TODO: does creating more threads than cores help? */
> +		if (!nr_threads) {
> +			nr_threads = istate->cache_nr / THREAD_COST;
> +			cpus = online_cpus();
> +			if (nr_threads > cpus)
> +				nr_threads = cpus;
> +		}
> +	} else {
> +		nr_threads = 1;
>  	}

I'd have thought we could just rely on online_cpus() returning 1 here to
avoid having to ask "do we even have thread support?". But I guess that
TODO comment implies that we might one day two 2 threads on a single
CPU.

-Peff
Ben Peart Oct. 29, 2018, 5:07 p.m. UTC | #2 
On 10/29/2018 10:30 AM, Jeff King wrote:
> On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 
>> -#ifndef NO_PTHREADS
>> -	nr_threads = git_config_get_index_threads();
>> +	if (HAVE_THREADS) {
>> +		nr_threads = git_config_get_index_threads();
>>   
>> -	/* TODO: does creating more threads than cores help? */
>> -	if (!nr_threads) {
>> -		nr_threads = istate->cache_nr / THREAD_COST;
>> -		cpus = online_cpus();
>> -		if (nr_threads > cpus)
>> -			nr_threads = cpus;
>> +		/* TODO: does creating more threads than cores help? */
>> +		if (!nr_threads) {
>> +			nr_threads = istate->cache_nr / THREAD_COST;
>> +			cpus = online_cpus();
>> +			if (nr_threads > cpus)
>> +				nr_threads = cpus;
>> +		}
>> +	} else {
>> +		nr_threads = 1;
>>   	}
> 
> I'd have thought we could just rely on online_cpus() returning 1 here to
> avoid having to ask "do we even have thread support?". But I guess that
> TODO comment implies that we might one day two 2 threads on a single
> CPU.
> 
> -Peff
> 

See my earlier response - the HAVE_THREADS tests are not needed.

We can remove the "TODO" comment - I tested it and it wasn't faster to 
have more threads than CPUs.
Ben Peart Oct. 29, 2018, 5:23 p.m. UTC | #3 
On 10/29/2018 10:30 AM, Jeff King wrote:
> On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 
>> -#ifndef NO_PTHREADS
>> -	nr_threads = git_config_get_index_threads();
>> +	if (HAVE_THREADS) {
>> +		nr_threads = git_config_get_index_threads();
>>   
>> -	/* TODO: does creating more threads than cores help? */
>> -	if (!nr_threads) {
>> -		nr_threads = istate->cache_nr / THREAD_COST;
>> -		cpus = online_cpus();
>> -		if (nr_threads > cpus)
>> -			nr_threads = cpus;
>> +		/* TODO: does creating more threads than cores help? */
>> +		if (!nr_threads) {
>> +			nr_threads = istate->cache_nr / THREAD_COST;
>> +			cpus = online_cpus();
>> +			if (nr_threads > cpus)
>> +				nr_threads = cpus;
>> +		}
>> +	} else {
>> +		nr_threads = 1;
>>   	}
> 
> I'd have thought we could just rely on online_cpus() returning 1 here to
> avoid having to ask "do we even have thread support?". But I guess that
> TODO comment implies that we might one day two 2 threads on a single
> CPU.
> 
> -Peff
> 

And here is the patch I created when testing out the idea of removing 
NO_PTHREADS.  It doesn't require any 'HAVE_THREADS' tests.



diff --git a/read-cache.c b/read-cache.c
index 1df5c16dbc..1f088799fc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
         struct index_entry_offset entries[FLEX_ARRAY];
  };

-#ifndef NO_PTHREADS
  static struct index_entry_offset_table *read_ieot_extension(const char 
*mmap, size_t mmap_size, size_t offset
);
  static void write_ieot_extension(struct strbuf *sb, struct 
index_entry_offset_table *ieot);
-#endif

  static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
  static void write_eoie_extension(struct strbuf *sb, git_hash_ctx 
*eoie_context, size_t offset);

  struct load_index_extensions
  {
-#ifndef NO_PTHREADS
         pthread_t pthread;
-#endif
         struct index_state *istate;
         const char *mmap;
         size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct 
index_state *istate,
         return consumed;
  }

-#ifndef NO_PTHREADS
-
  /*
   * Mostly randomly chosen maximum thread counts: we
   * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long 
load_cache_entries_threaded(struct index_state *istate, con


         return consumed;
  }
-#endif

  /* remember to discard_cache() before reading a different cache! */
  int do_read_index(struct index_state *istate, const char *path, int 
must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist)

         size_t mmap_size;
         struct load_index_extensions p;
         size_t extension_offset = 0;
-#ifndef NO_PTHREADS
         int nr_threads, cpus;
         struct index_entry_offset_table *ieot = NULL;
-#endif

         if (istate->initialized)
                 return istate->cache_nr;
@@ -2181,16 +2172,12 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist
)

         src_offset = sizeof(*hdr);

-#ifndef NO_PTHREADS
         nr_threads = git_config_get_index_threads();
-
-       /* TODO: does creating more threads than cores help? */
-       if (!nr_threads) {
+       if (!nr_threads)
                 nr_threads = istate->cache_nr / THREAD_COST;
-               cpus = online_cpus();
-               if (nr_threads > cpus)
-                       nr_threads = cpus;
-       }
+       cpus = online_cpus();
+       if (nr_threads > cpus)
+               nr_threads = cpus;

         if (nr_threads > 1) {
                 extension_offset = read_eoie_extension(mmap, mmap_size);
@@ -2219,22 +2206,16 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist
)
         } else {
                 src_offset += load_all_cache_entries(istate, mmap, 
mmap_size, src_offset);
         }
-#else
-       src_offset += load_all_cache_entries(istate, mmap, mmap_size, 
src_offset);
-#endif

         istate->timestamp.sec = st.st_mtime;
         istate->timestamp.nsec = ST_MTIME_NSEC(st);

         /* if we created a thread, join it otherwise load the 
extensions on the primary thread */
-#ifndef NO_PTHREADS
         if (extension_offset) {
                 int ret = pthread_join(p.pthread, NULL);
                 if (ret)
                         die(_("unable to join load_index_extensions 
thread: %s"), strerror(ret));
-       }
-#endif
-       if (!extension_offset) {
+       } else {
                 p.src_offset = src_offset;
                 load_index_extensions(&p);
         }
@@ -2756,7 +2737,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

         if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
                 return -1;

-#ifndef NO_PTHREADS
         nr_threads = git_config_get_index_threads();
         if (nr_threads != 1) {
                 int ieot_blocks, cpus;
@@ -2787,7 +2767,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

                         ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
                 }
         }
-#endif

         offset = lseek(newfd, 0, SEEK_CUR);
         if (offset < 0) {
@@ -2871,7 +2850,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

          * strip_extensions parameter as we need it when loading the shared
          * index.
          */
-#ifndef NO_PTHREADS
         if (ieot) {
                 struct strbuf sb = STRBUF_INIT;

@@ -2883,7 +2861,6 @@ static int do_write_index(struct index_state 
*istate, struct tempfile *tempfile,

                 if (err)
                         return -1;
         }
-#endif

         if (!strip_extensions && istate->split_index) {
                 struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3446,6 @@ static void write_eoie_extension(struct strbuf 
*sb, git_hash_ctx *eoie_context,

         strbuf_add(sb, hash, the_hash_algo->rawsz);
  }

-#ifndef NO_PTHREADS
  #define IEOT_VERSION   (1)

  static struct index_entry_offset_table *read_ieot_extension(const char 
*mmap, size_t mmap_size, size_t offset
)
@@ -3542,4 +3518,3 @@ static void write_ieot_extension(struct strbuf 
*sb, struct index_entry_offset_ta

                strbuf_add(sb, &buffer, sizeof(uint32_t));
         }
  }
-#endif
diff mbox series

Patch

diff --git a/read-cache.c b/read-cache.c
index d57958233e..ba870bc3fd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@  struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@  static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@  static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@  int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,15 +2172,18 @@  int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS) {
+		nr_threads = git_config_get_index_threads();
 
-	/* TODO: does creating more threads than cores help? */
-	if (!nr_threads) {
-		nr_threads = istate->cache_nr / THREAD_COST;
-		cpus = online_cpus();
-		if (nr_threads > cpus)
-			nr_threads = cpus;
+		/* TODO: does creating more threads than cores help? */
+		if (!nr_threads) {
+			nr_threads = istate->cache_nr / THREAD_COST;
+			cpus = online_cpus();
+			if (nr_threads > cpus)
+				nr_threads = cpus;
+		}
+	} else {
+		nr_threads = 1;
 	}
 
 	if (nr_threads > 1) {
@@ -2219,21 +2213,16 @@  int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-#else
-	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-#endif
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-#ifndef NO_PTHREADS
-	if (extension_offset) {
+	if (HAVE_THREADS && extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
 	}
-#endif
 	if (!extension_offset) {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
@@ -2756,8 +2745,11 @@  static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS)
+		nr_threads = git_config_get_index_threads();
+	else
+		nr_threads = 1;
+
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
 
@@ -2787,7 +2779,6 @@  static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-#endif
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2871,8 +2862,7 @@  static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-#ifndef NO_PTHREADS
-	if (ieot) {
+	if (HAVE_THREADS && ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
@@ -2883,7 +2873,6 @@  static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		if (err)
 			return -1;
 	}
-#endif
 
 	if (!strip_extensions && istate->split_index) {
 		struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3458,6 @@  static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 	strbuf_add(sb, hash, the_hash_algo->rawsz);
 }
 
-#ifndef NO_PTHREADS
 #define IEOT_VERSION	(1)
 
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
@@ -3542,4 +3530,3 @@  static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif