[PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

[PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

[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

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

[Jeff King]

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

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

[Junio C Hamano]

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

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

[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

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

[Jeff King]

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