From cea69151a4d4c0861a6dd5006267141b04ebbadb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 7 Oct 2020 14:19:23 -0400 Subject: [PATCH 1/3] index-pack: restore "resolving deltas" progress meter Commit f08cbf60fe (index-pack: make quantum of work smaller, 2020-09-08) refactored the main loop in threaded_second_pass(), but also deleted the call to display_progress() at the top of the loop. This means that users typically see no progress at all during the delta resolution phase (and for large repositories, Git appears to hang). This looks like an accident that was unrelated to the intended change of that commit, since we continue to update nr_resolved_deltas in resolve_delta(). Let's restore the call to get that progress back. We'll also add a test that confirms we generate the expected progress. This isn't perfect, as it wouldn't catch a bug where progress was delayed to the end. That was probably possible to trigger when receiving a thin pack, because we'd eventually call display_progress() from fix_unresolved_deltas(), but only once after doing all the work. However, since our test case generates a complete pack, it reliably demonstrates this particular bug and its fix. And we can't do better without making the test racy. Signed-off-by: Jeff King Acked-by: Jonathan Tan Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 4 ++++ t/t5302-pack-index.sh | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 8da4031e72..0f05533902 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1028,6 +1028,10 @@ static void *threaded_second_pass(void *data) struct object_entry *child_obj; struct base_data *child; + counter_lock(); + display_progress(progress, nr_resolved_deltas); + counter_unlock(); + work_lock(); if (list_empty(&work_head)) { /* diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh index c92e553a2f..7c9d687367 100755 --- a/t/t5302-pack-index.sh +++ b/t/t5302-pack-index.sh @@ -277,4 +277,11 @@ test_expect_success 'index-pack --fsck-objects also warns upon missing tagger in grep "^warning:.* expected .tagger. line" err ' +test_expect_success 'index-pack -v --stdin produces progress for both phases' ' + pack=$(git pack-objects --all pack err && + test_i18ngrep "Receiving objects" err && + test_i18ngrep "Resolving deltas" err +' + test_done From bebe1719470078b000950b0cca520d3e533996a9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 7 Oct 2020 14:19:43 -0400 Subject: [PATCH 2/3] index-pack: drop type_cas mutex The type_cas lock lost all of its callers in f08cbf60fe (index-pack: make quantum of work smaller, 2020-09-08), so we can safely delete it. The compiler didn't alert us that the variable became unused, because we still call pthread_mutex_init() and pthread_mutex_destroy() on it. It's worth considering also whether that commit was in error to remove the use of the lock. Why don't we need it now, if we did before, as described in ab791dd138 (index-pack: fix race condition with duplicate bases, 2014-08-29)? I think the answer is that we now look at and assign the child_obj->real_type field in the main thread while holding the work_lock(). So we don't have to worry about racing with the worker threads. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 0f05533902..f8f1b48e56 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -159,10 +159,6 @@ static pthread_mutex_t deepest_delta_mutex; #define deepest_delta_lock() lock_mutex(&deepest_delta_mutex) #define deepest_delta_unlock() unlock_mutex(&deepest_delta_mutex) -static pthread_mutex_t type_cas_mutex; -#define type_cas_lock() lock_mutex(&type_cas_mutex) -#define type_cas_unlock() unlock_mutex(&type_cas_mutex) - static pthread_key_t key; static inline void lock_mutex(pthread_mutex_t *mutex) @@ -186,7 +182,6 @@ static void init_thread(void) init_recursive_mutex(&read_mutex); pthread_mutex_init(&counter_mutex, NULL); pthread_mutex_init(&work_mutex, NULL); - pthread_mutex_init(&type_cas_mutex, NULL); if (show_stat) pthread_mutex_init(&deepest_delta_mutex, NULL); pthread_key_create(&key, NULL); @@ -209,7 +204,6 @@ static void cleanup_thread(void) pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&counter_mutex); pthread_mutex_destroy(&work_mutex); - pthread_mutex_destroy(&type_cas_mutex); if (show_stat) pthread_mutex_destroy(&deepest_delta_mutex); for (i = 0; i < nr_threads; i++) From ec6a8f9705cb61956f7fac61a4c0e27d9ae4bbd5 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 7 Oct 2020 13:16:58 -0700 Subject: [PATCH 3/3] index-pack: make get_base_data() comment clearer A comment mentions that we may free cached delta bases via find_unresolved_deltas(), but that function went away in f08cbf60fe (index-pack: make quantum of work smaller, 2020-09-08). Since we need to rewrite that comment anyway, make the entire comment clearer. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index f8f1b48e56..aee33ae6a8 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -888,18 +888,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry, } /* - * Walk from current node up - * to top parent if necessary to deflate the node. In normal - * situation, its parent node would be already deflated, so it just - * needs to apply delta. + * Ensure that this node has been reconstructed and return its contents. * - * In the worst case scenario, parent node is no longer deflated because - * we're running out of delta_base_cache_limit; we need to re-deflate - * parents, possibly up to the top base. - * - * All deflated objects here are subject to be freed if we exceed - * delta_base_cache_limit, just like in find_unresolved_deltas(), we - * just need to make sure the last node is not freed. + * In the typical and best case, this node would already be reconstructed + * (through the invocation to resolve_delta() in threaded_second_pass()) and it + * would not be pruned. However, if pruning of this node was necessary due to + * reaching delta_base_cache_limit, this function will find the closest + * ancestor with reconstructed data that has not been pruned (or if there is + * none, the ultimate base object), and reconstruct each node in the delta + * chain in order to generate the reconstructed data for this node. */ static void *get_base_data(struct base_data *c) {