load_cache_entries_threaded: remove unused src_offset parameter

PATCH In review Ben Peart 3 participants 5 messages Oct 22, 2018 – Oct 23, 2018

lore

Share & tools

lore

Ben Peart

author

2018-10-22 15:05 UTC #0 lore raw

PATCH 1/1 load_cache_entries_threaded: remove unused src_offset parameter

From: Ben Peart <[email protected]>

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 <[email protected]>

---

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(-)

read-cache.c +2 -2

index f9fa6a7979..6db6f0f220 100644
@@ -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

Signature · 1 line

2.18.0.windows.1

Jeff King

2018-10-22 20:17 UTC #1 lore raw

Quoted 5 lines · Ben Peart

From: Ben Peart <[email protected]>
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:

Review

Quoted 6 lines

(quoted diff) +1 -1

 	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.

Review

Quoted 3 lines

static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,
(quoted diff) +1 -1
-			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:

read-cache.c +2 -6

index 78c9516eb7..4b44a2eae5 100644
@@ -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

Junio C Hamano

2018-10-22 23:05 UTC #2 lore raw

Quoted 1 line · Jeff King

If nobody uses it, should we drop the return value, too? Like:

Yup.

Quoted 35 lines

read-cache.c +2 -7

index 78c9516eb7..4b44a2eae5 100644
@@ -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

Ben Peart

author

2018-10-23 19:13 UTC #3 lore raw

Quoted 6 lines · Junio C Hamano

Quoted 1 line · Jeff King
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

Quoted 35 lines

read-cache.c +2 -7

index 78c9516eb7..4b44a2eae5 100644
@@ -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

2018-10-23 20:07 UTC #4 lore raw

Quoted 7 lines · Ben Peart

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/[email protected]/

-Peff