Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

From: Ben Peart <peartben@gmail.com>
Date: Oct 29, 2018, 17:05 UTC
Message-ID: <9e489011-36b9-a8b3-838d-de1a097f4854@gmail.com>
In-Reply-To: <20181027071003.1347-10-pclouds@gmail.com>

On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote:

Show 64 quoted lines

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

Show 35 quoted lines

> +		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.

Show 15 quoted lines

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

Show 22 quoted lines

> +		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.

Show 25 quoted lines

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