* [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter @ 2018-10-22 15:05 Ben Peart 2018-10-22 20:17 ` Jeff King 0 siblings, 1 reply; 5+ messages in thread From: Ben Peart @ 2018-10-22 15:05 UTC (permalink / raw) To: git; +Cc: gitster, Ben Peart 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. Signed-off-by: Ben Peart <benpeart@microsoft.com> --- Notes: Base Ref: Web-Diff: https://github.com/benpeart/git/commit/7360721408 Checkout: git fetch https://github.com/benpeart/git read-index-multithread-no-src-offset-v1 && git checkout 7360721408 read-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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); base-commit: f58b85df6937e3f3d9ef26bb52a513c8ada17ffc -- 2.18.0.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter 2018-10-22 15:05 [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter Ben Peart @ 2018-10-22 20:17 ` Jeff King 2018-10-22 23:05 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Jeff King @ 2018-10-22 20:17 UTC (permalink / raw) To: Ben Peart; +Cc: git, gitster, Ben Peart 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter 2018-10-22 20:17 ` Jeff King @ 2018-10-22 23:05 ` Junio C Hamano 2018-10-23 19:13 ` Ben Peart 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2018-10-22 23:05 UTC (permalink / raw) To: Jeff King; +Cc: Ben Peart, git, Ben Peart 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter 2018-10-22 23:05 ` Junio C Hamano @ 2018-10-23 19:13 ` Ben Peart 2018-10-23 20:07 ` Jeff King 0 siblings, 1 reply; 5+ messages in thread From: Ben Peart @ 2018-10-23 19:13 UTC (permalink / raw) To: Junio C Hamano, Jeff King; +Cc: git, Ben Peart 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter 2018-10-23 19:13 ` Ben Peart @ 2018-10-23 20:07 ` Jeff King 0 siblings, 0 replies; 5+ messages in thread From: Jeff King @ 2018-10-23 20:07 UTC (permalink / raw) To: Ben Peart Cc: Junio C Hamano, git, Ben Peart, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-23 20:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-22 15:05 [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter Ben Peart 2018-10-22 20:17 ` Jeff King 2018-10-22 23:05 ` Junio C Hamano 2018-10-23 19:13 ` Ben Peart 2018-10-23 20:07 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).