On Mon, Oct 22, 2018 at 11:05:13AM -0400, Ben Peart wrote:
> From: Ben Peart <benpeart@xxxxxxxxxxxxx>
>
> 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:
> 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
