| Message ID | 20181022150513.18028-1-peartben@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] load_cache_entries_threaded: remove unused src_offset parameter | expand |
On Mon, Oct 22, 2018 at 11:05:13AM -0400, Ben Peart wrote: > 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: > 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
Jeff King <peff@peff.net> writes: > If nobody uses it, should we drop the return value, too? Like: Yup. > > 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
On 10/22/2018 7:05 PM, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> If nobody uses it, should we drop the return value, too? Like: > > Yup. > I'm good with that. At one point I also had the additional #ifndef NO_PTHREADS lines but it was starting to get messy with the threaded vs non-threaded code paths so I removed them. I'm fine with which ever people find more readable. It does make me wonder if there are still platforms taking new build of git that don't support threads. Do we still need to write/test/debug/read through the single threaded code paths? Is the diff Peff sent enough or do you want me to send another iteration on the patch? Thanks, Ben >> >> 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
On Tue, Oct 23, 2018 at 03:13:06PM -0400, Ben Peart wrote: > At one point I also had the additional #ifndef NO_PTHREADS lines but it was > starting to get messy with the threaded vs non-threaded code paths so I > removed them. I'm fine with which ever people find more readable. > > It does make me wonder if there are still platforms taking new build of git > that don't support threads. Do we still need to write/test/debug/read > through the single threaded code paths? I think the classic offenders here were old Unix systems like AIX, etc. I've no idea what the current state is on those platforms. I would love it if we could drop NO_PTHREADS. There's a lot of gnarly code there, and I strongly suspect a lot of bugs lurk in the non-threaded halves (e.g., especially around bits like "struct async" which is "maybe a thread, and maybe a fork" depending on your system, which introduces all kinds of subtle process-state dependencies). But I'm not really sure how to find out aside from adding a deprecation warning and seeing if anybody screams. See also this RFC from Duy, which might at least make the code itself a little easier to follow: https://public-inbox.org/git/20181018180522.17642-1-pclouds@gmail.com/ -Peff
diff --git a/read-cache.c b/read-cache.c index f9fa6a7979..6db6f0f220 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2037,7 +2037,7 @@ static void *load_cache_entries_thread(void *_data) } 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) { int i, offset, ieot_blocks, ieot_start, err; struct load_cache_entries_thread_data *data; @@ -2198,7 +2198,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) ieot = read_ieot_extension(mmap, mmap_size, extension_offset); 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);