{"thread":{"id":"52551","subject":"[PATCH 0/1] [PERF BUG] Fix size_mult option in fetch.writeCommitGraph","startedAt":"2020-01-02T16:14:19Z","lastAt":"2020-01-06T19:39:06Z","messageCount":6,"participants":["Derrick Stolee via GitGitGadget","Derrick Stolee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"389164","messageId":"pull.509.git.1577981654.gitgitgadget@gmail.com","threadId":"52551","inReplyTo":null,"subject":"[PATCH 0/1] [PERF BUG] Fix size_mult option in fetch.writeCommitGraph","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-02T16:14:13Z","receivedAt":"2020-01-02T16:14:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I found this while doing some digging into fetch behavior and split commit\ngraphs. I had been running fetch.writeCommitGraph=true on my local repos for\na while and noticed that the commit-graph chains were much longer than\nexpected.\n\nThe reason is silly, and the commit message includes all the details.\n\nThis behavior exists since v2.24.0, so I'm not sure if it makes the bar for\nv2.25.0 this late in the release cycle. At minimum, the change is very small\nand unlikely to cause more pain.\n\nThis is only a performance bug, and the effect is relatively small. A large\nlist of commit-graph files slows down the commit lookup time as we need to\nperform a linear number of binary searches. This only affects finding the\nfirst commit(s) in a commit walk, as after that we can navigate quickly to\nthe correct position using graph_pos. When a user runs gc (with \ngc.writeCommitGraph=true, on by default), the chain collapses to a single\nlevel, fixing the performance problem.\n\nThanks, -Stolee\n\nDerrick Stolee (1):\n fetch: set size_multiple in split_commit_graph_opts\n\n builtin/fetch.c | 4 +---\n commit-graph.c | 4 +++-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\n\nbase-commit: 99c33bed562b41de6ce9bd3fd561303d39645048\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-509%2Fderrickstolee%2Ffetch-write-commit-graph-split-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-509/derrickstolee/fetch-write-commit-graph-split-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/509\n-- \ngitgitgadget\n"},{"id":"389165","messageId":"91d89356a20625d04af74d458c28b32445e760c1.1577981654.git.gitgitgadget@gmail.com","threadId":"52551","inReplyTo":"pull.509.git.1577981654.gitgitgadget@gmail.com","subject":"[PATCH 1/1] fetch: set size_multiple in split_commit_graph_opts","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-02T16:14:14Z","receivedAt":"2020-01-02T16:14:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn 50f26bd (\"fetch: add fetch.writeCommitGraph config setting\",\n2019-09-02), the fetch builtin added the capability to write a\ncommit-graph using the \"--split\" feature. This feature creates\nmultiple commit-graph files, and those can merge based on a set\nof \"split options\" including a size multiple. The default size\nmultiple is 2, which intends to provide a log_2 N depth of the\ncommit-graph chain where N is the number of commits.\n\nHowever, I noticed during dogfooding that my commit-graph chains\nwere becoming quite large when left only to builds by 'git fetch'.\nIt turns out that in split_graph_merge_strategy(), we default the\nsize_mult variable to 2 except we override it with the context's\nsplit_opts if they exist. In builtin/fetch.c, we create such a\nsplit_opts, but do not populate it with values.\n\nThis problem is due to two failures:\n\n 1. It is unclear that we can add the flag COMMIT_GRAPH_WRITE_SPLIT\n with a NULL split_opts.\n 2. If we have a non-NULL split_opts, then we override the default\n values even if a zero value is given.\n\nCorrect both of these issues. First, do not override size_mult when\nthe options provide a zero value. Second, stop creating a split_opts\nin the fetch builtin.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/fetch.c | 4 +---\n commit-graph.c | 4 +++-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f8765b385b..b4c6d921d0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1866,15 +1866,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t (fetch_write_commit_graph < 0 &&\n \t the_repository->settings.fetch_write_commit_graph)) {\n \t\tint commit_graph_flags = COMMIT_GRAPH_WRITE_SPLIT;\n-\t\tstruct split_commit_graph_opts split_opts;\n-\t\tmemset(&split_opts, 0, sizeof(struct split_commit_graph_opts));\n \n \t\tif (progress)\n \t\t\tcommit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;\n \n \t\twrite_commit_graph_reachable(get_object_directory(),\n \t\t\t\t\t commit_graph_flags,\n-\t\t\t\t\t &split_opts);\n+\t\t\t\t\t NULL);\n \t}\n \n \tclose_object_store(the_repository->objects);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e771394aff..b205e65ed1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1542,7 +1542,9 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \tif (ctx->split_opts) {\n \t\tmax_commits = ctx->split_opts->max_commits;\n-\t\tsize_mult = ctx->split_opts->size_multiple;\n+\n+\t\tif (ctx->split_opts->size_multiple)\n+\t\t\tsize_mult = ctx->split_opts->size_multiple;\n \t}\n \n \tg = ctx->r->objects->commit_graph;\n-- \ngitgitgadget\n"},{"id":"389166","messageId":"a6dfda23-541c-14ea-fd72-b71fa782d3c7@gmail.com","threadId":"52551","inReplyTo":"91d89356a20625d04af74d458c28b32445e760c1.1577981654.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: set size_multiple in split_commit_graph_opts","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-02T16:20:41Z","receivedAt":"2020-01-02T16:20:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"As I was writing this commit message, I changed the plan for how\nto fix this. Originally, I was going to set size_multiple = 2 in\nbuiltin/fetch.c, which is how the subject line was created. I forgot\nto change that to something more like:\n\n\t\"commit-graph: prefer default size_mult when given zero\"\n\nOn 1/2/2020 11:14 AM, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> In 50f26bd (\"fetch: add fetch.writeCommitGraph config setting\",\n> 2019-09-02), the fetch builtin added the capability to write a\n> commit-graph using the \"--split\" feature. This feature creates\n> multiple commit-graph files, and those can merge based on a set\n> of \"split options\" including a size multiple. The default size\n> multiple is 2, which intends to provide a log_2 N depth of the\n> commit-graph chain where N is the number of commits.\n> \n> However, I noticed during dogfooding that my commit-graph chains\n> were becoming quite large when left only to builds by 'git fetch'.\n> It turns out that in split_graph_merge_strategy(), we default the\n> size_mult variable to 2 except we override it with the context's\n> split_opts if they exist. In builtin/fetch.c, we create such a\n> split_opts, but do not populate it with values.\n> \n> This problem is due to two failures:\n> \n> 1. It is unclear that we can add the flag COMMIT_GRAPH_WRITE_SPLIT\n> with a NULL split_opts.\n> 2. If we have a non-NULL split_opts, then we override the default\n> values even if a zero value is given.\n> \n> Correct both of these issues. First, do not override size_mult when\n> the options provide a zero value. Second, stop creating a split_opts\n> in the fetch builtin.\n\nThis is a correct description of the actual patch.\n\nThanks,\n-Stolee\n\n\n"},{"id":"389182","messageId":"xmqq7e29edfd.fsf@gitster-ct.c.googlers.com","threadId":"52551","inReplyTo":"91d89356a20625d04af74d458c28b32445e760c1.1577981654.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: set size_multiple in split_commit_graph_opts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-02T21:49:58Z","receivedAt":"2020-01-02T21:50:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> This problem is due to two failures:\n>\n> 1. It is unclear that we can add the flag COMMIT_GRAPH_WRITE_SPLIT\n> with a NULL split_opts.\n> 2. If we have a non-NULL split_opts, then we override the default\n> values even if a zero value is given.\n>\n> Correct both of these issues. First, do not override size_mult when\n> the options provide a zero value. Second, stop creating a split_opts\n> in the fetch builtin.\n\nOK, so there is the hardcoded default 2 in the code, and split_opts\nstructure *can* override it, but 0 in the field of the structure is\nmeant to signal \"no, I do not have any value to override the\ndefault\", not \"I do want to set the multiple to 0\"?\n\nMakes sense. Will queue.\n\nThanks.\n\n\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/fetch.c | 4 +---\n> commit-graph.c | 4 +++-\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index f8765b385b..b4c6d921d0 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1866,15 +1866,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t (fetch_write_commit_graph < 0 &&\n> \t the_repository->settings.fetch_write_commit_graph)) {\n> \t\tint commit_graph_flags = COMMIT_GRAPH_WRITE_SPLIT;\n> -\t\tstruct split_commit_graph_opts split_opts;\n> -\t\tmemset(&split_opts, 0, sizeof(struct split_commit_graph_opts));\n> \n> \t\tif (progress)\n> \t\t\tcommit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;\n> \n> \t\twrite_commit_graph_reachable(get_object_directory(),\n> \t\t\t\t\t commit_graph_flags,\n> -\t\t\t\t\t &split_opts);\n> +\t\t\t\t\t NULL);\n> \t}\n> \n> \tclose_object_store(the_repository->objects);\n> diff --git a/commit-graph.c b/commit-graph.c\n> index e771394aff..b205e65ed1 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1542,7 +1542,9 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n> \n> \tif (ctx->split_opts) {\n> \t\tmax_commits = ctx->split_opts->max_commits;\n> -\t\tsize_mult = ctx->split_opts->size_multiple;\n> +\n> +\t\tif (ctx->split_opts->size_multiple)\n> +\t\t\tsize_mult = ctx->split_opts->size_multiple;\n> \t}\n> \n> \tg = ctx->r->objects->commit_graph;\n"},{"id":"389193","messageId":"a6a6547b-495d-9929-daff-9c0069428703@gmail.com","threadId":"52551","inReplyTo":"xmqq7e29edfd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: set size_multiple in split_commit_graph_opts","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-03T13:07:44Z","receivedAt":"2020-01-03T13:07:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/2/2020 4:49 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> This problem is due to two failures:\n>>\n>> 1. It is unclear that we can add the flag COMMIT_GRAPH_WRITE_SPLIT\n>> with a NULL split_opts.\n>> 2. If we have a non-NULL split_opts, then we override the default\n>> values even if a zero value is given.\n>>\n>> Correct both of these issues. First, do not override size_mult when\n>> the options provide a zero value. Second, stop creating a split_opts\n>> in the fetch builtin.\n> \n> OK, so there is the hardcoded default 2 in the code, and split_opts\n> structure *can* override it, but 0 in the field of the structure is\n> meant to signal \"no, I do not have any value to override the\n> default\", not \"I do want to set the multiple to 0\"?\n\nCorrect. The multiple 0 makes it so we never merge layers of the\nchain, and this was happening accidentally. A caller could still\naccomplish this by passing -1, but that is not recommended.\n\n-Stolee\n\n"},{"id":"389281","messageId":"20200106193903.GB971477@coredump.intra.peff.net","threadId":"52551","inReplyTo":"91d89356a20625d04af74d458c28b32445e760c1.1577981654.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: set size_multiple in split_commit_graph_opts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-06T19:39:03Z","receivedAt":"2020-01-06T19:39:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 02, 2020 at 04:14:14PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> In 50f26bd (\"fetch: add fetch.writeCommitGraph config setting\",\n> 2019-09-02), the fetch builtin added the capability to write a\n> commit-graph using the \"--split\" feature. This feature creates\n> multiple commit-graph files, and those can merge based on a set\n> of \"split options\" including a size multiple. The default size\n> multiple is 2, which intends to provide a log_2 N depth of the\n> commit-graph chain where N is the number of commits.\n> \n> However, I noticed during dogfooding that my commit-graph chains\n> were becoming quite large when left only to builds by 'git fetch'.\n> It turns out that in split_graph_merge_strategy(), we default the\n> size_mult variable to 2 except we override it with the context's\n> split_opts if they exist. In builtin/fetch.c, we create such a\n> split_opts, but do not populate it with values.\n> \n> This problem is due to two failures:\n> \n> 1. It is unclear that we can add the flag COMMIT_GRAPH_WRITE_SPLIT\n> with a NULL split_opts.\n> 2. If we have a non-NULL split_opts, then we override the default\n> values even if a zero value is given.\n> \n> Correct both of these issues. First, do not override size_mult when\n> the options provide a zero value. Second, stop creating a split_opts\n> in the fetch builtin.\n\nThanks, the explanation and fix (both parts) look good to me, modulo the\nsubject correction you already noted.\n\n> ---\n> builtin/fetch.c | 4 +---\n> commit-graph.c | 4 +++-\n\nIs it worth covering this with a test?\n\nI guess the non-fetch code paths for splitting already cover this pretty\nwell, and this is just about managing to get the right number into the\ncommit-graph code. So perhaps it isn't worth it.\n\n-Peff\n"}]}