{"thread":{"id":"58771","subject":"[PATCH 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","startedAt":"2022-11-08T22:44:31Z","lastAt":"2022-11-14T00:08:44Z","messageCount":31,"participants":["Victoria Dye via GitGitGadget","Derrick Stolee","Victoria Dye","Taylor Blau","SZEDER Gábor","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"466912","messageId":"pull.1411.git.1667947465.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":null,"subject":"[PATCH 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T22:44:20Z","receivedAt":"2022-11-08T22:44:31Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Following up on a discussion [1] around cache tree refreshes in 'git reset',\nthis series updates callers of 'unpack_trees()' to skip its internal\ninvocation of 'cache_tree_update()' when 'prime_cache_tree()' is called\nimmediately after 'unpack_trees()'. 'cache_tree_update()' can be an\nexpensive operation, and it is redundant when 'prime_cache_tree()' clears\nand rebuilds the cache tree from scratch immediately after.\n\nThe first patch adds a test directly comparing the execution time of\n'prime_cache_tree()' with that of 'cache_tree_update()'. The results show\nthat on a fully-valid cache tree, they perform the same, but on a\nfully-invalid cache tree, 'prime_cache_tree()' is multiple times faster\n(although both are so fast that the total execution time of 100 invocations\nis needed to compare the results in the default perf repo).\n\nThe second patch introduces the 'skip_cache_tree_update' option for\n'unpack_trees()', but does not use it yet.\n\nThe remaining three patches update callers that make the aforementioned\nredundant cache tree updates. The performance impact on these callers ranges\nfrom \"negligible\" (in 'rebase') to \"substantial\" (in 'read-tree') - more\ndetails can be found in the commit messages of the patch associated with the\naffected code path.\n\nThanks!\n\n * Victoria\n\n[1] https://lore.kernel.org/git/xmqqlf30edvf.fsf@gitster.g/ [2]\nhttps://lore.kernel.org/git/f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com/\n\nVictoria Dye (5):\n cache-tree: add perf test comparing update and prime\n unpack-trees: add 'skip_cache_tree_update' option\n reset: use 'skip_cache_tree_update' option\n read-tree: use 'skip_cache_tree_update' option\n rebase: use 'skip_cache_tree_update' option\n\n Makefile | 1 +\n builtin/read-tree.c | 4 +++\n builtin/reset.c | 2 ++\n reset.c | 1 +\n sequencer.c | 1 +\n t/helper/test-cache-tree.c | 52 ++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/perf/p0006-read-tree-checkout.sh | 8 +++++\n t/perf/p0090-cache-tree.sh | 27 ++++++++++++++++\n t/perf/p7102-reset.sh | 21 ++++++++++++\n t/t1022-read-tree-partial-clone.sh | 2 +-\n unpack-trees.c | 3 +-\n unpack-trees.h | 3 +-\n 14 files changed, 124 insertions(+), 3 deletions(-)\n create mode 100644 t/helper/test-cache-tree.c\n create mode 100755 t/perf/p0090-cache-tree.sh\n create mode 100755 t/perf/p7102-reset.sh\n\n\nbase-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1411%2Fvdye%2Ffeature%2Fcache-tree-optimization-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1411/vdye/feature/cache-tree-optimization-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1411\n-- \ngitgitgadget\n"},{"id":"466913","messageId":"45c198c629da1627eabf0e63539f50aaa50381bf.1667947465.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"[PATCH 1/5] cache-tree: add perf test comparing update and prime","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T22:44:21Z","receivedAt":"2022-11-08T22:44:33Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a performance test comparing the execution times of 'prime_cache_tree()'\nand 'cache_tree_update(_, WRITE_TREE_SILENT | WRITE_TREE_REPAIR)'. The goal\nof comparing these two is to identify which is the faster method for\nrebuilding an invalid cache tree, ultimately to remove one when both are\n(reundantly) called in immediate succession.\n\nBoth methods are incredibly fast, so the new tests in 'p0090-cache-tree.sh'\nmust call the tested method many times in succession to get non-negligible\ntiming results. Results show a substantial difference in execution time\nbetween the two, with 'prime_cache_tree()' appearing to be the overall\nfaster method:\n\nTest this tree\n----------------------------------------------------\n0090.1: prime_cache_tree, clean 0.07(0.05+0.01)\n0090.2: cache_tree_update, clean 0.11(0.05+0.06)\n0090.3: prime_cache_tree, invalid 0.06(0.05+0.01)\n0090.4: cache_tree_update, invalid 0.50(0.41+0.07)\n\nSigned-off-by: Victoria Dye \n---\n Makefile | 1 +\n t/helper/test-cache-tree.c | 52 ++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/perf/p0090-cache-tree.sh | 27 ++++++++++++++++++++\n 5 files changed, 82 insertions(+)\n create mode 100644 t/helper/test-cache-tree.c\n create mode 100755 t/perf/p0090-cache-tree.sh\n\ndiff --git a/Makefile b/Makefile\nindex 4927379184c..3639c7c2a94 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o\n TEST_BUILTINS_OBJS += test-bitmap.o\n TEST_BUILTINS_OBJS += test-bloom.o\n TEST_BUILTINS_OBJS += test-bundle-uri.o\n+TEST_BUILTINS_OBJS += test-cache-tree.o\n TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\ndiff --git a/t/helper/test-cache-tree.c b/t/helper/test-cache-tree.c\nnew file mode 100644\nindex 00000000000..2fad6d06d30\n--- /dev/null\n+++ b/t/helper/test-cache-tree.c\n@@ -0,0 +1,52 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"tree.h\"\n+#include \"cache-tree.h\"\n+#include \"parse-options.h\"\n+\n+static char const * const test_cache_tree_usage[] = {\n+\tN_(\"test-tool cache-tree (prime|repair)\"),\n+\tNULL\n+};\n+\n+int cmd__cache_tree(int argc, const char **argv)\n+{\n+\tstruct object_id oid;\n+\tstruct tree *tree;\n+\tint fresh = 0;\n+\tint count = 1;\n+\tint i;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"fresh\", &fresh,\n+\t\t\t N_(\"clear the cache tree before each repetition\")),\n+\t\tOPT_INTEGER_F(0, \"count\", &count, N_(\"number of times to repeat the operation\"),\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_END()\n+\t};\n+\n+\tsetup_git_directory();\n+\n+\tparse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"unable to read index file\");\n+\n+\tget_oid(\"HEAD\", &oid);\n+\ttree = parse_tree_indirect(&oid);\n+\tfor (i = 0; i < count; i++) {\n+\t\tif (fresh)\n+\t\t\tcache_tree_free(&the_index.cache_tree);\n+\n+\t\tif (!argc)\n+\t\t\tdie(\"Must specify subcommand\");\n+\t\telse if (!strcmp(argv[0], \"prime\"))\n+\t\t\tprime_cache_tree(the_repository, &the_index, tree);\n+\t\telse if (!strcmp(argv[0], \"update\"))\n+\t\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t\telse\n+\t\t\tdie(\"Unknown command %s\", argv[0]);\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 01cda9358df..547a3be1c8b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -14,6 +14,7 @@ static struct test_cmd cmds[] = {\n \t{ \"bitmap\", cmd__bitmap },\n \t{ \"bloom\", cmd__bloom },\n \t{ \"bundle-uri\", cmd__bundle_uri },\n+\t{ \"cache-tree\", cmd__cache_tree },\n \t{ \"chmtime\", cmd__chmtime },\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex ca2948066fd..e44e1d896d3 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);\n int cmd__bitmap(int argc, const char **argv);\n int cmd__bloom(int argc, const char **argv);\n int cmd__bundle_uri(int argc, const char **argv);\n+int cmd__cache_tree(int argc, const char **argv);\n int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\ndiff --git a/t/perf/p0090-cache-tree.sh b/t/perf/p0090-cache-tree.sh\nnew file mode 100755\nindex 00000000000..91c13e28a27\n--- /dev/null\n+++ b/t/perf/p0090-cache-tree.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of cache tree operations\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+count=200\n+test_perf \"prime_cache_tree, clean\" \"\n+\ttest-tool cache-tree --count $count prime\n+\"\n+\n+test_perf \"cache_tree_update, clean\" \"\n+\ttest-tool cache-tree --count $count update\n+\"\n+\n+test_perf \"prime_cache_tree, invalid\" \"\n+\ttest-tool cache-tree --count $count --fresh prime\n+\"\n+\n+test_perf \"cache_tree_update, invalid\" \"\n+\ttest-tool cache-tree --count $count --fresh update\n+\"\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"466914","messageId":"908fe764670af2572a6171c163abf36f4ed8e41a.1667947465.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"[PATCH 3/5] reset: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T22:44:23Z","receivedAt":"2022-11-08T22:44:43Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable the 'skip_cache_tree_update' option in the variants that call\n'prime_cache_tree()' after 'unpack_trees()' (specifically, 'git reset\n--mixed' and 'git reset --hard'). This avoids redundantly rebuilding the\ncache tree in both 'cache_tree_update()' at the end of 'unpack_trees()' and\nin 'prime_cache_tree()', resulting in a small (but consistent) performance\nimprovement. From the newly-added 'p7102-reset.sh' test:\n\nTest before after\n--------------------------------------------------------------------\n7102.1: reset --hard (...) 2.11(0.40+1.54) 1.97(0.38+1.47) -6.6%\n\nSigned-off-by: Victoria Dye \n---\n builtin/reset.c | 2 ++\n t/perf/p7102-reset.sh | 21 +++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n create mode 100755 t/perf/p7102-reset.sh\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex fdce6f8c856..ab027774824 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -73,9 +73,11 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \tcase HARD:\n \t\topts.update = 1;\n \t\topts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n+\t\topts.skip_cache_tree_update = 1;\n \t\tbreak;\n \tcase MIXED:\n \t\topts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n+\t\topts.skip_cache_tree_update = 1;\n \t\t/* but opts.update=0, so working tree not updated */\n \t\tbreak;\n \tdefault:\ndiff --git a/t/perf/p7102-reset.sh b/t/perf/p7102-reset.sh\nnew file mode 100755\nindex 00000000000..9b039e8691f\n--- /dev/null\n+++ b/t/perf/p7102-reset.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+\n+test_description='performance of reset'\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+test_checkout_worktree\n+\n+test_perf 'reset --hard with change in tree' '\n+\tbase=$(git rev-parse HEAD) &&\n+\ttest_commit --no-tag A &&\n+\tnew=$(git rev-parse HEAD) &&\n+\n+\tfor i in $(test_seq 10)\n+\tdo\n+\t\tgit reset --hard $new &&\n+\t\tgit reset --hard $base || return $?\n+\tdone\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"466915","messageId":"d0a20cafd394165855620d76d9f5ab7c003338e6.1667947465.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"[PATCH 2/5] unpack-trees: add 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T22:44:22Z","receivedAt":"2022-11-08T22:44:45Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd (disabled by default) option to skip the 'cache_tree_update()' at the\nend of 'unpack_trees()'. In many cases, this cache tree update is redundant\nbecause the caller of 'unpack_trees()' immediately follows it with\n'prime_cache_tree()', rebuilding the entire cache tree from scratch. While\nthese operations aren't the most expensive part of operations like 'git\nreset', the duplicate calls still create a minor unnecessary slowdown.\n\nIntroduce an option for callers to skip the 'cache_tree_update()' in\n'unpack_trees()' if it is redundant (that is, if 'prime_cache_tree()' is\ncalled afterwards). At the moment, no 'unpack_trees()' callers use the new\noption; they will be updated in subsequent patches.\n\nSigned-off-by: Victoria Dye \n---\n unpack-trees.c | 3 ++-\n unpack-trees.h | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bae812156c4..8a762aa0772 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2043,7 +2043,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n \t\t\t\tcache_tree_verify(the_repository, &o->result);\n-\t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n+\t\t\tif (!o->skip_cache_tree_update &&\n+\t\t\t !cache_tree_fully_valid(o->result.cache_tree))\n \t\t\t\tcache_tree_update(&o->result,\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex efb9edfbb27..6ab0d74c84d 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -71,7 +71,8 @@ struct unpack_trees_options {\n \t\t quiet,\n \t\t exiting_early,\n \t\t show_all_errors,\n-\t\t dry_run;\n+\t\t dry_run,\n+\t\t skip_cache_tree_update;\n \tenum unpack_trees_reset_type reset;\n \tconst char *prefix;\n \tint cache_bottom;\n-- \ngitgitgadget\n\n"},{"id":"466916","messageId":"dd4edd7cad8d06b3464608cc3ce79bb0368a5e2e.1667947465.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"[PATCH 5/5] rebase: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T22:44:25Z","receivedAt":"2022-11-08T22:44:47Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable the 'skip_cache_tree_update' option in both 'do_reset()'\n('sequencer.c') and 'reset_head()' ('reset.c'). Both of these callers invoke\n'prime_cache_tree()' after 'unpack_trees()', so we can remove an unnecessary\ncache tree rebuild by skipping 'cache_tree_update()'.\n\nWhen testing with 'p3400-rebase.sh' and 'p3404-rebase-interactive.sh', the\nperformance change of this update was negligible, likely due to the\noperation being dominated by more expensive operations (like checking out\ntrees). However, since the change doesn't harm performance, it's worth\nkeeping this 'unpack_trees()' usage consistent with others that subsequently\ninvoke 'prime_cache_tree()'.\n\nSigned-off-by: Victoria Dye \n---\n reset.c | 1 +\n sequencer.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/reset.c b/reset.c\nindex e3383a93343..5ded23611f3 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -128,6 +128,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.merge = 1;\n \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n+\tunpack_tree_opts.skip_cache_tree_update = 1;\n \tinit_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);\n \tif (reset_hard)\n \t\tunpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..3f7a73ce4e1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3750,6 +3750,7 @@ static int do_reset(struct repository *r,\n \tunpack_tree_opts.merge = 1;\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n+\tunpack_tree_opts.skip_cache_tree_update = 1;\n \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n \n \tif (repo_read_index_unmerged(r)) {\n-- \ngitgitgadget\n"},{"id":"466917","messageId":"319f1d71b2e63455f319f1b460515fe0c4af33f6.1667947465.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"[PATCH 4/5] read-tree: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T22:44:24Z","receivedAt":"2022-11-08T22:44:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen running 'read-tree' with a single tree and no prefix,\n'prime_cache_tree()' is called after the tree is unpacked. In that\nsituation, skip a redundant call to 'cache_tree_update()' in\n'unpack_trees()' by enabling the 'skip_cache_tree_update' unpack option.\n\nRemoving the redundant cache tree update provides a substantial performance\nimprovement to 'git read-tree ', as shown by a test added to\n'p0006-read-tree-checkout.sh':\n\nTest before after\n----------------------------------------------------------------------\nread-tree br_ballast_plus_1 3.94(1.80+1.57) 3.00(1.14+1.28) -23.9%\n\nNote that the 'read-tree' in 't1022-read-tree-partial-clone.sh' is updated\nto read two trees, rather than one. The test was first introduced in\nd3da223f221 (cache-tree: prefetch in partial clone read-tree, 2021-07-23) to\nexercise the 'cache_tree_update()' code path, as used in 'git merge'. Since\nthis patch drops the call to 'cache_tree_update()' in single-tree 'git\nread-tree', change the test to use the two-tree variant so that\n'cache_tree_update()' is called as intended.\n\nSigned-off-by: Victoria Dye \n---\n builtin/read-tree.c | 4 ++++\n t/perf/p0006-read-tree-checkout.sh | 8 ++++++++\n t/t1022-read-tree-partial-clone.sh | 2 +-\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex f4cbe460b97..45c6652444b 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -249,6 +249,10 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tif (opts.debug_unpack)\n \t\topts.fn = debug_merge;\n \n+\t/* If we're going to prime_cache_tree later, skip cache tree update */\n+\tif (nr_trees == 1 && !opts.prefix)\n+\t\topts.skip_cache_tree_update = 1;\n+\n \tcache_tree_free(&active_cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\ndiff --git a/t/perf/p0006-read-tree-checkout.sh b/t/perf/p0006-read-tree-checkout.sh\nindex c481c012d2f..325566e18eb 100755\n--- a/t/perf/p0006-read-tree-checkout.sh\n+++ b/t/perf/p0006-read-tree-checkout.sh\n@@ -49,6 +49,14 @@ test_perf \"read-tree br_base br_ballast ($nr_files)\" '\n \tgit read-tree -n -m br_base br_ballast\n '\n \n+test_perf \"read-tree br_ballast_plus_1 ($nr_files)\" '\n+\t# Run read-tree 100 times for clearer performance results & comparisons\n+\tfor i in $(test_seq 100)\n+\tdo\n+\t\tgit read-tree -n -m br_ballast_plus_1 || return 1\n+\tdone\n+'\n+\n test_perf \"switch between br_base br_ballast ($nr_files)\" '\n \tgit checkout -q br_base &&\n \tgit checkout -q br_ballast\ndiff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh\nindex a9953b6a71c..da539716359 100755\n--- a/t/t1022-read-tree-partial-clone.sh\n+++ b/t/t1022-read-tree-partial-clone.sh\n@@ -19,7 +19,7 @@ test_expect_success 'read-tree in partial clone prefetches in one batch' '\n \tgit -C server config uploadpack.allowfilter 1 &&\n \tgit -C server config uploadpack.allowanysha1inwant 1 &&\n \tgit clone --bare --filter=blob:none \"file://$(pwd)/server\" client &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE $TREE &&\n \n \t# \"done\" marks the end of negotiation (once per fetch). Expect that\n \t# only one fetch occurs.\n-- \ngitgitgadget\n\n"},{"id":"466962","messageId":"6c1e50e3-cddb-4cc3-f83c-6ec2e2a06a9f@github.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-09T15:23:05Z","receivedAt":"2022-11-09T15:23:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/8/2022 5:44 PM, Victoria Dye via GitGitGadget wrote:\n> Following up on a discussion [1] around cache tree refreshes in 'git reset',\n> this series updates callers of 'unpack_trees()' to skip its internal\n> invocation of 'cache_tree_update()' when 'prime_cache_tree()' is called\n> immediately after 'unpack_trees()'. 'cache_tree_update()' can be an\n> expensive operation, and it is redundant when 'prime_cache_tree()' clears\n> and rebuilds the cache tree from scratch immediately after.\n> \n> The first patch adds a test directly comparing the execution time of\n> 'prime_cache_tree()' with that of 'cache_tree_update()'. The results show\n> that on a fully-valid cache tree, they perform the same, but on a\n> fully-invalid cache tree, 'prime_cache_tree()' is multiple times faster\n> (although both are so fast that the total execution time of 100 invocations\n> is needed to compare the results in the default perf repo).\n\nOne thing I found interesting is how you needed 200 iterations to show\na meaningful change in this test script, but in the case of 'git reset'\nwe can see sizeable improvements even with a single iteration.\n\nIs there something about this test that is artificially speeding up\nthese iterations? Perhaps the index has up-to-date filesystem information\nthat allows these methods to avoid filesystem interactions that are\nnecessary in the 'git reset' case?\n \n> The second patch introduces the 'skip_cache_tree_update' option for\n> 'unpack_trees()', but does not use it yet.\n> \n> The remaining three patches update callers that make the aforementioned\n> redundant cache tree updates. The performance impact on these callers ranges\n> from \"negligible\" (in 'rebase') to \"substantial\" (in 'read-tree') - more\n> details can be found in the commit messages of the patch associated with the\n> affected code path.\n\nI found these patches well motivated and the code change to be so\nunobtrusive that the benefits are well worth the new options.\n\nThanks,\n-Stolee\n"},{"id":"466994","messageId":"99c1e5e0-d5cd-cf0e-25ba-31bc96a089c6@github.com","threadId":"58771","inReplyTo":"6c1e50e3-cddb-4cc3-f83c-6ec2e2a06a9f@github.com","subject":"Re: [PATCH 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-09T22:18:33Z","receivedAt":"2022-11-09T22:19:21Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 11/8/2022 5:44 PM, Victoria Dye via GitGitGadget wrote:\n>> Following up on a discussion [1] around cache tree refreshes in 'git reset',\n>> this series updates callers of 'unpack_trees()' to skip its internal\n>> invocation of 'cache_tree_update()' when 'prime_cache_tree()' is called\n>> immediately after 'unpack_trees()'. 'cache_tree_update()' can be an\n>> expensive operation, and it is redundant when 'prime_cache_tree()' clears\n>> and rebuilds the cache tree from scratch immediately after.\n>>\n>> The first patch adds a test directly comparing the execution time of\n>> 'prime_cache_tree()' with that of 'cache_tree_update()'. The results show\n>> that on a fully-valid cache tree, they perform the same, but on a\n>> fully-invalid cache tree, 'prime_cache_tree()' is multiple times faster\n>> (although both are so fast that the total execution time of 100 invocations\n>> is needed to compare the results in the default perf repo).\n> \n> One thing I found interesting is how you needed 200 iterations to show\n> a meaningful change in this test script, but in the case of 'git reset'\n> we can see sizeable improvements even with a single iteration.\n\nAll of the new performance tests run with multiple iterations: 20 for reset\n(10 iterations of two resets each), 100 for read-tree, 200 for the\ncomparison of 'cache_tree_update()' & 'prime_cache_tree()'. Those counts\nwere picked mostly by trial-and-error, to strike a balance of \"the test\ndoesn't take too long to run\" and \"the change in execution time is clearly\nvisible in the results.\"\n\n> \n> Is there something about this test that is artificially speeding up\n> these iterations? Perhaps the index has up-to-date filesystem information\n> that allows these methods to avoid filesystem interactions that are\n> necessary in the 'git reset' case?\n\nI would expect the \"cache_tree_update, invalid\" test's execution time, when\nscaled to the iterations of 'read-tree' and 'reset', to match the change in\ntiming of those commands, but the command tests are reporting *much* larger\nimprovements (e.g., I'd expect a 0.27s improvement in 'git read-tree', but\nthe results are *consistently* >=0.9s).\n\nPer trace2 logs, a single invocation of 'read-tree' matching the one added\nin 'p0006' spent 0.010108s in 'cache_tree_update()'. Over 100 iterations,\nthe total time would be ~1.01s, which lines up with the 'p0006' test\nresults. However, the trace2 results for \"test-tool cache-tree --count 3\n--fresh --update\" show the first iteration taking 0.013060s (looks good),\nthen the next taking 0.003755s, then 0.004026s (_much_ faster than\nexpected).\n\nTo be honest, I can't figure out what's going on there. It might be some\nkind of runtime/memory optimization with repeatedly rebuilding the same\ncache tree (doesn't seem to be compiler optimization, since the speedup\nstill happens with '-O0'). The only sure-fire way to avoid it seems to be\nmoving the iteration outside of 'test-cache-tree.c' and into 'p0090'.\nUnfortunately, the command initialization overhead *really* slows things\ndown, but I can add a \"control\" test (with no cache tree refresh) to\nquantify how long that initialization takes.\n\nWhile looking into this, I found a few other things I'd like to add to/fix\nin that test (add a \"partially-invalidated\" cache tree case, use the\noriginal cache tree OID in 'prime_cache_tree()' rather than the OID at\nHEAD), so I'll re-roll with those + the updated iteration logic.\n\nThanks for bringing this up!\n\n> \n>> The second patch introduces the 'skip_cache_tree_update' option for\n>> 'unpack_trees()', but does not use it yet.\n>>\n>> The remaining three patches update callers that make the aforementioned\n>> redundant cache tree updates. The performance impact on these callers ranges\n>> from \"negligible\" (in 'rebase') to \"substantial\" (in 'read-tree') - more\n>> details can be found in the commit messages of the patch associated with the\n>> affected code path.\n> \n> I found these patches well motivated and the code change to be so\n> unobtrusive that the benefits are well worth the new options.\n\nThanks!\n\n> \n> Thanks,\n> -Stolee\n\n"},{"id":"467000","messageId":"Y2wxQdOUmuUHNec1@nand.local","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-09T23:01:21Z","receivedAt":"2022-11-09T23:02:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 08, 2022 at 10:44:20PM +0000, Victoria Dye via GitGitGadget wrote:\n> Victoria Dye (5):\n> cache-tree: add perf test comparing update and prime\n> unpack-trees: add 'skip_cache_tree_update' option\n> reset: use 'skip_cache_tree_update' option\n> read-tree: use 'skip_cache_tree_update' option\n> rebase: use 'skip_cache_tree_update' option\n\nVery cleanly done and demonstrated. I'll keep an eye out for the reroll\nthat you and Stolee discussed below.\n\n\nThanks,\nTaylor\n"},{"id":"467012","messageId":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.git.1667947465.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T01:57:12Z","receivedAt":"2022-11-10T01:58:12Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Following up on a discussion [1] around cache tree refreshes in 'git reset',\nthis series updates callers of 'unpack_trees()' to skip its internal\ninvocation of 'cache_tree_update()' when 'prime_cache_tree()' is called\nimmediately after 'unpack_trees()'. 'cache_tree_update()' can be an\nexpensive operation, and it is redundant when 'prime_cache_tree()' clears\nand rebuilds the cache tree from scratch immediately after.\n\nThe first patch adds a test directly comparing the execution time of\n'prime_cache_tree()' with that of 'cache_tree_update()'. The results show\nthat on a fully-valid cache tree, they perform the same, but on a partially-\nor fully-invalid cache tree (the more likely case in commands with the\naforementioned redundancy), 'prime_cache_tree()' is faster.\n\nThe second patch introduces the 'skip_cache_tree_update' option for\n'unpack_trees()', but does not use it yet.\n\nThe remaining three patches update callers that make the aforementioned\nredundant cache tree updates. The performance impact on these callers ranges\nfrom \"negligible\" (in 'rebase') to \"substantial\" (in 'read-tree') - more\ndetails can be found in the commit messages of the patch associated with the\naffected code path.\n\n\nChanges since V1\n================\n\n * Rewrote 'p0090' to more accurately and reliably test 'prime_cache_tree()'\n vs. 'cache_tree_update()'.\n * Moved iterative cache tree update out of C and into the shell tests (to\n avoid potential runtime optimizations)\n * Added a \"control\" test to document how much of the execution time is\n startup overhead\n * Added tests demonstrating performance in partially-invalid cache trees.\n * Fixed the use of 'prime_cache_tree()' in 'test-tool cache-tree', changing\n it from using the tree at HEAD to the current cache tree.\n\nThanks!\n\n * Victoria\n\n[1] https://lore.kernel.org/git/xmqqlf30edvf.fsf@gitster.g/ [2]\nhttps://lore.kernel.org/git/f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com/\n\nVictoria Dye (5):\n cache-tree: add perf test comparing update and prime\n unpack-trees: add 'skip_cache_tree_update' option\n reset: use 'skip_cache_tree_update' option\n read-tree: use 'skip_cache_tree_update' option\n rebase: use 'skip_cache_tree_update' option\n\n Makefile | 1 +\n builtin/read-tree.c | 4 ++\n builtin/reset.c | 2 +\n reset.c | 1 +\n sequencer.c | 1 +\n t/helper/test-cache-tree.c | 64 ++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/perf/p0006-read-tree-checkout.sh | 8 ++++\n t/perf/p0090-cache-tree.sh | 36 +++++++++++++++++\n t/perf/p7102-reset.sh | 21 ++++++++++\n t/t1022-read-tree-partial-clone.sh | 2 +-\n unpack-trees.c | 3 +-\n unpack-trees.h | 3 +-\n 14 files changed, 145 insertions(+), 3 deletions(-)\n create mode 100644 t/helper/test-cache-tree.c\n create mode 100755 t/perf/p0090-cache-tree.sh\n create mode 100755 t/perf/p7102-reset.sh\n\n\nbase-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1411%2Fvdye%2Ffeature%2Fcache-tree-optimization-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1411/vdye/feature/cache-tree-optimization-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1411\n\nRange-diff vs v1:\n\n 1: 45c198c629d ! 1: 833519d87c8 cache-tree: add perf test comparing update and prime\n @@ Commit message\n rebuilding an invalid cache tree, ultimately to remove one when both are\n (reundantly) called in immediate succession.\n \n - Both methods are incredibly fast, so the new tests in 'p0090-cache-tree.sh'\n - must call the tested method many times in succession to get non-negligible\n - timing results. Results show a substantial difference in execution time\n - between the two, with 'prime_cache_tree()' appearing to be the overall\n - faster method:\n + Both methods are fast, so the new tests in 'p0090-cache-tree.sh' must call\n + each tested function multiple times to ensure the reported times (to 0.01s\n + resolution) convey the differences between them.\n \n - Test this tree\n - ----------------------------------------------------\n - 0090.1: prime_cache_tree, clean 0.07(0.05+0.01)\n - 0090.2: cache_tree_update, clean 0.11(0.05+0.06)\n - 0090.3: prime_cache_tree, invalid 0.06(0.05+0.01)\n - 0090.4: cache_tree_update, invalid 0.50(0.41+0.07)\n + The tests compare the timing of a 'test-tool cache-tree' run as a no-op (to\n + capture a baseline for the overhead associated with running the tool),\n + 'cache_tree_update()', and 'prime_cache_tree()' on four scenarios:\n +\n + - A completely valid cache tree\n + - A cache tree with 2 invalid paths\n + - A cache tree with 50 invalid paths\n + - A completely empty cache tree\n +\n + Example results:\n +\n + Test this tree\n + -----------------------------------------------------------\n + 0090.2: no-op, clean 1.27(0.48+0.52)\n + 0090.3: prime_cache_tree, clean 2.02(0.83+0.85)\n + 0090.4: cache_tree_update, clean 1.30(0.49+0.54)\n + 0090.5: no-op, invalidate 2 1.29(0.48+0.54)\n + 0090.6: prime_cache_tree, invalidate 2 1.98(0.81+0.83)\n + 0090.7: cache_tree_update, invalidate 2 2.12(0.94+0.86)\n + 0090.8: no-op, invalidate 50 1.32(0.50+0.55)\n + 0090.9: prime_cache_tree, invalidate 50 2.10(0.86+0.89)\n + 0090.10: cache_tree_update, invalidate 50 2.35(1.14+0.90)\n + 0090.11: no-op, empty 1.33(0.50+0.54)\n + 0090.12: prime_cache_tree, empty 2.04(0.84+0.87)\n + 0090.13: cache_tree_update, empty 2.51(1.27+0.92)\n +\n + These timings show that, while 'cache_tree_update()' is faster when the\n + cache tree is completely valid, it is equal to or slower than\n + 'prime_cache_tree()' when there are any invalid paths. Since the redundant\n + calls are mostly in scenarios where the cache tree will be at least\n + partially invalid (e.g., 'git reset --hard'), 'prime_cache_tree()' will\n + likely perform better than 'cache_tree_update()' in typical cases.\n \n Signed-off-by: Victoria Dye \n \n @@ t/helper/test-cache-tree.c (new)\n +#include \"parse-options.h\"\n +\n +static char const * const test_cache_tree_usage[] = {\n -+\tN_(\"test-tool cache-tree (prime|repair)\"),\n ++\tN_(\"test-tool cache-tree (control|prime|update)\"),\n +\tNULL\n +};\n +\n @@ t/helper/test-cache-tree.c (new)\n +{\n +\tstruct object_id oid;\n +\tstruct tree *tree;\n -+\tint fresh = 0;\n -+\tint count = 1;\n ++\tint empty = 0;\n ++\tint invalidate_qty = 0;\n +\tint i;\n +\n +\tstruct option options[] = {\n -+\t\tOPT_BOOL(0, \"fresh\", &fresh,\n -+\t\t\t N_(\"clear the cache tree before each repetition\")),\n -+\t\tOPT_INTEGER_F(0, \"count\", &count, N_(\"number of times to repeat the operation\"),\n ++\t\tOPT_BOOL(0, \"empty\", &empty,\n ++\t\t\t N_(\"clear the cache tree before each iteration\")),\n ++\t\tOPT_INTEGER_F(0, \"invalidate\", &invalidate_qty,\n ++\t\t\t N_(\"number of entries in the cache tree to invalidate (default 0)\"),\n +\t\t\t PARSE_OPT_NONEG),\n +\t\tOPT_END()\n +\t};\n @@ t/helper/test-cache-tree.c (new)\n +\tif (read_cache() < 0)\n +\t\tdie(\"unable to read index file\");\n +\n -+\tget_oid(\"HEAD\", &oid);\n ++\toidcpy(&oid, &the_index.cache_tree->oid);\n +\ttree = parse_tree_indirect(&oid);\n -+\tfor (i = 0; i < count; i++) {\n -+\t\tif (fresh)\n -+\t\t\tcache_tree_free(&the_index.cache_tree);\n -+\n -+\t\tif (!argc)\n -+\t\t\tdie(\"Must specify subcommand\");\n -+\t\telse if (!strcmp(argv[0], \"prime\"))\n -+\t\t\tprime_cache_tree(the_repository, &the_index, tree);\n -+\t\telse if (!strcmp(argv[0], \"update\"))\n -+\t\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n -+\t\telse\n -+\t\t\tdie(\"Unknown command %s\", argv[0]);\n ++\tif (!tree)\n ++\t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n ++\n ++\tif (empty) {\n ++\t\t/* clear the cache tree & allocate a new one */\n ++\t\tcache_tree_free(&the_index.cache_tree);\n ++\t\tthe_index.cache_tree = cache_tree();\n ++\t} else if (invalidate_qty) {\n ++\t\t/* invalidate the specified number of unique paths */\n ++\t\tfloat f_interval = (float)the_index.cache_nr / invalidate_qty;\n ++\t\tint interval = f_interval < 1.0 ? 1 : (int)f_interval;\n ++\t\tfor (i = 0; i < invalidate_qty && i * interval < the_index.cache_nr; i++)\n ++\t\t\tcache_tree_invalidate_path(&the_index, the_index.cache[i * interval]->name);\n +\t}\n +\n ++\tif (!argc)\n ++\t\tdie(\"Must specify subcommand\");\n ++\telse if (!strcmp(argv[0], \"prime\"))\n ++\t\tprime_cache_tree(the_repository, &the_index, tree);\n ++\telse if (!strcmp(argv[0], \"update\"))\n ++\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n ++\t/* use \"control\" subcommand to specify no-op */\n ++\telse if (!!strcmp(argv[0], \"control\"))\n ++\t\tdie(\"Unknown command %s\", argv[0]);\n ++\n +\treturn 0;\n +}\n \n @@ t/perf/p0090-cache-tree.sh (new)\n @@\n +#!/bin/sh\n +\n -+test_description=\"Tests performance of cache tree operations\"\n ++test_description=\"Tests performance of cache tree update operations\"\n +\n +. ./perf-lib.sh\n +\n +test_perf_large_repo\n +test_checkout_worktree\n +\n -+count=200\n -+test_perf \"prime_cache_tree, clean\" \"\n -+\ttest-tool cache-tree --count $count prime\n -+\"\n ++count=100\n ++\n ++test_expect_success 'setup cache tree' '\n ++\tgit write-tree\n ++'\n +\n -+test_perf \"cache_tree_update, clean\" \"\n -+\ttest-tool cache-tree --count $count update\n -+\"\n ++test_cache_tree () {\n ++\ttest_perf \"$1, $3\" \"\n ++\t\tfor i in \\$(test_seq $count)\n ++\t\tdo\n ++\t\t\ttest-tool cache-tree $4 $2\n ++\t\tdone\n ++\t\"\n ++}\n +\n -+test_perf \"prime_cache_tree, invalid\" \"\n -+\ttest-tool cache-tree --count $count --fresh prime\n -+\"\n ++test_cache_tree_update_functions () {\n ++\ttest_cache_tree 'no-op' 'control' \"$1\" \"$2\"\n ++\ttest_cache_tree 'prime_cache_tree' 'prime' \"$1\" \"$2\"\n ++\ttest_cache_tree 'cache_tree_update' 'update' \"$1\" \"$2\"\n ++}\n +\n -+test_perf \"cache_tree_update, invalid\" \"\n -+\ttest-tool cache-tree --count $count --fresh update\n -+\"\n ++test_cache_tree_update_functions \"clean\" \"\"\n ++test_cache_tree_update_functions \"invalidate 2\" \"--invalidate 2\"\n ++test_cache_tree_update_functions \"invalidate 50\" \"--invalidate 50\"\n ++test_cache_tree_update_functions \"empty\" \"--empty\"\n +\n +test_done\n 2: d0a20cafd39 = 2: b015a4f531c unpack-trees: add 'skip_cache_tree_update' option\n 3: 908fe764670 = 3: 4f6039971b8 reset: use 'skip_cache_tree_update' option\n 4: 319f1d71b2e = 4: 5a646bc47c9 read-tree: use 'skip_cache_tree_update' option\n 5: dd4edd7cad8 = 5: fffe2fc17ed rebase: use 'skip_cache_tree_update' option\n\n-- \ngitgitgadget\n"},{"id":"467013","messageId":"4f6039971b87d84b1f44d8da4cd106777cc1c38f.1668045438.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] reset: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T01:57:15Z","receivedAt":"2022-11-10T01:58:15Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable the 'skip_cache_tree_update' option in the variants that call\n'prime_cache_tree()' after 'unpack_trees()' (specifically, 'git reset\n--mixed' and 'git reset --hard'). This avoids redundantly rebuilding the\ncache tree in both 'cache_tree_update()' at the end of 'unpack_trees()' and\nin 'prime_cache_tree()', resulting in a small (but consistent) performance\nimprovement. From the newly-added 'p7102-reset.sh' test:\n\nTest before after\n--------------------------------------------------------------------\n7102.1: reset --hard (...) 2.11(0.40+1.54) 1.97(0.38+1.47) -6.6%\n\nSigned-off-by: Victoria Dye \n---\n builtin/reset.c | 2 ++\n t/perf/p7102-reset.sh | 21 +++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n create mode 100755 t/perf/p7102-reset.sh\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex fdce6f8c856..ab027774824 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -73,9 +73,11 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \tcase HARD:\n \t\topts.update = 1;\n \t\topts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n+\t\topts.skip_cache_tree_update = 1;\n \t\tbreak;\n \tcase MIXED:\n \t\topts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n+\t\topts.skip_cache_tree_update = 1;\n \t\t/* but opts.update=0, so working tree not updated */\n \t\tbreak;\n \tdefault:\ndiff --git a/t/perf/p7102-reset.sh b/t/perf/p7102-reset.sh\nnew file mode 100755\nindex 00000000000..9b039e8691f\n--- /dev/null\n+++ b/t/perf/p7102-reset.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+\n+test_description='performance of reset'\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+test_checkout_worktree\n+\n+test_perf 'reset --hard with change in tree' '\n+\tbase=$(git rev-parse HEAD) &&\n+\ttest_commit --no-tag A &&\n+\tnew=$(git rev-parse HEAD) &&\n+\n+\tfor i in $(test_seq 10)\n+\tdo\n+\t\tgit reset --hard $new &&\n+\t\tgit reset --hard $base || return $?\n+\tdone\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"467014","messageId":"b015a4f531c6f8f25f08c06dcdf3a9d709a00cd3.1668045438.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] unpack-trees: add 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T01:57:14Z","receivedAt":"2022-11-10T01:58:18Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd (disabled by default) option to skip the 'cache_tree_update()' at the\nend of 'unpack_trees()'. In many cases, this cache tree update is redundant\nbecause the caller of 'unpack_trees()' immediately follows it with\n'prime_cache_tree()', rebuilding the entire cache tree from scratch. While\nthese operations aren't the most expensive part of operations like 'git\nreset', the duplicate calls still create a minor unnecessary slowdown.\n\nIntroduce an option for callers to skip the 'cache_tree_update()' in\n'unpack_trees()' if it is redundant (that is, if 'prime_cache_tree()' is\ncalled afterwards). At the moment, no 'unpack_trees()' callers use the new\noption; they will be updated in subsequent patches.\n\nSigned-off-by: Victoria Dye \n---\n unpack-trees.c | 3 ++-\n unpack-trees.h | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bae812156c4..8a762aa0772 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2043,7 +2043,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n \t\t\t\tcache_tree_verify(the_repository, &o->result);\n-\t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n+\t\t\tif (!o->skip_cache_tree_update &&\n+\t\t\t !cache_tree_fully_valid(o->result.cache_tree))\n \t\t\t\tcache_tree_update(&o->result,\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex efb9edfbb27..6ab0d74c84d 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -71,7 +71,8 @@ struct unpack_trees_options {\n \t\t quiet,\n \t\t exiting_early,\n \t\t show_all_errors,\n-\t\t dry_run;\n+\t\t dry_run,\n+\t\t skip_cache_tree_update;\n \tenum unpack_trees_reset_type reset;\n \tconst char *prefix;\n \tint cache_bottom;\n-- \ngitgitgadget\n\n"},{"id":"467015","messageId":"833519d87c843eb8147a45b1ec2c1fd3f3c21905.1668045438.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] cache-tree: add perf test comparing update and prime","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T01:57:13Z","receivedAt":"2022-11-10T01:58:22Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a performance test comparing the execution times of 'prime_cache_tree()'\nand 'cache_tree_update(_, WRITE_TREE_SILENT | WRITE_TREE_REPAIR)'. The goal\nof comparing these two is to identify which is the faster method for\nrebuilding an invalid cache tree, ultimately to remove one when both are\n(reundantly) called in immediate succession.\n\nBoth methods are fast, so the new tests in 'p0090-cache-tree.sh' must call\neach tested function multiple times to ensure the reported times (to 0.01s\nresolution) convey the differences between them.\n\nThe tests compare the timing of a 'test-tool cache-tree' run as a no-op (to\ncapture a baseline for the overhead associated with running the tool),\n'cache_tree_update()', and 'prime_cache_tree()' on four scenarios:\n\n- A completely valid cache tree\n- A cache tree with 2 invalid paths\n- A cache tree with 50 invalid paths\n- A completely empty cache tree\n\nExample results:\n\nTest this tree\n-----------------------------------------------------------\n0090.2: no-op, clean 1.27(0.48+0.52)\n0090.3: prime_cache_tree, clean 2.02(0.83+0.85)\n0090.4: cache_tree_update, clean 1.30(0.49+0.54)\n0090.5: no-op, invalidate 2 1.29(0.48+0.54)\n0090.6: prime_cache_tree, invalidate 2 1.98(0.81+0.83)\n0090.7: cache_tree_update, invalidate 2 2.12(0.94+0.86)\n0090.8: no-op, invalidate 50 1.32(0.50+0.55)\n0090.9: prime_cache_tree, invalidate 50 2.10(0.86+0.89)\n0090.10: cache_tree_update, invalidate 50 2.35(1.14+0.90)\n0090.11: no-op, empty 1.33(0.50+0.54)\n0090.12: prime_cache_tree, empty 2.04(0.84+0.87)\n0090.13: cache_tree_update, empty 2.51(1.27+0.92)\n\nThese timings show that, while 'cache_tree_update()' is faster when the\ncache tree is completely valid, it is equal to or slower than\n'prime_cache_tree()' when there are any invalid paths. Since the redundant\ncalls are mostly in scenarios where the cache tree will be at least\npartially invalid (e.g., 'git reset --hard'), 'prime_cache_tree()' will\nlikely perform better than 'cache_tree_update()' in typical cases.\n\nSigned-off-by: Victoria Dye \n---\n Makefile | 1 +\n t/helper/test-cache-tree.c | 64 ++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/perf/p0090-cache-tree.sh | 36 +++++++++++++++++++++\n 5 files changed, 103 insertions(+)\n create mode 100644 t/helper/test-cache-tree.c\n create mode 100755 t/perf/p0090-cache-tree.sh\n\ndiff --git a/Makefile b/Makefile\nindex 4927379184c..3639c7c2a94 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o\n TEST_BUILTINS_OBJS += test-bitmap.o\n TEST_BUILTINS_OBJS += test-bloom.o\n TEST_BUILTINS_OBJS += test-bundle-uri.o\n+TEST_BUILTINS_OBJS += test-cache-tree.o\n TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\ndiff --git a/t/helper/test-cache-tree.c b/t/helper/test-cache-tree.c\nnew file mode 100644\nindex 00000000000..8d06039fb5c\n--- /dev/null\n+++ b/t/helper/test-cache-tree.c\n@@ -0,0 +1,64 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"tree.h\"\n+#include \"cache-tree.h\"\n+#include \"parse-options.h\"\n+\n+static char const * const test_cache_tree_usage[] = {\n+\tN_(\"test-tool cache-tree (control|prime|update)\"),\n+\tNULL\n+};\n+\n+int cmd__cache_tree(int argc, const char **argv)\n+{\n+\tstruct object_id oid;\n+\tstruct tree *tree;\n+\tint empty = 0;\n+\tint invalidate_qty = 0;\n+\tint i;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"empty\", &empty,\n+\t\t\t N_(\"clear the cache tree before each iteration\")),\n+\t\tOPT_INTEGER_F(0, \"invalidate\", &invalidate_qty,\n+\t\t\t N_(\"number of entries in the cache tree to invalidate (default 0)\"),\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_END()\n+\t};\n+\n+\tsetup_git_directory();\n+\n+\tparse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"unable to read index file\");\n+\n+\toidcpy(&oid, &the_index.cache_tree->oid);\n+\ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n+\n+\tif (empty) {\n+\t\t/* clear the cache tree & allocate a new one */\n+\t\tcache_tree_free(&the_index.cache_tree);\n+\t\tthe_index.cache_tree = cache_tree();\n+\t} else if (invalidate_qty) {\n+\t\t/* invalidate the specified number of unique paths */\n+\t\tfloat f_interval = (float)the_index.cache_nr / invalidate_qty;\n+\t\tint interval = f_interval < 1.0 ? 1 : (int)f_interval;\n+\t\tfor (i = 0; i < invalidate_qty && i * interval < the_index.cache_nr; i++)\n+\t\t\tcache_tree_invalidate_path(&the_index, the_index.cache[i * interval]->name);\n+\t}\n+\n+\tif (!argc)\n+\t\tdie(\"Must specify subcommand\");\n+\telse if (!strcmp(argv[0], \"prime\"))\n+\t\tprime_cache_tree(the_repository, &the_index, tree);\n+\telse if (!strcmp(argv[0], \"update\"))\n+\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t/* use \"control\" subcommand to specify no-op */\n+\telse if (!!strcmp(argv[0], \"control\"))\n+\t\tdie(\"Unknown command %s\", argv[0]);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 01cda9358df..547a3be1c8b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -14,6 +14,7 @@ static struct test_cmd cmds[] = {\n \t{ \"bitmap\", cmd__bitmap },\n \t{ \"bloom\", cmd__bloom },\n \t{ \"bundle-uri\", cmd__bundle_uri },\n+\t{ \"cache-tree\", cmd__cache_tree },\n \t{ \"chmtime\", cmd__chmtime },\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex ca2948066fd..e44e1d896d3 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);\n int cmd__bitmap(int argc, const char **argv);\n int cmd__bloom(int argc, const char **argv);\n int cmd__bundle_uri(int argc, const char **argv);\n+int cmd__cache_tree(int argc, const char **argv);\n int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\ndiff --git a/t/perf/p0090-cache-tree.sh b/t/perf/p0090-cache-tree.sh\nnew file mode 100755\nindex 00000000000..a8eabca2c4d\n--- /dev/null\n+++ b/t/perf/p0090-cache-tree.sh\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of cache tree update operations\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+count=100\n+\n+test_expect_success 'setup cache tree' '\n+\tgit write-tree\n+'\n+\n+test_cache_tree () {\n+\ttest_perf \"$1, $3\" \"\n+\t\tfor i in \\$(test_seq $count)\n+\t\tdo\n+\t\t\ttest-tool cache-tree $4 $2\n+\t\tdone\n+\t\"\n+}\n+\n+test_cache_tree_update_functions () {\n+\ttest_cache_tree 'no-op' 'control' \"$1\" \"$2\"\n+\ttest_cache_tree 'prime_cache_tree' 'prime' \"$1\" \"$2\"\n+\ttest_cache_tree 'cache_tree_update' 'update' \"$1\" \"$2\"\n+}\n+\n+test_cache_tree_update_functions \"clean\" \"\"\n+test_cache_tree_update_functions \"invalidate 2\" \"--invalidate 2\"\n+test_cache_tree_update_functions \"invalidate 50\" \"--invalidate 50\"\n+test_cache_tree_update_functions \"empty\" \"--empty\"\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"467016","messageId":"fffe2fc17ed3beb05376f1377ea193199c13c657.1668045438.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] rebase: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T01:57:17Z","receivedAt":"2022-11-10T01:58:23Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable the 'skip_cache_tree_update' option in both 'do_reset()'\n('sequencer.c') and 'reset_head()' ('reset.c'). Both of these callers invoke\n'prime_cache_tree()' after 'unpack_trees()', so we can remove an unnecessary\ncache tree rebuild by skipping 'cache_tree_update()'.\n\nWhen testing with 'p3400-rebase.sh' and 'p3404-rebase-interactive.sh', the\nperformance change of this update was negligible, likely due to the\noperation being dominated by more expensive operations (like checking out\ntrees). However, since the change doesn't harm performance, it's worth\nkeeping this 'unpack_trees()' usage consistent with others that subsequently\ninvoke 'prime_cache_tree()'.\n\nSigned-off-by: Victoria Dye \n---\n reset.c | 1 +\n sequencer.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/reset.c b/reset.c\nindex e3383a93343..5ded23611f3 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -128,6 +128,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.merge = 1;\n \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n+\tunpack_tree_opts.skip_cache_tree_update = 1;\n \tinit_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);\n \tif (reset_hard)\n \t\tunpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..3f7a73ce4e1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3750,6 +3750,7 @@ static int do_reset(struct repository *r,\n \tunpack_tree_opts.merge = 1;\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n+\tunpack_tree_opts.skip_cache_tree_update = 1;\n \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n \n \tif (repo_read_index_unmerged(r)) {\n-- \ngitgitgadget\n"},{"id":"467017","messageId":"5a646bc47c911bb6b58e00574ac30afab1eec00b.1668045438.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] read-tree: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T01:57:16Z","receivedAt":"2022-11-10T01:58:25Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen running 'read-tree' with a single tree and no prefix,\n'prime_cache_tree()' is called after the tree is unpacked. In that\nsituation, skip a redundant call to 'cache_tree_update()' in\n'unpack_trees()' by enabling the 'skip_cache_tree_update' unpack option.\n\nRemoving the redundant cache tree update provides a substantial performance\nimprovement to 'git read-tree ', as shown by a test added to\n'p0006-read-tree-checkout.sh':\n\nTest before after\n----------------------------------------------------------------------\nread-tree br_ballast_plus_1 3.94(1.80+1.57) 3.00(1.14+1.28) -23.9%\n\nNote that the 'read-tree' in 't1022-read-tree-partial-clone.sh' is updated\nto read two trees, rather than one. The test was first introduced in\nd3da223f221 (cache-tree: prefetch in partial clone read-tree, 2021-07-23) to\nexercise the 'cache_tree_update()' code path, as used in 'git merge'. Since\nthis patch drops the call to 'cache_tree_update()' in single-tree 'git\nread-tree', change the test to use the two-tree variant so that\n'cache_tree_update()' is called as intended.\n\nSigned-off-by: Victoria Dye \n---\n builtin/read-tree.c | 4 ++++\n t/perf/p0006-read-tree-checkout.sh | 8 ++++++++\n t/t1022-read-tree-partial-clone.sh | 2 +-\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex f4cbe460b97..45c6652444b 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -249,6 +249,10 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tif (opts.debug_unpack)\n \t\topts.fn = debug_merge;\n \n+\t/* If we're going to prime_cache_tree later, skip cache tree update */\n+\tif (nr_trees == 1 && !opts.prefix)\n+\t\topts.skip_cache_tree_update = 1;\n+\n \tcache_tree_free(&active_cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\ndiff --git a/t/perf/p0006-read-tree-checkout.sh b/t/perf/p0006-read-tree-checkout.sh\nindex c481c012d2f..325566e18eb 100755\n--- a/t/perf/p0006-read-tree-checkout.sh\n+++ b/t/perf/p0006-read-tree-checkout.sh\n@@ -49,6 +49,14 @@ test_perf \"read-tree br_base br_ballast ($nr_files)\" '\n \tgit read-tree -n -m br_base br_ballast\n '\n \n+test_perf \"read-tree br_ballast_plus_1 ($nr_files)\" '\n+\t# Run read-tree 100 times for clearer performance results & comparisons\n+\tfor i in $(test_seq 100)\n+\tdo\n+\t\tgit read-tree -n -m br_ballast_plus_1 || return 1\n+\tdone\n+'\n+\n test_perf \"switch between br_base br_ballast ($nr_files)\" '\n \tgit checkout -q br_base &&\n \tgit checkout -q br_ballast\ndiff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh\nindex a9953b6a71c..da539716359 100755\n--- a/t/t1022-read-tree-partial-clone.sh\n+++ b/t/t1022-read-tree-partial-clone.sh\n@@ -19,7 +19,7 @@ test_expect_success 'read-tree in partial clone prefetches in one batch' '\n \tgit -C server config uploadpack.allowfilter 1 &&\n \tgit -C server config uploadpack.allowanysha1inwant 1 &&\n \tgit clone --bare --filter=blob:none \"file://$(pwd)/server\" client &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE $TREE &&\n \n \t# \"done\" marks the end of negotiation (once per fetch). Expect that\n \t# only one fetch occurs.\n-- \ngitgitgadget\n\n"},{"id":"467018","messageId":"Y2xeJmkMVnn0tk5V@nand.local","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-10T02:12:54Z","receivedAt":"2022-11-10T02:13:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 01:57:12AM +0000, Victoria Dye via GitGitGadget wrote:\n> Changes since V1\n> ================\n>\n> * Rewrote 'p0090' to more accurately and reliably test 'prime_cache_tree()'\n> vs. 'cache_tree_update()'.\n> * Moved iterative cache tree update out of C and into the shell tests (to\n> avoid potential runtime optimizations)\n> * Added a \"control\" test to document how much of the execution time is\n> startup overhead\n> * Added tests demonstrating performance in partially-invalid cache trees.\n> * Fixed the use of 'prime_cache_tree()' in 'test-tool cache-tree', changing\n> it from using the tree at HEAD to the current cache tree.\n\nAll seem very reasonable to me, and the range-diff matches what you say.\n\nLet's hear from Stolee, who reviewed the first round, too, and then we\nshould feel comfortable to start merging this down.\n\n\nThanks,\nTaylor\n"},{"id":"467038","messageId":"20221110072342.GA1159673@szeder.dev","threadId":"58771","inReplyTo":"45c198c629da1627eabf0e63539f50aaa50381bf.1667947465.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] cache-tree: add perf test comparing update and prime","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-10T07:23:42Z","receivedAt":"2022-11-10T07:23:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 08, 2022 at 10:44:21PM +0000, Victoria Dye via GitGitGadget wrote:\n> diff --git a/t/helper/test-cache-tree.c b/t/helper/test-cache-tree.c\n> new file mode 100644\n> index 00000000000..2fad6d06d30\n> --- /dev/null\n> +++ b/t/helper/test-cache-tree.c\n> @@ -0,0 +1,52 @@\n> +#include \"test-tool.h\"\n> +#include \"cache.h\"\n> +#include \"tree.h\"\n> +#include \"cache-tree.h\"\n> +#include \"parse-options.h\"\n> +\n> +static char const * const test_cache_tree_usage[] = {\n> +\tN_(\"test-tool cache-tree (prime|repair)\"),\n\nThe code looking at 'argv[0]' below only handles \"prime\" and \"update\",\nbut not \"repair\".\n\n> +\tNULL\n> +};\n> +\n> +int cmd__cache_tree(int argc, const char **argv)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct tree *tree;\n> +\tint fresh = 0;\n> +\tint count = 1;\n> +\tint i;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"fresh\", &fresh,\n> +\t\t\t N_(\"clear the cache tree before each repetition\")),\n> +\t\tOPT_INTEGER_F(0, \"count\", &count, N_(\"number of times to repeat the operation\"),\n> +\t\t\t PARSE_OPT_NONEG),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tsetup_git_directory();\n> +\n> +\tparse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);\n\nHere 'argc' must be updated with the return value of parse_options(),\notherwise the 'if (!argc)' condition doesn't catch what it's supposed\nto, and the subsequent 'else if' segfaults when passes the NULL\nargv[0] to strcmp().\n\n> +\n> +\tif (read_cache() < 0)\n> +\t\tdie(\"unable to read index file\");\n> +\n> +\tget_oid(\"HEAD\", &oid);\n> +\ttree = parse_tree_indirect(&oid);\n> +\tfor (i = 0; i < count; i++) {\n> +\t\tif (fresh)\n> +\t\t\tcache_tree_free(&the_index.cache_tree);\n> +\n> +\t\tif (!argc)\n\nWhat if argc > 1?\n\n> +\t\t\tdie(\"Must specify subcommand\");\n\nI think it would be nice to show usage here ...\n\n> +\t\telse if (!strcmp(argv[0], \"prime\"))\n> +\t\t\tprime_cache_tree(the_repository, &the_index, tree);\n> +\t\telse if (!strcmp(argv[0], \"update\"))\n> +\t\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> +\t\telse\n> +\t\t\tdie(\"Unknown command %s\", argv[0]);\n\n... and here as well.\n\n> +\t}\n> +\n> +\treturn 0;\n> +}\n"},{"id":"467046","messageId":"44b0331a-17e5-1528-2249-e89f0bdd6ffb@dunelm.org.uk","threadId":"58771","inReplyTo":"fffe2fc17ed3beb05376f1377ea193199c13c657.1668045438.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] rebase: use 'skip_cache_tree_update' option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-10T14:40:22Z","receivedAt":"2022-11-10T14:40:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nOn 10/11/2022 01:57, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Enable the 'skip_cache_tree_update' option in both 'do_reset()'\n> ('sequencer.c') and 'reset_head()' ('reset.c'). Both of these callers invoke\n> 'prime_cache_tree()' after 'unpack_trees()', so we can remove an unnecessary\n> cache tree rebuild by skipping 'cache_tree_update()'.\n> \n> When testing with 'p3400-rebase.sh' and 'p3404-rebase-interactive.sh', the\n> performance change of this update was negligible, likely due to the\n> operation being dominated by more expensive operations (like checking out\n> trees).\n\nYes, we only call this once at the beginning of the rebase and then for \nany reset commands and the run time will be dominated by picking commits.\n\n> However, since the change doesn't harm performance, it's worth\n> keeping this 'unpack_trees()' usage consistent with others that subsequently\n> invoke 'prime_cache_tree()'.\n\nThat makes sense\n\n> Signed-off-by: Victoria Dye \n> ---\n> reset.c | 1 +\n> sequencer.c | 1 +\n> 2 files changed, 2 insertions(+)\n> \n> diff --git a/reset.c b/reset.c\n> index e3383a93343..5ded23611f3 100644\n> --- a/reset.c\n> +++ b/reset.c\n> @@ -128,6 +128,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n\tunpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;\n> \tunpack_tree_opts.update = 1;\n> \tunpack_tree_opts.merge = 1;\n> \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n> +\tunpack_tree_opts.skip_cache_tree_update = 1;\n\nI've added an extra context line above to show that we do either a \none-way or two-way merge - is it safe to skip the cache_tree_update for \nthe two-way merge? (I'm afraid I seem to have forgotten everything I \nlearnt about prime_cache_tree() and cache_tree_update() when we \ndiscussed this optimization before).\n\nBest Wishes\n\nPhillip\n\n> \tinit_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);\n> \tif (reset_hard)\n> \t\tunpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n> diff --git a/sequencer.c b/sequencer.c\n> index e658df7e8ff..3f7a73ce4e1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3750,6 +3750,7 @@ static int do_reset(struct repository *r,\n> \tunpack_tree_opts.merge = 1;\n> \tunpack_tree_opts.update = 1;\n> \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n> +\tunpack_tree_opts.skip_cache_tree_update = 1;\n> \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n> \n> \tif (repo_read_index_unmerged(r)) {\n"},{"id":"467047","messageId":"acc2a6d9-16aa-2576-d9cb-ca75fd94a2fa@github.com","threadId":"58771","inReplyTo":"99c1e5e0-d5cd-cf0e-25ba-31bc96a089c6@github.com","subject":"Re: [PATCH 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-10T14:44:36Z","receivedAt":"2022-11-10T14:44:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/9/2022 5:18 PM, Victoria Dye wrote:\n> Derrick Stolee wrote:\n>> On 11/8/2022 5:44 PM, Victoria Dye via GitGitGadget wrote:\n>>> Following up on a discussion [1] around cache tree refreshes in 'git reset',\n>>> this series updates callers of 'unpack_trees()' to skip its internal\n>>> invocation of 'cache_tree_update()' when 'prime_cache_tree()' is called\n>>> immediately after 'unpack_trees()'. 'cache_tree_update()' can be an\n>>> expensive operation, and it is redundant when 'prime_cache_tree()' clears\n>>> and rebuilds the cache tree from scratch immediately after.\n>>>\n>>> The first patch adds a test directly comparing the execution time of\n>>> 'prime_cache_tree()' with that of 'cache_tree_update()'. The results show\n>>> that on a fully-valid cache tree, they perform the same, but on a\n>>> fully-invalid cache tree, 'prime_cache_tree()' is multiple times faster\n>>> (although both are so fast that the total execution time of 100 invocations\n>>> is needed to compare the results in the default perf repo).\n>>\n>> One thing I found interesting is how you needed 200 iterations to show\n>> a meaningful change in this test script, but in the case of 'git reset'\n>> we can see sizeable improvements even with a single iteration.\n> \n> All of the new performance tests run with multiple iterations: 20 for reset\n> (10 iterations of two resets each), 100 for read-tree, 200 for the\n> comparison of 'cache_tree_update()' & 'prime_cache_tree()'. Those counts\n> were picked mostly by trial-and-error, to strike a balance of \"the test\n> doesn't take too long to run\" and \"the change in execution time is clearly\n> visible in the results.\"\n\nThanks for pointing out my misunderstanding. I missed the repeat counts\nbecause 2-3 seconds \"seemed right\" based on performance tests of large\nmonorepos, but clearly that's not right when using the Git repository for\nperformance tests.\n>> Is there something about this test that is artificially speeding up\n>> these iterations? Perhaps the index has up-to-date filesystem information\n>> that allows these methods to avoid filesystem interactions that are\n>> necessary in the 'git reset' case?\n> \n> I would expect the \"cache_tree_update, invalid\" test's execution time, when\n> scaled to the iterations of 'read-tree' and 'reset', to match the change in\n> timing of those commands, but the command tests are reporting *much* larger\n> improvements (e.g., I'd expect a 0.27s improvement in 'git read-tree', but\n> the results are *consistently* >=0.9s).\n> \n> Per trace2 logs, a single invocation of 'read-tree' matching the one added\n> in 'p0006' spent 0.010108s in 'cache_tree_update()'. Over 100 iterations,\n> the total time would be ~1.01s, which lines up with the 'p0006' test\n> results. However, the trace2 results for \"test-tool cache-tree --count 3\n> --fresh --update\" show the first iteration taking 0.013060s (looks good),\n> then the next taking 0.003755s, then 0.004026s (_much_ faster than\n> expected).\n> \n> To be honest, I can't figure out what's going on there. It might be some\n> kind of runtime/memory optimization with repeatedly rebuilding the same\n> cache tree (doesn't seem to be compiler optimization, since the speedup\n> still happens with '-O0'). The only sure-fire way to avoid it seems to be\n> moving the iteration outside of 'test-cache-tree.c' and into 'p0090'.\n> Unfortunately, the command initialization overhead *really* slows things\n> down, but I can add a \"control\" test (with no cache tree refresh) to\n> quantify how long that initialization takes.\n\nGetting unit-level performance tests is always tricky. Sometimes the best\nway to do it is to collect a sample using GIT_TRACE2_PERF and then manually\ncollect the region times. It could be a fun project to integrate region\nmeasurements into the performance test suite instead of only end-to-end\ntimings.\n \n> While looking into this, I found a few other things I'd like to add to/fix\n> in that test (add a \"partially-invalidated\" cache tree case, use the\n> original cache tree OID in 'prime_cache_tree()' rather than the OID at\n> HEAD), so I'll re-roll with those + the updated iteration logic.\n\nTaking a look now. Thanks!\n\n-Stolee\n"},{"id":"467071","messageId":"52f94376-bd05-9470-3804-4fcbc5751d18@github.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-10T17:26:31Z","receivedAt":"2022-11-10T17:26:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/9/2022 8:57 PM, Victoria Dye via GitGitGadget wrote:\n> Changes since V1\n> ================\n> \n> * Rewrote 'p0090' to more accurately and reliably test 'prime_cache_tree()'\n> vs. 'cache_tree_update()'.\n> * Moved iterative cache tree update out of C and into the shell tests (to\n> avoid potential runtime optimizations)\n> * Added a \"control\" test to document how much of the execution time is\n> startup overhead\n> * Added tests demonstrating performance in partially-invalid cache trees.\n> * Fixed the use of 'prime_cache_tree()' in 'test-tool cache-tree', changing\n> it from using the tree at HEAD to the current cache tree.\n\nI did a re-read of this series and it looks good to me.\n\nThanks for doing this investigation!\n-Stolee\n"},{"id":"467075","messageId":"85230269-2473-2c6a-45a3-59b2b2ed4e3b@github.com","threadId":"58771","inReplyTo":"44b0331a-17e5-1528-2249-e89f0bdd6ffb@dunelm.org.uk","subject":"Re: [PATCH v2 5/5] rebase: use 'skip_cache_tree_update' option","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-10T18:19:57Z","receivedAt":"2022-11-10T18:21:03Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Phillip Wood wrote:\n> Hi Victoria\n> \n> On 10/11/2022 01:57, Victoria Dye via GitGitGadget wrote:\n>> Signed-off-by: Victoria Dye \n>> ---\n>>   reset.c     | 1 +\n>>   sequencer.c | 1 +\n>>   2 files changed, 2 insertions(+)\n>>\n>> diff --git a/reset.c b/reset.c\n>> index e3383a93343..5ded23611f3 100644\n>> --- a/reset.c\n>> +++ b/reset.c\n>> @@ -128,6 +128,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n>> unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;\n>>       unpack_tree_opts.update = 1;\n>>       unpack_tree_opts.merge = 1;\n>>       unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n>> +     unpack_tree_opts.skip_cache_tree_update = 1;\n> \n> I've added an extra context line above to show that we do either a one-way\n> or two-way merge - is it safe to skip the cache_tree_update for the\n> two-way merge? (I'm afraid I seem to have forgotten everything I learnt\n> about prime_cache_tree() and cache_tree_update() when we discussed this\n> optimization before).\n\nYes - 'prime_cache_tree()' is called immediately after 'unpack_trees()' in\nboth the one-way and two-way merge cases. Because 'prime_cache_tree()'\nunconditionally clears the cache tree and rebuilds it from scratch,\nrepairing the cache tree with 'cache_tree_update()' at the end of\n'unpack_trees()' is unnecessary.\n"},{"id":"467076","messageId":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v2.git.1668045438.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T19:06:00Z","receivedAt":"2022-11-10T19:07:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Following up on a discussion [1] around cache tree refreshes in 'git reset',\nthis series updates callers of 'unpack_trees()' to skip its internal\ninvocation of 'cache_tree_update()' when 'prime_cache_tree()' is called\nimmediately after 'unpack_trees()'. 'cache_tree_update()' can be an\nexpensive operation, and it is redundant when 'prime_cache_tree()' clears\nand rebuilds the cache tree from scratch immediately after.\n\nThe first patch adds a test directly comparing the execution time of\n'prime_cache_tree()' with that of 'cache_tree_update()'. The results show\nthat on a fully-valid cache tree, they perform the same, but on a partially-\nor fully-invalid cache tree (the more likely case in commands with the\naforementioned redundancy), 'prime_cache_tree()' is faster.\n\nThe second patch introduces the 'skip_cache_tree_update' option for\n'unpack_trees()', but does not use it yet.\n\nThe remaining three patches update callers that make the aforementioned\nredundant cache tree updates. The performance impact on these callers ranges\nfrom \"negligible\" (in 'rebase') to \"substantial\" (in 'read-tree') - more\ndetails can be found in the commit messages of the patch associated with the\naffected code path.\n\n\nChanges since V2\n================\n\n * Cleaned up option handling & provided more informative error messages in\n 'test-tool cache-tree'. The changes don't affect any behavior in the\n added tests & 'test-tool cache-tree' won't be used outside of\n development, but the improvements here will help future readers avoid\n propagating error-prone implementations.\n * Note that the suggestion to change the \"unknown subcommand\" error to a\n 'usage()' error was not taken, as it would be somewhat cumbersome to\n use a formatted string with it. This is in line with other custom\n subcommand parsing in Git, such as in 'fsmonitor--daemon.c'.\n\n\nChanges since V1\n================\n\n * Rewrote 'p0090' to more accurately and reliably test 'prime_cache_tree()'\n vs. 'cache_tree_update()'.\n * Moved iterative cache tree update out of C and into the shell tests (to\n avoid potential runtime optimizations)\n * Added a \"control\" test to document how much of the execution time is\n startup overhead\n * Added tests demonstrating performance in partially-invalid cache trees.\n * Fixed the use of 'prime_cache_tree()' in 'test-tool cache-tree', changing\n it from using the tree at HEAD to the current cache tree.\n\nThanks!\n\n * Victoria\n\n[1] https://lore.kernel.org/git/xmqqlf30edvf.fsf@gitster.g/ [2]\nhttps://lore.kernel.org/git/f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com/\n\nVictoria Dye (5):\n cache-tree: add perf test comparing update and prime\n unpack-trees: add 'skip_cache_tree_update' option\n reset: use 'skip_cache_tree_update' option\n read-tree: use 'skip_cache_tree_update' option\n rebase: use 'skip_cache_tree_update' option\n\n Makefile | 1 +\n builtin/read-tree.c | 4 ++\n builtin/reset.c | 2 +\n reset.c | 1 +\n sequencer.c | 1 +\n t/helper/test-cache-tree.c | 64 ++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/perf/p0006-read-tree-checkout.sh | 8 ++++\n t/perf/p0090-cache-tree.sh | 36 +++++++++++++++++\n t/perf/p7102-reset.sh | 21 ++++++++++\n t/t1022-read-tree-partial-clone.sh | 2 +-\n unpack-trees.c | 3 +-\n unpack-trees.h | 3 +-\n 14 files changed, 145 insertions(+), 3 deletions(-)\n create mode 100644 t/helper/test-cache-tree.c\n create mode 100755 t/perf/p0090-cache-tree.sh\n create mode 100755 t/perf/p7102-reset.sh\n\n\nbase-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1411%2Fvdye%2Ffeature%2Fcache-tree-optimization-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1411/vdye/feature/cache-tree-optimization-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1411\n\nRange-diff vs v2:\n\n 1: 833519d87c8 ! 1: 2b48a684156 cache-tree: add perf test comparing update and prime\n @@ Commit message\n partially invalid (e.g., 'git reset --hard'), 'prime_cache_tree()' will\n likely perform better than 'cache_tree_update()' in typical cases.\n \n + Helped-by: SZEDER Gábor \n Signed-off-by: Victoria Dye \n \n ## Makefile ##\n @@ t/helper/test-cache-tree.c (new)\n +\n +\tsetup_git_directory();\n +\n -+\tparse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);\n ++\targc = parse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);\n +\n +\tif (read_cache() < 0)\n -+\t\tdie(\"unable to read index file\");\n ++\t\tdie(_(\"unable to read index file\"));\n +\n +\toidcpy(&oid, &the_index.cache_tree->oid);\n +\ttree = parse_tree_indirect(&oid);\n @@ t/helper/test-cache-tree.c (new)\n +\t\t\tcache_tree_invalidate_path(&the_index, the_index.cache[i * interval]->name);\n +\t}\n +\n -+\tif (!argc)\n -+\t\tdie(\"Must specify subcommand\");\n ++\tif (argc != 1)\n ++\t\tusage_with_options(test_cache_tree_usage, options);\n +\telse if (!strcmp(argv[0], \"prime\"))\n +\t\tprime_cache_tree(the_repository, &the_index, tree);\n +\telse if (!strcmp(argv[0], \"update\"))\n +\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n +\t/* use \"control\" subcommand to specify no-op */\n +\telse if (!!strcmp(argv[0], \"control\"))\n -+\t\tdie(\"Unknown command %s\", argv[0]);\n ++\t\tdie(_(\"Unhandled subcommand '%s'\"), argv[0]);\n +\n +\treturn 0;\n +}\n 2: b015a4f531c = 2: 0e03614f0fd unpack-trees: add 'skip_cache_tree_update' option\n 3: 4f6039971b8 = 3: 386f18ca36a reset: use 'skip_cache_tree_update' option\n 4: 5a646bc47c9 = 4: ea5c82ce992 read-tree: use 'skip_cache_tree_update' option\n 5: fffe2fc17ed = 5: 100c01e936c rebase: use 'skip_cache_tree_update' option\n\n-- \ngitgitgadget\n"},{"id":"467077","messageId":"2b48a6841561c70221343e58251746a052957377.1668107165.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] cache-tree: add perf test comparing update and prime","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T19:06:01Z","receivedAt":"2022-11-10T19:07:47Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a performance test comparing the execution times of 'prime_cache_tree()'\nand 'cache_tree_update(_, WRITE_TREE_SILENT | WRITE_TREE_REPAIR)'. The goal\nof comparing these two is to identify which is the faster method for\nrebuilding an invalid cache tree, ultimately to remove one when both are\n(reundantly) called in immediate succession.\n\nBoth methods are fast, so the new tests in 'p0090-cache-tree.sh' must call\neach tested function multiple times to ensure the reported times (to 0.01s\nresolution) convey the differences between them.\n\nThe tests compare the timing of a 'test-tool cache-tree' run as a no-op (to\ncapture a baseline for the overhead associated with running the tool),\n'cache_tree_update()', and 'prime_cache_tree()' on four scenarios:\n\n- A completely valid cache tree\n- A cache tree with 2 invalid paths\n- A cache tree with 50 invalid paths\n- A completely empty cache tree\n\nExample results:\n\nTest this tree\n-----------------------------------------------------------\n0090.2: no-op, clean 1.27(0.48+0.52)\n0090.3: prime_cache_tree, clean 2.02(0.83+0.85)\n0090.4: cache_tree_update, clean 1.30(0.49+0.54)\n0090.5: no-op, invalidate 2 1.29(0.48+0.54)\n0090.6: prime_cache_tree, invalidate 2 1.98(0.81+0.83)\n0090.7: cache_tree_update, invalidate 2 2.12(0.94+0.86)\n0090.8: no-op, invalidate 50 1.32(0.50+0.55)\n0090.9: prime_cache_tree, invalidate 50 2.10(0.86+0.89)\n0090.10: cache_tree_update, invalidate 50 2.35(1.14+0.90)\n0090.11: no-op, empty 1.33(0.50+0.54)\n0090.12: prime_cache_tree, empty 2.04(0.84+0.87)\n0090.13: cache_tree_update, empty 2.51(1.27+0.92)\n\nThese timings show that, while 'cache_tree_update()' is faster when the\ncache tree is completely valid, it is equal to or slower than\n'prime_cache_tree()' when there are any invalid paths. Since the redundant\ncalls are mostly in scenarios where the cache tree will be at least\npartially invalid (e.g., 'git reset --hard'), 'prime_cache_tree()' will\nlikely perform better than 'cache_tree_update()' in typical cases.\n\nHelped-by: SZEDER Gábor \nSigned-off-by: Victoria Dye \n---\n Makefile | 1 +\n t/helper/test-cache-tree.c | 64 ++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/perf/p0090-cache-tree.sh | 36 +++++++++++++++++++++\n 5 files changed, 103 insertions(+)\n create mode 100644 t/helper/test-cache-tree.c\n create mode 100755 t/perf/p0090-cache-tree.sh\n\ndiff --git a/Makefile b/Makefile\nindex 4927379184c..3639c7c2a94 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-advise.o\n TEST_BUILTINS_OBJS += test-bitmap.o\n TEST_BUILTINS_OBJS += test-bloom.o\n TEST_BUILTINS_OBJS += test-bundle-uri.o\n+TEST_BUILTINS_OBJS += test-cache-tree.o\n TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\ndiff --git a/t/helper/test-cache-tree.c b/t/helper/test-cache-tree.c\nnew file mode 100644\nindex 00000000000..93051b25f56\n--- /dev/null\n+++ b/t/helper/test-cache-tree.c\n@@ -0,0 +1,64 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"tree.h\"\n+#include \"cache-tree.h\"\n+#include \"parse-options.h\"\n+\n+static char const * const test_cache_tree_usage[] = {\n+\tN_(\"test-tool cache-tree (control|prime|update)\"),\n+\tNULL\n+};\n+\n+int cmd__cache_tree(int argc, const char **argv)\n+{\n+\tstruct object_id oid;\n+\tstruct tree *tree;\n+\tint empty = 0;\n+\tint invalidate_qty = 0;\n+\tint i;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"empty\", &empty,\n+\t\t\t N_(\"clear the cache tree before each iteration\")),\n+\t\tOPT_INTEGER_F(0, \"invalidate\", &invalidate_qty,\n+\t\t\t N_(\"number of entries in the cache tree to invalidate (default 0)\"),\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_END()\n+\t};\n+\n+\tsetup_git_directory();\n+\n+\targc = parse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"unable to read index file\"));\n+\n+\toidcpy(&oid, &the_index.cache_tree->oid);\n+\ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n+\n+\tif (empty) {\n+\t\t/* clear the cache tree & allocate a new one */\n+\t\tcache_tree_free(&the_index.cache_tree);\n+\t\tthe_index.cache_tree = cache_tree();\n+\t} else if (invalidate_qty) {\n+\t\t/* invalidate the specified number of unique paths */\n+\t\tfloat f_interval = (float)the_index.cache_nr / invalidate_qty;\n+\t\tint interval = f_interval < 1.0 ? 1 : (int)f_interval;\n+\t\tfor (i = 0; i < invalidate_qty && i * interval < the_index.cache_nr; i++)\n+\t\t\tcache_tree_invalidate_path(&the_index, the_index.cache[i * interval]->name);\n+\t}\n+\n+\tif (argc != 1)\n+\t\tusage_with_options(test_cache_tree_usage, options);\n+\telse if (!strcmp(argv[0], \"prime\"))\n+\t\tprime_cache_tree(the_repository, &the_index, tree);\n+\telse if (!strcmp(argv[0], \"update\"))\n+\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t/* use \"control\" subcommand to specify no-op */\n+\telse if (!!strcmp(argv[0], \"control\"))\n+\t\tdie(_(\"Unhandled subcommand '%s'\"), argv[0]);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 01cda9358df..547a3be1c8b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -14,6 +14,7 @@ static struct test_cmd cmds[] = {\n \t{ \"bitmap\", cmd__bitmap },\n \t{ \"bloom\", cmd__bloom },\n \t{ \"bundle-uri\", cmd__bundle_uri },\n+\t{ \"cache-tree\", cmd__cache_tree },\n \t{ \"chmtime\", cmd__chmtime },\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex ca2948066fd..e44e1d896d3 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -8,6 +8,7 @@ int cmd__advise_if_enabled(int argc, const char **argv);\n int cmd__bitmap(int argc, const char **argv);\n int cmd__bloom(int argc, const char **argv);\n int cmd__bundle_uri(int argc, const char **argv);\n+int cmd__cache_tree(int argc, const char **argv);\n int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\ndiff --git a/t/perf/p0090-cache-tree.sh b/t/perf/p0090-cache-tree.sh\nnew file mode 100755\nindex 00000000000..a8eabca2c4d\n--- /dev/null\n+++ b/t/perf/p0090-cache-tree.sh\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of cache tree update operations\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+count=100\n+\n+test_expect_success 'setup cache tree' '\n+\tgit write-tree\n+'\n+\n+test_cache_tree () {\n+\ttest_perf \"$1, $3\" \"\n+\t\tfor i in \\$(test_seq $count)\n+\t\tdo\n+\t\t\ttest-tool cache-tree $4 $2\n+\t\tdone\n+\t\"\n+}\n+\n+test_cache_tree_update_functions () {\n+\ttest_cache_tree 'no-op' 'control' \"$1\" \"$2\"\n+\ttest_cache_tree 'prime_cache_tree' 'prime' \"$1\" \"$2\"\n+\ttest_cache_tree 'cache_tree_update' 'update' \"$1\" \"$2\"\n+}\n+\n+test_cache_tree_update_functions \"clean\" \"\"\n+test_cache_tree_update_functions \"invalidate 2\" \"--invalidate 2\"\n+test_cache_tree_update_functions \"invalidate 50\" \"--invalidate 50\"\n+test_cache_tree_update_functions \"empty\" \"--empty\"\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"467078","messageId":"0e03614f0fd7dd717b21ff9395345c06ecdcee04.1668107165.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] unpack-trees: add 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T19:06:02Z","receivedAt":"2022-11-10T19:07:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd (disabled by default) option to skip the 'cache_tree_update()' at the\nend of 'unpack_trees()'. In many cases, this cache tree update is redundant\nbecause the caller of 'unpack_trees()' immediately follows it with\n'prime_cache_tree()', rebuilding the entire cache tree from scratch. While\nthese operations aren't the most expensive part of operations like 'git\nreset', the duplicate calls still create a minor unnecessary slowdown.\n\nIntroduce an option for callers to skip the 'cache_tree_update()' in\n'unpack_trees()' if it is redundant (that is, if 'prime_cache_tree()' is\ncalled afterwards). At the moment, no 'unpack_trees()' callers use the new\noption; they will be updated in subsequent patches.\n\nSigned-off-by: Victoria Dye \n---\n unpack-trees.c | 3 ++-\n unpack-trees.h | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bae812156c4..8a762aa0772 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2043,7 +2043,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n \t\t\t\tcache_tree_verify(the_repository, &o->result);\n-\t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n+\t\t\tif (!o->skip_cache_tree_update &&\n+\t\t\t !cache_tree_fully_valid(o->result.cache_tree))\n \t\t\t\tcache_tree_update(&o->result,\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex efb9edfbb27..6ab0d74c84d 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -71,7 +71,8 @@ struct unpack_trees_options {\n \t\t quiet,\n \t\t exiting_early,\n \t\t show_all_errors,\n-\t\t dry_run;\n+\t\t dry_run,\n+\t\t skip_cache_tree_update;\n \tenum unpack_trees_reset_type reset;\n \tconst char *prefix;\n \tint cache_bottom;\n-- \ngitgitgadget\n\n"},{"id":"467079","messageId":"386f18ca36a9ffc15917c8bcc877769c7c1db7be.1668107165.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] reset: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T19:06:03Z","receivedAt":"2022-11-10T19:08:02Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable the 'skip_cache_tree_update' option in the variants that call\n'prime_cache_tree()' after 'unpack_trees()' (specifically, 'git reset\n--mixed' and 'git reset --hard'). This avoids redundantly rebuilding the\ncache tree in both 'cache_tree_update()' at the end of 'unpack_trees()' and\nin 'prime_cache_tree()', resulting in a small (but consistent) performance\nimprovement. From the newly-added 'p7102-reset.sh' test:\n\nTest before after\n--------------------------------------------------------------------\n7102.1: reset --hard (...) 2.11(0.40+1.54) 1.97(0.38+1.47) -6.6%\n\nSigned-off-by: Victoria Dye \n---\n builtin/reset.c | 2 ++\n t/perf/p7102-reset.sh | 21 +++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n create mode 100755 t/perf/p7102-reset.sh\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex fdce6f8c856..ab027774824 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -73,9 +73,11 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \tcase HARD:\n \t\topts.update = 1;\n \t\topts.reset = UNPACK_RESET_OVERWRITE_UNTRACKED;\n+\t\topts.skip_cache_tree_update = 1;\n \t\tbreak;\n \tcase MIXED:\n \t\topts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\n+\t\topts.skip_cache_tree_update = 1;\n \t\t/* but opts.update=0, so working tree not updated */\n \t\tbreak;\n \tdefault:\ndiff --git a/t/perf/p7102-reset.sh b/t/perf/p7102-reset.sh\nnew file mode 100755\nindex 00000000000..9b039e8691f\n--- /dev/null\n+++ b/t/perf/p7102-reset.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+\n+test_description='performance of reset'\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+test_checkout_worktree\n+\n+test_perf 'reset --hard with change in tree' '\n+\tbase=$(git rev-parse HEAD) &&\n+\ttest_commit --no-tag A &&\n+\tnew=$(git rev-parse HEAD) &&\n+\n+\tfor i in $(test_seq 10)\n+\tdo\n+\t\tgit reset --hard $new &&\n+\t\tgit reset --hard $base || return $?\n+\tdone\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"467080","messageId":"100c01e936cb331ab5b3c231dcd3050ea06e1868.1668107165.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] rebase: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T19:06:05Z","receivedAt":"2022-11-10T19:08:11Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable the 'skip_cache_tree_update' option in both 'do_reset()'\n('sequencer.c') and 'reset_head()' ('reset.c'). Both of these callers invoke\n'prime_cache_tree()' after 'unpack_trees()', so we can remove an unnecessary\ncache tree rebuild by skipping 'cache_tree_update()'.\n\nWhen testing with 'p3400-rebase.sh' and 'p3404-rebase-interactive.sh', the\nperformance change of this update was negligible, likely due to the\noperation being dominated by more expensive operations (like checking out\ntrees). However, since the change doesn't harm performance, it's worth\nkeeping this 'unpack_trees()' usage consistent with others that subsequently\ninvoke 'prime_cache_tree()'.\n\nSigned-off-by: Victoria Dye \n---\n reset.c | 1 +\n sequencer.c | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/reset.c b/reset.c\nindex e3383a93343..5ded23611f3 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -128,6 +128,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.merge = 1;\n \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n+\tunpack_tree_opts.skip_cache_tree_update = 1;\n \tinit_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);\n \tif (reset_hard)\n \t\tunpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..3f7a73ce4e1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3750,6 +3750,7 @@ static int do_reset(struct repository *r,\n \tunpack_tree_opts.merge = 1;\n \tunpack_tree_opts.update = 1;\n \tunpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */\n+\tunpack_tree_opts.skip_cache_tree_update = 1;\n \tinit_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);\n \n \tif (repo_read_index_unmerged(r)) {\n-- \ngitgitgadget\n"},{"id":"467081","messageId":"ea5c82ce992192681bc1ea230d1d57d0a1011ed6.1668107165.git.gitgitgadget@gmail.com","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] read-tree: use 'skip_cache_tree_update' option","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T19:06:04Z","receivedAt":"2022-11-10T19:08:13Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen running 'read-tree' with a single tree and no prefix,\n'prime_cache_tree()' is called after the tree is unpacked. In that\nsituation, skip a redundant call to 'cache_tree_update()' in\n'unpack_trees()' by enabling the 'skip_cache_tree_update' unpack option.\n\nRemoving the redundant cache tree update provides a substantial performance\nimprovement to 'git read-tree ', as shown by a test added to\n'p0006-read-tree-checkout.sh':\n\nTest before after\n----------------------------------------------------------------------\nread-tree br_ballast_plus_1 3.94(1.80+1.57) 3.00(1.14+1.28) -23.9%\n\nNote that the 'read-tree' in 't1022-read-tree-partial-clone.sh' is updated\nto read two trees, rather than one. The test was first introduced in\nd3da223f221 (cache-tree: prefetch in partial clone read-tree, 2021-07-23) to\nexercise the 'cache_tree_update()' code path, as used in 'git merge'. Since\nthis patch drops the call to 'cache_tree_update()' in single-tree 'git\nread-tree', change the test to use the two-tree variant so that\n'cache_tree_update()' is called as intended.\n\nSigned-off-by: Victoria Dye \n---\n builtin/read-tree.c | 4 ++++\n t/perf/p0006-read-tree-checkout.sh | 8 ++++++++\n t/t1022-read-tree-partial-clone.sh | 2 +-\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex f4cbe460b97..45c6652444b 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -249,6 +249,10 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tif (opts.debug_unpack)\n \t\topts.fn = debug_merge;\n \n+\t/* If we're going to prime_cache_tree later, skip cache tree update */\n+\tif (nr_trees == 1 && !opts.prefix)\n+\t\topts.skip_cache_tree_update = 1;\n+\n \tcache_tree_free(&active_cache_tree);\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\ndiff --git a/t/perf/p0006-read-tree-checkout.sh b/t/perf/p0006-read-tree-checkout.sh\nindex c481c012d2f..325566e18eb 100755\n--- a/t/perf/p0006-read-tree-checkout.sh\n+++ b/t/perf/p0006-read-tree-checkout.sh\n@@ -49,6 +49,14 @@ test_perf \"read-tree br_base br_ballast ($nr_files)\" '\n \tgit read-tree -n -m br_base br_ballast\n '\n \n+test_perf \"read-tree br_ballast_plus_1 ($nr_files)\" '\n+\t# Run read-tree 100 times for clearer performance results & comparisons\n+\tfor i in $(test_seq 100)\n+\tdo\n+\t\tgit read-tree -n -m br_ballast_plus_1 || return 1\n+\tdone\n+'\n+\n test_perf \"switch between br_base br_ballast ($nr_files)\" '\n \tgit checkout -q br_base &&\n \tgit checkout -q br_ballast\ndiff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh\nindex a9953b6a71c..da539716359 100755\n--- a/t/t1022-read-tree-partial-clone.sh\n+++ b/t/t1022-read-tree-partial-clone.sh\n@@ -19,7 +19,7 @@ test_expect_success 'read-tree in partial clone prefetches in one batch' '\n \tgit -C server config uploadpack.allowfilter 1 &&\n \tgit -C server config uploadpack.allowanysha1inwant 1 &&\n \tgit clone --bare --filter=blob:none \"file://$(pwd)/server\" client &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE $TREE &&\n \n \t# \"done\" marks the end of negotiation (once per fetch). Expect that\n \t# only one fetch occurs.\n-- \ngitgitgadget\n\n"},{"id":"467082","messageId":"20221110195029.GD1159673@szeder.dev","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-10T19:50:29Z","receivedAt":"2022-11-10T19:50:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Nov 10, 2022 at 07:06:00PM +0000, Victoria Dye via GitGitGadget wrote:\n> Changes since V2\n> ================\n> \n> * Cleaned up option handling & provided more informative error messages in\n> 'test-tool cache-tree'. The changes don't affect any behavior in the\n> added tests & 'test-tool cache-tree' won't be used outside of\n> development, but the improvements here will help future readers avoid\n> propagating error-prone implementations.\n> * Note that the suggestion to change the \"unknown subcommand\" error to a\n> 'usage()' error was not taken, as it would be somewhat cumbersome to\n> use a formatted string with it.\n\nI'm not sure I understand what's cumbersome. It's as simple as:\n\n if (...) {\n error(_(\"unknown subcommand: `%s'\"), argv[0]);\n usage_with_options(test_cache_tree_usage, options);\n }\n\n> This is in line with other custom\n> subcommand parsing in Git, such as in 'fsmonitor--daemon.c'.\n\nThe option parsing in 'fsmonitor--daemon.c' is broken, please don't\nconsider it as an example to follow.\n\n"},{"id":"467085","messageId":"9c3b71d2-6481-e702-329c-33ee988dd7ce@github.com","threadId":"58771","inReplyTo":"20221110195029.GD1159673@szeder.dev","subject":"Re: [PATCH v3 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-10T20:54:03Z","receivedAt":"2022-11-10T20:54:17Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"SZEDER Gábor wrote:\n> On Thu, Nov 10, 2022 at 07:06:00PM +0000, Victoria Dye via GitGitGadget wrote:\n>> Changes since V2\n>> ================\n>>\n>> * Cleaned up option handling & provided more informative error messages in\n>> 'test-tool cache-tree'. The changes don't affect any behavior in the\n>> added tests & 'test-tool cache-tree' won't be used outside of\n>> development, but the improvements here will help future readers avoid\n>> propagating error-prone implementations.\n>> * Note that the suggestion to change the \"unknown subcommand\" error to a\n>> 'usage()' error was not taken, as it would be somewhat cumbersome to\n>> use a formatted string with it.\n> \n> I'm not sure I understand what's cumbersome. It's as simple as:\n> \n> if (...) {\n> error(_(\"unknown subcommand: `%s'\"), argv[0]);\n> usage_with_options(test_cache_tree_usage, options);\n> }\n\nTo be honest, the cumbersome approach I was thinking of was 'sprintf()'-ing\nthe subcommand into the string and calling 'usage()' with that - your\nsuggestion is certainly much simpler. However, as a matter of personal\npreference, I still think the 'die()' is sufficient in the context of this\ntest helper (especially given that other test helpers do the same).\n\n> \n>> This is in line with other custom\n>> subcommand parsing in Git, such as in 'fsmonitor--daemon.c'.\n> \n> The option parsing in 'fsmonitor--daemon.c' is broken, please don't\n> consider it as an example to follow.\n\nWhile I understand your desire to helpfully guide users, I don't see\nanything to suggest that particular example is \"broken.\" The error conveys\nthe cause of the problem to a user, who could then run without arguments (or\nwith -h) to see what the valid subcommands are. And, in the case of this\ntest helper, I'm not particularly concerned with perfecting the (already\nsubjective) user experience, given that it's an internal-only tool.\n\nIf there are examples of proper usage patterns that future commands should\nfollow, I'd recommend updating 'CodingGuidelines' and/or\n'MyFirstContribution' to mention them. Codifying recommendations like that\ncan help avoid churn in reviews and, long-term, push Git to align on a\nuniform style.\n\n"},{"id":"467117","messageId":"Y224iAeMiazdJspp@nand.local","threadId":"58771","inReplyTo":"pull.1411.v3.git.1668107165.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T02:50:48Z","receivedAt":"2022-11-11T02:52:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 07:06:00PM +0000, Victoria Dye via GitGitGadget wrote:\n> Changes since V2\n> ================\n>\n> * Cleaned up option handling & provided more informative error messages in\n> 'test-tool cache-tree'. The changes don't affect any behavior in the\n> added tests & 'test-tool cache-tree' won't be used outside of\n> development, but the improvements here will help future readers avoid\n> propagating error-prone implementations.\n> * Note that the suggestion to change the \"unknown subcommand\" error to a\n> 'usage()' error was not taken, as it would be somewhat cumbersome to\n> use a formatted string with it. This is in line with other custom\n> subcommand parsing in Git, such as in 'fsmonitor--daemon.c'.\n\nThanks. The range-diff confirms what you say above. So between that and\nan affirmative review on the last round, I think we are ready to start\nmerging this one down.\n\nThanks,\nTaylor\n"},{"id":"467230","messageId":"5f6f4311-445e-f738-01ec-e36207eed910@github.com","threadId":"58771","inReplyTo":"Y224iAeMiazdJspp@nand.local","subject":"Re: [PATCH v3 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-14T00:08:39Z","receivedAt":"2022-11-14T00:08:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/10/22 9:50 PM, Taylor Blau wrote:\n> On Thu, Nov 10, 2022 at 07:06:00PM +0000, Victoria Dye via GitGitGadget wrote:\n>> Changes since V2\n>> ================\n>>\n>> * Cleaned up option handling & provided more informative error messages in\n>> 'test-tool cache-tree'. The changes don't affect any behavior in the\n>> added tests & 'test-tool cache-tree' won't be used outside of\n>> development, but the improvements here will help future readers avoid\n>> propagating error-prone implementations.\n>> * Note that the suggestion to change the \"unknown subcommand\" error to a\n>> 'usage()' error was not taken, as it would be somewhat cumbersome to\n>> use a formatted string with it. This is in line with other custom\n>> subcommand parsing in Git, such as in 'fsmonitor--daemon.c'.\n> \n> Thanks. The range-diff confirms what you say above. So between that and\n> an affirmative review on the last round, I think we are ready to start\n> merging this one down.\n\nI agree. This version still LGTM.\n\nThanks,\n-Stolee\n\n"}]}