Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter
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-pclo...@gmail.com/ -Peff
Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter
On 10/22/2018 7:05 PM, Junio C Hamano wrote:
Jeff King 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
Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter
Jeff King 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
Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter
On Mon, Oct 22, 2018 at 11:05:13AM -0400, Ben Peart wrote:
> From: Ben Peart
>
> 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
[PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter
From: Ben Peart
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
---
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
