[09/10] read-cache.c: remove #ifdef NO_PTHREADS
diff mbox series

Message ID 20181027071003.1347-10-pclouds@gmail.com
State New
Headers show
Series
  • Reduce #ifdef NO_PTHREADS
Related show

Commit Message

Duy Nguyen Oct. 27, 2018, 7:10 a.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

Ben Peart Oct. 29, 2018, 5:05 p.m. UTC | #1
On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   read-cache.c | 49 ++++++++++++++++++-------------------------------
>   1 file changed, 18 insertions(+), 31 deletions(-)
> 
> 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) {

In this case, nr_threads is already capped to online_cpus() below so 
this HAVE_THREADS test isn't needed.

> +		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) {

Here extension_offset won't be set if there wasn't a thread created so 
the HAVE_THREADS test is not needed.

>   		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)

This shouldn't be needed either.  My assumption was that if someone 
explicitly asked for multiple threads we're better off failing than 
silently ignoring their request.  The default/automatic setting will 
detect the number of cpus and behave correctly.

> +		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) {

Again, this one isn't needed.  If there is only 1 thread, we don't read 
the eoie or the ieot (see the code just above this) so the ieot test is 
sufficient.

>   		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
>
Duy Nguyen Oct. 29, 2018, 5:21 p.m. UTC | #2
On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote:
> > @@ -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)
>
> This shouldn't be needed either.  My assumption was that if someone
> explicitly asked for multiple threads we're better off failing than
> silently ignoring their request.  The default/automatic setting will
> detect the number of cpus and behave correctly.

No. I can have ~/.gitconfig shared between different machines. One
with multithread support and one without. I should not have to fall
back to conditional includes in order to use the same config file on
the machine without multithread.
Ben Peart Oct. 29, 2018, 5:58 p.m. UTC | #3
On 10/29/2018 1:21 PM, Duy Nguyen wrote:
> On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote:
>>> @@ -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)
>>
>> This shouldn't be needed either.  My assumption was that if someone
>> explicitly asked for multiple threads we're better off failing than
>> silently ignoring their request.  The default/automatic setting will
>> detect the number of cpus and behave correctly.
> 
> No. I can have ~/.gitconfig shared between different machines. One
> with multithread support and one without. I should not have to fall
> back to conditional includes in order to use the same config file on
> the machine without multithread.
> 

If you want to share the ~/.gitconfig across machines that have 
different numbers of CPUs, it makes more sense to me to leave the 
setting at the "auto" setting rather than specifically overriding it to 
a number that won't work on at least one of your machines.  Then it all 
"just works" without the need to conditional includes.
Junio C Hamano Oct. 30, 2018, 1:44 a.m. UTC | #4
Duy Nguyen <pclouds@gmail.com> writes:

>> This shouldn't be needed either.  My assumption was that if someone
>> explicitly asked for multiple threads we're better off failing than
>> silently ignoring their request.  The default/automatic setting will
>> detect the number of cpus and behave correctly.
>
> No. I can have ~/.gitconfig shared between different machines. One
> with multithread support and one without. I should not have to fall
> back to conditional includes in order to use the same config file on
> the machine without multithread.

Our attitude has been to fail unsatisfyable requests from the
command line loudly to let the users' know, and tolerate having
elements from the future versions of Git in the configuration file
by ignoring them or tweaking them (silently or with a warning).

So this hunk is in line with the current practice, and we should
keep it this way.  It is a separate discussion if we want to rethink
that whole attitude, that discussion may be worth having, and I
think that discussion is wider than this single topic.

Patch
diff mbox series

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