{"thread":{"id":"64024","subject":"[PATCH 0/4] line-log: optimize merge commit processing","startedAt":"2025-08-24T19:06:56Z","lastAt":"2025-08-28T20:27:04Z","messageCount":12,"participants":["SZEDER Gábor","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"524844","messageId":"20250824190644.2573279-1-szeder.dev@gmail.com","threadId":"64024","inReplyTo":null,"subject":"[PATCH 0/4] line-log: optimize merge commit processing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-24T19:06:40Z","receivedAt":"2025-08-24T19:06:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The first patch is an optimization of the line-level log machinery.\nThe rest are cleanups in the area that I subjectively consider slight\nimprovements.\n\nSZEDER Gábor (4):\n line-log: avoid unnecessary tree diffs when processing merge commits\n line-log: get rid of the parents array in\n process_ranges_merge_commit()\n line-log: initialize diff queue in process_ranges_ordinary_commit()\n line-log: simplify condition checking for merge commits\n\n line-log.c | 50 ++++++++++++++++++++------------------------------\n 1 file changed, 20 insertions(+), 30 deletions(-)\n\n-- \n2.51.0.433.g1a66b3fb12\n\n"},{"id":"524845","messageId":"20250824190644.2573279-2-szeder.dev@gmail.com","threadId":"64024","inReplyTo":"20250824190644.2573279-1-szeder.dev@gmail.com","subject":"[PATCH 1/4] line-log: avoid unnecessary tree diffs when processing merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-24T19:06:41Z","receivedAt":"2025-08-24T19:06:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"In process_ranges_merge_commit(), the line-level log first creates an\narray of diff queues by iterating over all parents of a merge commit\nand computing a tree diff for each. Then in a second loop it iterates\nover those diff queues, and if it finds that none of the interesting\npaths were modified in one of them, then it will return early. This\nmeans that when none of the interesting paths were modified between a\nmerge and its first parent, then the tree diff between the merge and\nits second (Nth...) parent was computed in vain.\n\nUnify these two loops, so when it iterates over all parents of a merge\ncommit, then it first computes the tree diff between the merge and\nthat particular parent and then processes the resulting diff queue\nright away. This way we can spare some tree diff computing, thereby\nspeeding up line-level log in repositories with mergy history:\n\n # git.git, 25.8% of commits are merges:\n Benchmark 1: ./git_v2.51.0 -C ~/src/git log -L:'lookup_commit(':commit.c v2.51.0\n Time (mean ± σ): 1.001 s ± 0.009 s [User: 0.906 s, System: 0.095 s]\n Range (min … max): 0.991 s … 1.023 s 10 runs\n\n Benchmark 2: ./git -C ~/src/git log -L:'lookup_commit(':commit.c v2.51.0\n Time (mean ± σ): 445.5 ms ± 3.4 ms [User: 358.8 ms, System: 84.3 ms]\n Range (min … max): 440.1 ms … 450.3 ms 10 runs\n\n Summary\n './git -C ~/src/git log -L:'lookup_commit(':commit.c v2.51.0' ran\n 2.25 ± 0.03 times faster than './git_v2.51.0 -C ~/src/git log -L:'lookup_commit(':commit.c v2.51.0'\n\n # linux.git, 7.5% of commits are merges:\n Benchmark 1: ./git_v2.51.0 -C ~/src/linux.git log -L:build_restore_work_registers:arch/mips/mm/tlbex.c v6.16\n Time (mean ± σ): 3.246 s ± 0.007 s [User: 2.835 s, System: 0.409 s]\n Range (min … max): 3.232 s … 3.255 s 10 runs\n\n Benchmark 2: ./git -C ~/src/linux.git log -L:build_restore_work_registers:arch/mips/mm/tlbex.c v6.16\n Time (mean ± σ): 2.467 s ± 0.014 s [User: 2.113 s, System: 0.353 s]\n Range (min … max): 2.455 s … 2.505 s 10 runs\n\n Summary\n './git -C ~/src/linux.git log -L:build_restore_work_registers:arch/mips/mm/tlbex.c v6.16' ran\n 1.32 ± 0.01 times faster than './git_v2.51.0 -C ~/src/linux.git log -L:build_restore_work_registers:arch/mips/mm/tlbex.c v6.16'\n\nAnd since now each iteration computes a tree diff and processes its\nresult, there is no reason to store the diff queues for each merge\nparent anymore, so replace that diff queue array with a loop-local\ndiff queue variable. With this change the static free_diffqueues()\nhelper function in 'line-log.c' has no more callers left, remove it.\n\nSigned-off-by: SZEDER Gábor \n---\n line-log.c | 20 +++++---------------\n 1 file changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 07f2154e84..cf30915c94 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1087,13 +1087,6 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)\n \treturn new_filepair;\n }\n \n-static void free_diffqueues(int n, struct diff_queue_struct *dq)\n-{\n-\tfor (int i = 0; i < n; i++)\n-\t\tdiff_queue_clear(&dq[i]);\n-\tfree(dq);\n-}\n-\n static int process_all_files(struct line_log_data **range_out,\n \t\t\t struct rev_info *rev,\n \t\t\t struct diff_queue_struct *queue,\n@@ -1209,7 +1202,6 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n static int process_ranges_merge_commit(struct rev_info *rev, struct commit *commit,\n \t\t\t\t struct line_log_data *range)\n {\n-\tstruct diff_queue_struct *diffqueues;\n \tstruct line_log_data **cand;\n \tstruct commit **parents;\n \tstruct commit_list *p;\n@@ -1220,20 +1212,19 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \tif (nparents > 1 && rev->first_parent_only)\n \t\tnparents = 1;\n \n-\tALLOC_ARRAY(diffqueues, nparents);\n \tCALLOC_ARRAY(cand, nparents);\n \tALLOC_ARRAY(parents, nparents);\n \n \tp = commit->parents;\n \tfor (i = 0; i < nparents; i++) {\n+\t\tstruct diff_queue_struct diffqueue = DIFF_QUEUE_INIT;\n+\t\tint changed;\n \t\tparents[i] = p->item;\n \t\tp = p->next;\n-\t\tqueue_diffs(range, &rev->diffopt, &diffqueues[i], commit, parents[i]);\n-\t}\n+\t\tqueue_diffs(range, &rev->diffopt, &diffqueue, commit, parents[i]);\n \n-\tfor (i = 0; i < nparents; i++) {\n-\t\tint changed;\n-\t\tchanged = process_all_files(&cand[i], rev, &diffqueues[i], range);\n+\t\tchanged = process_all_files(&cand[i], rev, &diffqueue, range);\n+\t\tdiff_queue_clear(&diffqueue);\n \t\tif (!changed) {\n \t\t\t/*\n \t\t\t * This parent can take all the blame, so we\n@@ -1267,7 +1258,6 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t\tfree(cand[i]);\n \t}\n \tfree(cand);\n-\tfree_diffqueues(nparents, diffqueues);\n \treturn ret;\n \n \t/* NEEDSWORK evil merge detection stuff */\n-- \n2.51.0.433.g1a66b3fb12\n\n"},{"id":"524846","messageId":"20250824190644.2573279-3-szeder.dev@gmail.com","threadId":"64024","inReplyTo":"20250824190644.2573279-1-szeder.dev@gmail.com","subject":"[PATCH 2/4] line-log: get rid of the parents array in process_ranges_merge_commit()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-24T19:06:42Z","receivedAt":"2025-08-24T19:07:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"We can easily iterate through the parents of a merge commit without\nturning the list of parents into a dynamically allocated array of\nparents, so let's do so. This way we can avoid a memory allocation\nfor each processed merge commit, though its effect on runtime seems to\nbe unmeasurable.\n\nSigned-off-by: SZEDER Gábor \n---\n line-log.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex cf30915c94..b2a31ae956 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1203,7 +1203,6 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t\t\t\t struct line_log_data *range)\n {\n \tstruct line_log_data **cand;\n-\tstruct commit **parents;\n \tstruct commit_list *p;\n \tint i;\n \tint nparents = commit_list_count(commit->parents);\n@@ -1213,15 +1212,15 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t\tnparents = 1;\n \n \tCALLOC_ARRAY(cand, nparents);\n-\tALLOC_ARRAY(parents, nparents);\n \n-\tp = commit->parents;\n-\tfor (i = 0; i < nparents; i++) {\n+\tfor (p = commit->parents, i = 0;\n+\t p && i < nparents;\n+\t p = p->next, i++) {\n+\t\tstruct commit *parent = p->item;\n \t\tstruct diff_queue_struct diffqueue = DIFF_QUEUE_INIT;\n \t\tint changed;\n-\t\tparents[i] = p->item;\n-\t\tp = p->next;\n-\t\tqueue_diffs(range, &rev->diffopt, &diffqueue, commit, parents[i]);\n+\n+\t\tqueue_diffs(range, &rev->diffopt, &diffqueue, commit, parent);\n \n \t\tchanged = process_all_files(&cand[i], rev, &diffqueue, range);\n \t\tdiff_queue_clear(&diffqueue);\n@@ -1230,9 +1229,9 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t\t\t * This parent can take all the blame, so we\n \t\t\t * don't follow any other path in history\n \t\t\t */\n-\t\t\tadd_line_range(rev, parents[i], cand[i]);\n+\t\t\tadd_line_range(rev, parent, cand[i]);\n \t\t\tfree_commit_list(commit->parents);\n-\t\t\tcommit_list_append(parents[i], &commit->parents);\n+\t\t\tcommit_list_append(parent, &commit->parents);\n \n \t\t\tret = 0;\n \t\t\tgoto out;\n@@ -1243,14 +1242,15 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \t * No single parent took the blame. We add the candidates\n \t * from the above loop to the parents.\n \t */\n-\tfor (i = 0; i < nparents; i++)\n-\t\tadd_line_range(rev, parents[i], cand[i]);\n+\tfor (p = commit->parents, i = 0;\n+\t p && i < nparents;\n+\t p = p->next, i++)\n+\t\tadd_line_range(rev, p->item, cand[i]);\n \n \tret = 1;\n \n out:\n \tclear_commit_line_range(rev, commit);\n-\tfree(parents);\n \tfor (i = 0; i < nparents; i++) {\n \t\tif (!cand[i])\n \t\t\tcontinue;\n-- \n2.51.0.433.g1a66b3fb12\n\n"},{"id":"524847","messageId":"20250824190644.2573279-4-szeder.dev@gmail.com","threadId":"64024","inReplyTo":"20250824190644.2573279-1-szeder.dev@gmail.com","subject":"[PATCH 3/4] line-log: initialize diff queue in process_ranges_ordinary_commit()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-24T19:06:43Z","receivedAt":"2025-08-24T19:07:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"process_ranges_ordinary_commit() uses a local diff queue variable,\nwhich it leaves uninitialized before passing its address to\nqueue_diffs(). This is not an issue, because at the end of that\nfunction the contents of an other diff queue is moved into it by\nsimply overwriting whatever is in there, i.e. without reading any\nuninitialized memory.\n\nStill, seeing the uninitialized diff queue being passed around scared\nme more than once, so out of caution let's make sure that it's\ninitialized.\n\nSigned-off-by: SZEDER Gábor \n---\n line-log.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex b2a31ae956..71fa857ee8 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1182,7 +1182,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n \t\t\t\t\t struct line_log_data *range)\n {\n \tstruct commit *parent = NULL;\n-\tstruct diff_queue_struct queue;\n+\tstruct diff_queue_struct queue = DIFF_QUEUE_INIT;\n \tstruct line_log_data *parent_range;\n \tint changed;\n \n-- \n2.51.0.433.g1a66b3fb12\n\n"},{"id":"524848","messageId":"20250824190644.2573279-5-szeder.dev@gmail.com","threadId":"64024","inReplyTo":"20250824190644.2573279-1-szeder.dev@gmail.com","subject":"[PATCH 4/4] line-log: simplify condition checking for merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-24T19:06:44Z","receivedAt":"2025-08-24T19:07:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"In process_ranges_arbitrary_commit() the condition deciding whether\nthe given commit is not a merge, i.e. that it doesn't have more than\none parent, is head-scratchingly backwards, flip it.\n\nSigned-off-by: SZEDER Gábor \n---\n line-log.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 71fa857ee8..188d387d40 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1273,10 +1273,10 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit\n \t\t\tstruct line_log_data *prange = line_log_data_copy(range);\n \t\t\tadd_line_range(rev, commit->parents->item, prange);\n \t\t\tclear_commit_line_range(rev, commit);\n-\t\t} else if (!commit->parents || !commit->parents->next)\n-\t\t\tchanged = process_ranges_ordinary_commit(rev, commit, range);\n-\t\telse\n+\t\t} else if (commit->parents && commit->parents->next)\n \t\t\tchanged = process_ranges_merge_commit(rev, commit, range);\n+\t\telse\n+\t\t\tchanged = process_ranges_ordinary_commit(rev, commit, range);\n \t}\n \n \tif (!changed)\n-- \n2.51.0.433.g1a66b3fb12\n\n"},{"id":"524863","messageId":"930745d3-85a6-467b-a87b-b57e9623a604@gmail.com","threadId":"64024","inReplyTo":"20250824190644.2573279-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/4] line-log: avoid unnecessary tree diffs when processing merge commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-25T14:13:43Z","receivedAt":"2025-08-25T14:14:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2025 3:06 PM, SZEDER Gábor wrote:\n> In process_ranges_merge_commit(), the line-level log first creates an\n> array of diff queues by iterating over all parents of a merge commit\n> and computing a tree diff for each. Then in a second loop it iterates\n> over those diff queues, and if it finds that none of the interesting\n> paths were modified in one of them, then it will return early. This\n> means that when none of the interesting paths were modified between a\n> merge and its first parent, then the tree diff between the merge and\n> its second (Nth...) parent was computed in vain.\n\nGreat find! This goes all the way back to the original implementation\nin 12da1d1f6f (Implement line-history search (git log -L), 2013-03-28)\nwhere this detail could easily be missed in the rest of the scaffolding\nto implement the feature.\n\nThis is an understandable mistake to make as it can take some time to\nunderstand Git's simplified history computation and how it short-\ncircuits these merge diffs in most cases.\n> Summary\n> './git -C ~/src/git log -L:'lookup_commit(':commit.c v2.51.0' ran\n> 2.25 ± 0.03 times faster than './git_v2.51.0 -C ~/src/git log -L:'lookup_commit(':commit.c v2.51.0'\n\n> Summary\n> './git -C ~/src/linux.git log -L:build_restore_work_registers:arch/mips/mm/tlbex.c v6.16' ran\n> 1.32 ± 0.01 times faster than './git_v2.51.0 -C ~/src/linux.git log -L:build_restore_work_registers:arch/mips/mm/tlbex.c v6.16'\n\nGreat stats!\n> And since now each iteration computes a tree diff and processes its\n> result, there is no reason to store the diff queues for each merge\n> parent anymore, so replace that diff queue array with a loop-local\n> diff queue variable. With this change the static free_diffqueues()\n> helper function in 'line-log.c' has no more callers left, remove it.\n> \n> Signed-off-by: SZEDER Gábor \n> ---\n> line-log.c | 20 +++++---------------\n> 1 file changed, 5 insertions(+), 15 deletions(-)\n> \n> diff --git a/line-log.c b/line-log.c\n> index 07f2154e84..cf30915c94 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -1087,13 +1087,6 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)\n> \treturn new_filepair;\n> }\n> \n> -static void free_diffqueues(int n, struct diff_queue_struct *dq)\n> -{\n> -\tfor (int i = 0; i < n; i++)\n> -\t\tdiff_queue_clear(&dq[i]);\n> -\tfree(dq);\n> -}\n> -\n> static int process_all_files(struct line_log_data **range_out,\n> \t\t\t struct rev_info *rev,\n> \t\t\t struct diff_queue_struct *queue,\n> @@ -1209,7 +1202,6 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n> static int process_ranges_merge_commit(struct rev_info *rev, struct commit *commit,\n> \t\t\t\t struct line_log_data *range)\n> {\n> -\tstruct diff_queue_struct *diffqueues;\n> \tstruct line_log_data **cand;\n> \tstruct commit **parents;\n> \tstruct commit_list *p;\n> @@ -1220,20 +1212,19 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n> \tif (nparents > 1 && rev->first_parent_only)\n> \t\tnparents = 1;\n> \n> -\tALLOC_ARRAY(diffqueues, nparents);\n> \tCALLOC_ARRAY(cand, nparents);\n> \tALLOC_ARRAY(parents, nparents);\n> \n> \tp = commit->parents;\n> \tfor (i = 0; i < nparents; i++) {\n> +\t\tstruct diff_queue_struct diffqueue = DIFF_QUEUE_INIT;\n> +\t\tint changed;\n> \t\tparents[i] = p->item;\n> \t\tp = p->next;\n> -\t\tqueue_diffs(range, &rev->diffopt, &diffqueues[i], commit, parents[i]);\n> -\t}\n> +\t\tqueue_diffs(range, &rev->diffopt, &diffqueue, commit, parents[i]);\n> \n> -\tfor (i = 0; i < nparents; i++) {\n> -\t\tint changed;\n> -\t\tchanged = process_all_files(&cand[i], rev, &diffqueues[i], range);\n> +\t\tchanged = process_all_files(&cand[i], rev, &diffqueue, range);\n> +\t\tdiff_queue_clear(&diffqueue);\n> \t\tif (!changed) {\n> \t\t\t/*\n> \t\t\t * This parent can take all the blame, so we\n> @@ -1267,7 +1258,6 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n> \t\tfree(cand[i]);\n> \t}\n> \tfree(cand);\n> -\tfree_diffqueues(nparents, diffqueues);\n> \treturn ret;\n> \n> \t/* NEEDSWORK evil merge detection stuff */\n\nThis diff is a lot cleaner than I expected it to be. Excellent!\n\nI applied this patch locally and tested it on a few repos I have\nto give extra confidence to your patch.\n\nFor an internal monorepo, I was able to measure these results:\n\nBenchmark 1: old\n Time (mean ± σ): 19.709 s ± 0.014 s [User: 18.846 s, System: 0.862 s]\n Range (min … max): 19.681 s … 19.725 s 10 runs\n \nBenchmark 2: new\n Time (mean ± σ): 9.061 s ± 0.015 s [User: 8.487 s, System: 0.574 s]\n Range (min … max): 9.042 s … 9.089 s 10 runs\n \nSummary\n 'new' ran\n 2.18 ± 0.00 times faster than 'old'\n\nI did also want to check to see the impact of f32dde8c12 (line-log:\nintegrate with changed-path Bloom filters, 2020-05-11), and having\ncomputed filters diminished the size of your impact somewhat:\n\nYour Git example:\n\nBenchmark 1: old\n Time (mean ± σ): 279.2 ms ± 2.2 ms [User: 231.0 ms, System: 47.9 ms]\n Range (min … max): 275.5 ms … 282.6 ms 10 runs\n \nBenchmark 2: new \n Time (mean ± σ): 242.4 ms ± 3.6 ms [User: 191.8 ms, System: 50.4 ms]\n Range (min … max): 237.3 ms … 249.9 ms 12 runs\n \nSummary\n 'new ' ran\n 1.15 ± 0.02 times faster than 'old'\n\nYour Linux example:\n\nBenchmark 1: old\n Time (mean ± σ): 1.694 s ± 0.008 s [User: 1.524 s, System: 0.169 s]\n Range (min … max): 1.688 s … 1.714 s 10 runs\n \nBenchmark 2: new \n Time (mean ± σ): 1.644 s ± 0.008 s [User: 1.482 s, System: 0.161 s]\n Range (min … max): 1.636 s … 1.663 s 10 runs\n \nSummary\n 'new ' ran\n 1.03 ± 0.01 times faster than 'old'\n\nMy internal monorepo example:\n\nBenchmark 1: old\n Time (mean ± σ): 3.749 s ± 0.007 s [User: 3.188 s, System: 0.559 s]\n Range (min … max): 3.736 s … 3.759 s 10 runs\n \nBenchmark 2: new\n Time (mean ± σ): 2.713 s ± 0.005 s [User: 2.318 s, System: 0.394 s]\n Range (min … max): 2.706 s … 2.723 s 10 runs\n \nSummary\n 'new' ran\n 1.38 ± 0.00 times faster than 'old'\n\nRerunning with \"-c commitGraph.readChangedPaths=false\" resulted\nin numbers closer to your examples (940ms->420ms for Git and\n2.6->2.1s for Linux). I expect most users to be in the situation\nwhere there are no changed-path Bloom filters, so this is very\ngood to deliver that value.\n\nAt first, I found this to be concerning: we only store filters\nfor the diff between a commit and its first parent, so this cost\nof visiting the later parents should be _much worse_ in those\ncases. However, it turns out that the way that the filters are\nhandled in line_log_process_ranges_arbitrary_commit() avoids a\ncall to process_ranges_merge_commit() if the filter says that the\nfirst parent is TREESAME on the given path.\n\nThis means that a good chunk of the performance benefits in\nf32dde8c12 (line-log: integrate with changed-path Bloom filters,\n2020-05-11) are _actually_ due to avoiding this extra work for\nmultiple parents. Thanks for digging in and bringing this benefit\nto all users!\n\nThanks,\n-Stolee\n\n\n"},{"id":"524864","messageId":"8adecfbf-8593-4084-81f8-d7c23950e4e4@gmail.com","threadId":"64024","inReplyTo":"20250824190644.2573279-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/4] line-log: optimize merge commit processing","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-25T14:16:04Z","receivedAt":"2025-08-25T14:16:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2025 3:06 PM, SZEDER Gábor wrote:\n> The first patch is an optimization of the line-level log machinery.\n> The rest are cleanups in the area that I subjectively consider slight\n> improvements.\n> \n> SZEDER Gábor (4):\n> line-log: avoid unnecessary tree diffs when processing merge commits\n\nI gave a detailed double-check of your perf numbers in a direct reply\nto that patch.\n\n> line-log: get rid of the parents array in\n> process_ranges_merge_commit()\n> line-log: initialize diff queue in process_ranges_ordinary_commit()\n> line-log: simplify condition checking for merge commits\n\nThese cleanups are very welcome. The whole series is excellent.\n\nThanks,\n-Stolee\n\n"},{"id":"524865","messageId":"xmqqms7ntnvq.fsf@gitster.g","threadId":"64024","inReplyTo":"20250824190644.2573279-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/4] line-log: avoid unnecessary tree diffs when processing merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T15:35:53Z","receivedAt":"2025-08-25T15:35:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> @@ -1209,7 +1202,6 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n> static int process_ranges_merge_commit(struct rev_info *rev, struct commit *commit,\n> \t\t\t\t struct line_log_data *range)\n> {\n> -\tstruct diff_queue_struct *diffqueues;\n> \tstruct line_log_data **cand;\n> \tstruct commit **parents;\n> \tstruct commit_list *p;\n> @@ -1220,20 +1212,19 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n> \tif (nparents > 1 && rev->first_parent_only)\n> \t\tnparents = 1;\n> \n> -\tALLOC_ARRAY(diffqueues, nparents);\n> \tCALLOC_ARRAY(cand, nparents);\n> \tALLOC_ARRAY(parents, nparents);\n> \n> \tp = commit->parents;\n> \tfor (i = 0; i < nparents; i++) {\n> +\t\tstruct diff_queue_struct diffqueue = DIFF_QUEUE_INIT;\n> +\t\tint changed;\n> \t\tparents[i] = p->item;\n> \t\tp = p->next;\n> -\t\tqueue_diffs(range, &rev->diffopt, &diffqueues[i], commit, parents[i]);\n> -\t}\n> +\t\tqueue_diffs(range, &rev->diffopt, &diffqueue, commit, parents[i]);\n> \n> -\tfor (i = 0; i < nparents; i++) {\n> -\t\tint changed;\n> -\t\tchanged = process_all_files(&cand[i], rev, &diffqueues[i], range);\n> +\t\tchanged = process_all_files(&cand[i], rev, &diffqueue, range);\n> +\t\tdiff_queue_clear(&diffqueue);\n> \t\tif (!changed) {\n> \t\t\t/*\n> \t\t\t * This parent can take all the blame, so we\n\nThis is surprisingly small change that eliminates quite a lot of\nwaste. Nicely done.\n\n> @@ -1267,7 +1258,6 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n> \t\tfree(cand[i]);\n> \t}\n> \tfree(cand);\n> -\tfree_diffqueues(nparents, diffqueues);\n> \treturn ret;\n> \n> \t/* NEEDSWORK evil merge detection stuff */\n"},{"id":"524893","messageId":"xmqq4itvp19r.fsf@gitster.g","threadId":"64024","inReplyTo":"20250824190644.2573279-5-szeder.dev@gmail.com","subject":"Re: [PATCH 4/4] line-log: simplify condition checking for merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T20:57:52Z","receivedAt":"2025-08-25T20:57:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> In process_ranges_arbitrary_commit() the condition deciding whether\n> the given commit is not a merge, i.e. that it doesn't have more than\n> one parent, is head-scratchingly backwards, flip it.\n\nHmph, the condition is about \"is it a root commit? or is it a\nsingle-parent commit?\", which does not sound overly complicated to\nme.\n\n> Signed-off-by: SZEDER Gábor \n> ---\n> line-log.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/line-log.c b/line-log.c\n> index 71fa857ee8..188d387d40 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -1273,10 +1273,10 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit\n> \t\t\tstruct line_log_data *prange = line_log_data_copy(range);\n> \t\t\tadd_line_range(rev, commit->parents->item, prange);\n> \t\t\tclear_commit_line_range(rev, commit);\n> -\t\t} else if (!commit->parents || !commit->parents->next)\n> -\t\t\tchanged = process_ranges_ordinary_commit(rev, commit, range);\n> -\t\telse\n> +\t\t} else if (commit->parents && commit->parents->next)\n> \t\t\tchanged = process_ranges_merge_commit(rev, commit, range);\n> +\t\telse\n> +\t\t\tchanged = process_ranges_ordinary_commit(rev, commit, range);\n> \t}\n> \n> \tif (!changed)\n"},{"id":"524895","messageId":"f98f3db4-cd36-4a24-903f-7aebf6af3d51@gmail.com","threadId":"64024","inReplyTo":"xmqq4itvp19r.fsf@gitster.g","subject":"Re: [PATCH 4/4] line-log: simplify condition checking for merge commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-25T21:43:10Z","receivedAt":"2025-08-25T21:43:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2025 4:57 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n>> In process_ranges_arbitrary_commit() the condition deciding whether\n>> the given commit is not a merge, i.e. that it doesn't have more than\n>> one parent, is head-scratchingly backwards, flip it.\n> \n> Hmph, the condition is about \"is it a root commit? or is it a\n> single-parent commit?\", which does not sound overly complicated to\n> me.\n\nIt is something that one can interpret carefully by thinking about\nit, but the negation and OR condition made me need to pause and\nthink about it, while the positive of \"does it have a parent and\na second parent?\" was something that flowed naturally when I read\nit.\n\nDefinitely a taste thing, so I could see you wanting to skip this\none on a pure \"don't touch what's not broken\" policy.\n\nThanks,\n-Stolee\n\n"},{"id":"524896","messageId":"xmqqqzwznjya.fsf@gitster.g","threadId":"64024","inReplyTo":"f98f3db4-cd36-4a24-903f-7aebf6af3d51@gmail.com","subject":"Re: [PATCH 4/4] line-log: simplify condition checking for merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T21:57:17Z","receivedAt":"2025-08-25T21:57:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> ... but the negation and OR condition made me need to pause and\n> think about it, while the positive of \"does it have a parent and\n> a second parent?\" was something that flowed naturally when I read\n> it.\n\nYeah, that I 100% agree with. If it were\n\n\tif (!(c->parent && c->parent->next))\n\t\thandle_ordinary_commit();\n\telse\n\t\thandle_merge_commit();\n\nthat would have been very easy to grok. I do not have a strong\npreference between that and\n\n\tif (c->parent && c->parent->next)\n\t\thandle_merge_commit();\n\telse\n\t\thandle_ordinary_commit();\n\nmyself, but I always felt that handling ordinary commits was the\nprimary thing in this code path, which made me react to the swapping\nof orders of these two calls.\n\n> Definitely a taste thing, so I could see you wanting to skip this\n> one on a pure \"don't touch what's not broken\" policy.\n\nTrue, too, but the code that fails to be in a readable shape too\nfalls into the \"broken\" category, so in that sense I do not mind\nqueuing the patch, either (and indeed tonight's 'seen' will include\nthis step in the topic).\n\nThanks.\n"},{"id":"525146","messageId":"aLC7lQQWfdB/QUk3@szeder.dev","threadId":"64024","inReplyTo":"xmqqms7ntnvq.fsf@gitster.g","subject":"Re: [PATCH 1/4] line-log: avoid unnecessary tree diffs when processing merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-08-28T20:27:01Z","receivedAt":"2025-08-28T20:27:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 25, 2025 at 08:35:53AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > @@ -1209,7 +1202,6 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n> > static int process_ranges_merge_commit(struct rev_info *rev, struct commit *commit,\n> > \t\t\t\t struct line_log_data *range)\n> > {\n> > -\tstruct diff_queue_struct *diffqueues;\n> > \tstruct line_log_data **cand;\n> > \tstruct commit **parents;\n> > \tstruct commit_list *p;\n> > @@ -1220,20 +1212,19 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n> > \tif (nparents > 1 && rev->first_parent_only)\n> > \t\tnparents = 1;\n> > \n> > -\tALLOC_ARRAY(diffqueues, nparents);\n> > \tCALLOC_ARRAY(cand, nparents);\n> > \tALLOC_ARRAY(parents, nparents);\n> > \n> > \tp = commit->parents;\n> > \tfor (i = 0; i < nparents; i++) {\n> > +\t\tstruct diff_queue_struct diffqueue = DIFF_QUEUE_INIT;\n> > +\t\tint changed;\n> > \t\tparents[i] = p->item;\n> > \t\tp = p->next;\n> > -\t\tqueue_diffs(range, &rev->diffopt, &diffqueues[i], commit, parents[i]);\n> > -\t}\n> > +\t\tqueue_diffs(range, &rev->diffopt, &diffqueue, commit, parents[i]);\n> > \n> > -\tfor (i = 0; i < nparents; i++) {\n> > -\t\tint changed;\n> > -\t\tchanged = process_all_files(&cand[i], rev, &diffqueues[i], range);\n> > +\t\tchanged = process_all_files(&cand[i], rev, &diffqueue, range);\n> > +\t\tdiff_queue_clear(&diffqueue);\n> > \t\tif (!changed) {\n> > \t\t\t/*\n> > \t\t\t * This parent can take all the blame, so we\n> \n> This is surprisingly small change that eliminates quite a lot of\n> waste. Nicely done.\n\nIt's funny you say that...\n\nThis patch series just turned 6 years old this weekend, and up until\nSunday morning this first patch was actually two, because the\noptimization and the removal of the now unnecessary diffqueues array\nwere two separate patches that I finally decided to squash together.\n\nHere is the diff of that optimization-only patch :)\n\n ---- >8 ----\n\n line-log.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 07f2154e84..b3766c67ea 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1220,19 +1220,17 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm\n \tif (nparents > 1 && rev->first_parent_only)\n \t\tnparents = 1;\n \n-\tALLOC_ARRAY(diffqueues, nparents);\n+\tCALLOC_ARRAY(diffqueues, nparents);\n \tCALLOC_ARRAY(cand, nparents);\n \tALLOC_ARRAY(parents, nparents);\n \n \tp = commit->parents;\n \tfor (i = 0; i < nparents; i++) {\n+\t\tint changed;\n \t\tparents[i] = p->item;\n \t\tp = p->next;\n \t\tqueue_diffs(range, &rev->diffopt, &diffqueues[i], commit, parents[i]);\n-\t}\n \n-\tfor (i = 0; i < nparents; i++) {\n-\t\tint changed;\n \t\tchanged = process_all_files(&cand[i], rev, &diffqueues[i], range);\n \t\tif (!changed) {\n \t\t\t/*\n"}]}