{"thread":{"id":"40199","subject":"[PATCH] commit: don't rewrite shared index unnecessarily","startedAt":"2015-08-27T17:07:54Z","lastAt":"2015-08-31T18:41:31Z","messageCount":4,"participants":["David Turner","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"268802","messageId":"1440695274-12400-1-git-send-email-dturner@twopensource.com","threadId":"40199","inReplyTo":null,"subject":"[PATCH] commit: don't rewrite shared index unnecessarily","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-27T17:07:54Z","receivedAt":"2015-08-27T17:07:54Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Remove a cache invalidation which would cause the shared index to be\nrewritten on as-is commits.\n\nWhen the cache-tree has changed, we need to update it. But we don't\nnecessarily need to update the shared index. So setting\nactive_cache_changed to SOMETHING_CHANGED is unnecessary. Instead, we\nlet update_main_cache_tree just update the CACHE_TREE_CHANGED bit.\n\nIn order to test this, make test-dump-split-index not segfault on\nmissing replace_bitmap/delete_bitmap. This new codepath is not called\nnow that the test passes, but is necessary to avoid a segfault when the\nnew test is run with the old builtin/commit.c code.\n\nSigned-off-by: David Turner \n---\n\nI introduced this bug last year while improving the cache-tree code.\nI guess I probably didn't notice that active_cache_changed wasn't a\nboolean.\n\n---\n builtin/commit.c | 4 +---\n t/t0090-cache-tree.sh | 10 ++++++++++\n test-dump-split-index.c | 6 ++++--\n 3 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 254477f..1692620 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -404,10 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\thold_locked_index(&index_lock, 1);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tif (active_cache_changed\n-\t\t || !cache_tree_fully_valid(active_cache_tree)) {\n+\t\t || !cache_tree_fully_valid(active_cache_tree))\n \t\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\t\t\tactive_cache_changed = 1;\n-\t\t}\n \t\tif (active_cache_changed) {\n \t\t\tif (write_locked_index(&the_index, &index_lock,\n \t\t\t\t\t COMMIT_LOCK))\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 601d02d..f92dd1f 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -218,4 +218,14 @@ test_expect_success 'no phantom error when switching trees' '\n \t! test -s errors\n '\n \n+test_expect_success 'switching trees does not invalidate shared index' '\n+\tgit update-index --split-index &&\n+\t>split &&\n+\tgit add split &&\n+\ttest-dump-split-index .git/index | grep -v ^own >before &&\n+\tgit commit -m \"as-is\" &&\n+\ttest-dump-split-index .git/index | grep -v ^own >after &&\n+\ttest_cmp before after\n+'\n+\n test_done\ndiff --git a/test-dump-split-index.c b/test-dump-split-index.c\nindex 9cf3112..861d28c 100644\n--- a/test-dump-split-index.c\n+++ b/test-dump-split-index.c\n@@ -26,9 +26,11 @@ int main(int ac, char **av)\n \t\t sha1_to_hex(ce->sha1), ce_stage(ce), ce->name);\n \t}\n \tprintf(\"replacements:\");\n-\tewah_each_bit(si->replace_bitmap, show_bit, NULL);\n+\tif (si->replace_bitmap)\n+\t\tewah_each_bit(si->replace_bitmap, show_bit, NULL);\n \tprintf(\"\\ndeletions:\");\n-\tewah_each_bit(si->delete_bitmap, show_bit, NULL);\n+\tif (si->delete_bitmap)\n+\t\tewah_each_bit(si->delete_bitmap, show_bit, NULL);\n \tprintf(\"\\n\");\n \treturn 0;\n }\n-- \n2.4.2.622.gac67c30-twtrsrc\n"},{"id":"268975","messageId":"CACsJy8D=KyWBYf1LW=n928rmG=9xavSaR+1G1g+F7mMTKFCpfg@mail.gmail.com","threadId":"40199","inReplyTo":"1440695274-12400-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH] commit: don't rewrite shared index unnecessarily","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-31T10:02:31Z","receivedAt":"2015-08-31T10:02:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 28, 2015 at 12:07 AM, David Turner wrote:\n> Remove a cache invalidation which would cause the shared index to be\n> rewritten on as-is commits.\n>\n> When the cache-tree has changed, we need to update it. But we don't\n> necessarily need to update the shared index. So setting\n> active_cache_changed to SOMETHING_CHANGED is unnecessary. Instead, we\n> let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.\n>\n> In order to test this, make test-dump-split-index not segfault on\n> missing replace_bitmap/delete_bitmap. This new codepath is not called\n> now that the test passes, but is necessary to avoid a segfault when the\n> new test is run with the old builtin/commit.c code.\n>\n> Signed-off-by: David Turner \n\nAck.\n\nI made SOMETHING_CHANGED \"1\" for catching these cases (there were a\nfew on-flight topics when this series was being cooked and I was\nafraid I could not cache all active_cache_changed sites).\n\n> ---\n>\n> I introduced this bug last year while improving the cache-tree code.\n> I guess I probably didn't notice that active_cache_changed wasn't a\n> boolean.\n\nSo.. you did you split-index? Cool. Never heard anyone using it for\nreal. It needs the other part to improve reading/refresh side to get\nto full potential though..\n-- \nDuy\n"},{"id":"268998","messageId":"xmqqio7vxy6e.fsf@gitster.mtv.corp.google.com","threadId":"40199","inReplyTo":"CACsJy8D=KyWBYf1LW=n928rmG=9xavSaR+1G1g+F7mMTKFCpfg@mail.gmail.com","subject":"Re: [PATCH] commit: don't rewrite shared index unnecessarily","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T15:41:45Z","receivedAt":"2015-08-31T15:41:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Aug 28, 2015 at 12:07 AM, David Turner wrote:\n>> Remove a cache invalidation which would cause the shared index to be\n>> rewritten on as-is commits.\n>>\n>> When the cache-tree has changed, we need to update it. But we don't\n>> necessarily need to update the shared index. So setting\n>> active_cache_changed to SOMETHING_CHANGED is unnecessary. Instead, we\n>> let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.\n>>\n>> In order to test this, make test-dump-split-index not segfault on\n>> missing replace_bitmap/delete_bitmap. This new codepath is not called\n>> now that the test passes, but is necessary to avoid a segfault when the\n>> new test is run with the old builtin/commit.c code.\n>>\n>> Signed-off-by: David Turner \n>\n> Ack.\n>\n> I made SOMETHING_CHANGED \"1\" for catching these cases (there were a\n> few on-flight topics when this series was being cooked and I was\n> afraid I could not cache all active_cache_changed sites).\n\nThanks.\n\n>> ---\n>>\n>> I introduced this bug last year while improving the cache-tree code.\n>> I guess I probably didn't notice that active_cache_changed wasn't a\n>> boolean.\n>\n> So.. you did you split-index? Cool. Never heard anyone using it for\n> real. It needs the other part to improve reading/refresh side to get\n> to full potential though..\n\n;-)\n"},{"id":"269017","messageId":"1441046491.25570.13.camel@twopensource.com","threadId":"40199","inReplyTo":"CACsJy8D=KyWBYf1LW=n928rmG=9xavSaR+1G1g+F7mMTKFCpfg@mail.gmail.com","subject":"Re: [PATCH] commit: don't rewrite shared index unnecessarily","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-08-31T18:41:31Z","receivedAt":"2015-08-31T18:41:31Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-08-31 at 17:02 +0700, Duy Nguyen wrote:\n> On Fri, Aug 28, 2015 at 12:07 AM, David Turner wrote:\n> > Remove a cache invalidation which would cause the shared index to be\n> > rewritten on as-is commits.\n> >\n> > When the cache-tree has changed, we need to update it. But we don't\n> > necessarily need to update the shared index. So setting\n> > active_cache_changed to SOMETHING_CHANGED is unnecessary. Instead, we\n> > let update_main_cache_tree just update the CACHE_TREE_CHANGED bit.\n> >\n> > In order to test this, make test-dump-split-index not segfault on\n> > missing replace_bitmap/delete_bitmap. This new codepath is not called\n> > now that the test passes, but is necessary to avoid a segfault when the\n> > new test is run with the old builtin/commit.c code.\n> >\n> > Signed-off-by: David Turner \n> \n> Ack.\n> \n> I made SOMETHING_CHANGED \"1\" for catching these cases (there were a\n> few on-flight topics when this series was being cooked and I was\n> afraid I could not cache all active_cache_changed sites).\n> \n> > ---\n> >\n> > I introduced this bug last year while improving the cache-tree code.\n> > I guess I probably didn't notice that active_cache_changed wasn't a\n> > boolean.\n> \n> So.. you did you split-index? Cool. Never heard anyone using it for\n> real. It needs the other part to improve reading/refresh side to get\n> to full potential though..\n\nI am experimenting with an \"unskipped index\", to reduce the cost of\nindex loading and processing in sparse checkouts. The unskipped index\nis an index on the (v3) index -- that is, it lists the byte offsets of\nall entries in the sharedindex where CE_SKIP_WORKTREE is 0. In\naddition, there is a list of the offsets of *all* entries, which we can\nbinary search in. There's also some other metadata that I'm figuring\nout as I go along. This might allow us to avoid loading the full shared\nindex in most cases. Or it might be a total dead end. Not sure yet.\n"}]}