Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

From: Jeff King <peff@peff.net>
Date: Oct 22, 2018, 20:17 UTC
Message-ID: <20181022201721.GD9917@sigill.intra.peff.net>
In-Reply-To: <20181022150513.18028-1-peartben@gmail.com>

On Mon, Oct 22, 2018 at 11:05:13AM -0400, Ben Peart wrote:

Show 5 quoted lines

> From: Ben Peart <benpeart@microsoft.com>
> 
> Remove the src_offset parameter which is unused as a result of switching
> to the IEOT table of offsets.  Also stop incrementing src_offset in the
> multi-threaded codepath as it is no longer used and could cause confusion.

Hmm, OK. We only do threads if we have ieot:

Show 6 quoted lines

>  	if (ieot) {
> -		src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, src_offset, nr_threads, ieot);
> +		load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);
>  		free(ieot);
>  	} else {
>  		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);

And we only have ieot if we had an extension_offset:

          if (extension_offset && nr_threads > 1)
                  ieot = read_ieot_extension(mmap, mmap_size, extension_offset);

So later, when we _do_ use src_offset, we know that this code should never trigger in the threaded case:

          if (!extension_offset) {
                  p.src_offset = src_offset;
                  load_index_extensions(&p);
          }

So I think it's right, but it's rather subtle. I wonder if we could do it like this:

	unsigned long entry_offset;
  [...]
  #ifndef NO_PTHREADS
	if (ieot)
		load_cache_entries_threaded(...);
	else
		entry_offset = load_all_cache_entries(...);
  #else
	entry_offset = load_all_cache_entries(...);
  [...]

  p.src_offset = src_offset + entry_offset;

and then the compiler could warn us that entry_offset is used uninitialized (though I would not be surprised if the compiler gets confused in this case).

Not sure if it is worth the trouble or not.

>  static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,
> -			unsigned long src_offset, int nr_threads, struct index_entry_offset_table *ieot)
> +			int nr_threads, struct index_entry_offset_table *ieot)

If nobody uses it, should we drop the return value, too? Like:

diff --git a/read-cache.c b/read-cache.c
index 78c9516eb7..4b44a2eae5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)
 	return NULL;
 }
 
-static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,
-						 int nr_threads, struct index_entry_offset_table *ieot)
+static void load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,
+					int nr_threads, struct index_entry_offset_table *ieot)
 {
 	int i, offset, ieot_blocks, ieot_start, err;
 	struct load_cache_entries_thread_data *data;
-	unsigned long consumed = 0;
 
 	/* a little sanity checking */
 	if (istate->name_hash_initialized)
@@ -2115,12 +2114,9 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 		if (err)
 			die(_("unable to join load_cache_entries thread: %s"), strerror(err));
 		mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
-		consumed += p->consumed;
 	}
 
 	free(data);
-
-	return consumed;
 }
 #endif
 

-Peff