[PATCH] commit: don't rewrite shared index unnecessarily

[PATCH] commit: don't rewrite shared index unnecessarily

[David Turner]

Remove a cache invalidation which would cause the shared index to be
rewritten on as-is commits.

When the cache-tree has changed, we need to update it.  But we don't
necessarily need to update the shared index.  So setting
active_cache_changed to SOMETHING_CHANGED is unnecessary.  Instead, we
let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.

In order to test this, make test-dump-split-index not segfault on
missing replace_bitmap/delete_bitmap.  This new codepath is not called
now that the test passes, but is necessary to avoid a segfault when the
new test is run with the old builtin/commit.c code.

Signed-off-by: David Turner <dturner@twopensource.com>
---

I introduced this bug last year while improving the cache-tree code.
I guess I probably didn't notice that active_cache_changed wasn't a
boolean.

---
 builtin/commit.c        |  4 +---
 t/t0090-cache-tree.sh   | 10 ++++++++++
 test-dump-split-index.c |  6 ++++--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 254477f..1692620 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -404,10 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
 		if (active_cache_changed
-		    || !cache_tree_fully_valid(active_cache_tree)) {
+		    || !cache_tree_fully_valid(active_cache_tree))
 			update_main_cache_tree(WRITE_TREE_SILENT);
-			active_cache_changed = 1;
-		}
 		if (active_cache_changed) {
 			if (write_locked_index(&the_index, &index_lock,
 					       COMMIT_LOCK))
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 601d02d..f92dd1f 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -218,4 +218,14 @@ test_expect_success 'no phantom error when switching trees' '
 	! test -s errors
 '
 
+test_expect_success 'switching trees does not invalidate shared index' '
+	git update-index --split-index &&
+	>split &&
+	git add split &&
+	test-dump-split-index .git/index | grep -v ^own >before &&
+	git commit -m "as-is" &&
+	test-dump-split-index .git/index | grep -v ^own >after &&
+	test_cmp before after
+'
+
 test_done
diff --git a/test-dump-split-index.c b/test-dump-split-index.c
index 9cf3112..861d28c 100644
--- a/test-dump-split-index.c
+++ b/test-dump-split-index.c
@@ -26,9 +26,11 @@ int main(int ac, char **av)
 		       sha1_to_hex(ce->sha1), ce_stage(ce), ce->name);
 	}
 	printf("replacements:");
-	ewah_each_bit(si->replace_bitmap, show_bit, NULL);
+	if (si->replace_bitmap)
+		ewah_each_bit(si->replace_bitmap, show_bit, NULL);
 	printf("\ndeletions:");
-	ewah_each_bit(si->delete_bitmap, show_bit, NULL);
+	if (si->delete_bitmap)
+		ewah_each_bit(si->delete_bitmap, show_bit, NULL);
 	printf("\n");
 	return 0;
 }
-- 
2.4.2.622.gac67c30-twtrsrc

Re: [PATCH] commit: don't rewrite shared index unnecessarily

[Duy Nguyen]

On Fri, Aug 28, 2015 at 12:07 AM, David Turner <dturner@twopensource.com> wrote:
> Remove a cache invalidation which would cause the shared index to be
> rewritten on as-is commits.
>
> When the cache-tree has changed, we need to update it.  But we don't
> necessarily need to update the shared index.  So setting
> active_cache_changed to SOMETHING_CHANGED is unnecessary.  Instead, we
> let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.
>
> In order to test this, make test-dump-split-index not segfault on
> missing replace_bitmap/delete_bitmap.  This new codepath is not called
> now that the test passes, but is necessary to avoid a segfault when the
> new test is run with the old builtin/commit.c code.
>
> Signed-off-by: David Turner <dturner@twopensource.com>

Ack.

I made SOMETHING_CHANGED "1" for catching these cases (there were a
few on-flight topics when this series was being cooked and I was
afraid I could not cache all active_cache_changed sites).

> ---
>
> I introduced this bug last year while improving the cache-tree code.
> I guess I probably didn't notice that active_cache_changed wasn't a
> boolean.

So.. you did you split-index? Cool. Never heard anyone using it for
real. It needs the other part to improve reading/refresh side to get
to full potential though..
-- 
Duy

Re: [PATCH] commit: don't rewrite shared index unnecessarily

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Aug 28, 2015 at 12:07 AM, David Turner <dturner@twopensource.com> wrote:
>> Remove a cache invalidation which would cause the shared index to be
>> rewritten on as-is commits.
>>
>> When the cache-tree has changed, we need to update it.  But we don't
>> necessarily need to update the shared index.  So setting
>> active_cache_changed to SOMETHING_CHANGED is unnecessary.  Instead, we
>> let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.
>>
>> In order to test this, make test-dump-split-index not segfault on
>> missing replace_bitmap/delete_bitmap.  This new codepath is not called
>> now that the test passes, but is necessary to avoid a segfault when the
>> new test is run with the old builtin/commit.c code.
>>
>> Signed-off-by: David Turner <dturner@twopensource.com>
>
> Ack.
>
> I made SOMETHING_CHANGED "1" for catching these cases (there were a
> few on-flight topics when this series was being cooked and I was
> afraid I could not cache all active_cache_changed sites).

Thanks.

>> ---
>>
>> I introduced this bug last year while improving the cache-tree code.
>> I guess I probably didn't notice that active_cache_changed wasn't a
>> boolean.
>
> So.. you did you split-index? Cool. Never heard anyone using it for
> real. It needs the other part to improve reading/refresh side to get
> to full potential though..

;-)

Re: [PATCH] commit: don't rewrite shared index unnecessarily

[David Turner]

On Mon, 2015-08-31 at 17:02 +0700, Duy Nguyen wrote:
> On Fri, Aug 28, 2015 at 12:07 AM, David Turner <dturner@twopensource.com> wrote:
> > Remove a cache invalidation which would cause the shared index to be
> > rewritten on as-is commits.
> >
> > When the cache-tree has changed, we need to update it.  But we don't
> > necessarily need to update the shared index.  So setting
> > active_cache_changed to SOMETHING_CHANGED is unnecessary.  Instead, we
> > let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.
> >
> > In order to test this, make test-dump-split-index not segfault on
> > missing replace_bitmap/delete_bitmap.  This new codepath is not called
> > now that the test passes, but is necessary to avoid a segfault when the
> > new test is run with the old builtin/commit.c code.
> >
> > Signed-off-by: David Turner <dturner@twopensource.com>
> 
> Ack.
> 
> I made SOMETHING_CHANGED "1" for catching these cases (there were a
> few on-flight topics when this series was being cooked and I was
> afraid I could not cache all active_cache_changed sites).
> 
> > ---
> >
> > I introduced this bug last year while improving the cache-tree code.
> > I guess I probably didn't notice that active_cache_changed wasn't a
> > boolean.
> 
> So.. you did you split-index? Cool. Never heard anyone using it for
> real. It needs the other part to improve reading/refresh side to get
> to full potential though..

I am experimenting with an "unskipped index", to reduce the cost of
index loading and processing in sparse checkouts.  The unskipped index
is an index on the (v3) index -- that is, it lists the byte offsets of
all entries in the sharedindex where CE_SKIP_WORKTREE is 0.  In
addition, there is a list of the offsets of *all* entries, which we can
binary search in.  There's also some other metadata that I'm figuring
out as I go along.  This might allow us to avoid loading the full shared
index in most cases.  Or it might be a total dead end.  Not sure yet.