{"thread":{"id":"31728","subject":"git pull takes ~8 seconds on up-to-date Linux git tree","startedAt":"2012-10-04T14:14:54Z","lastAt":"2012-10-06T12:57:35Z","messageCount":13,"participants":["Markus Trippelsdorf","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"200518","messageId":"20121004141454.GA246@x4","threadId":"31728","inReplyTo":null,"subject":"git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2012-10-04T14:14:54Z","receivedAt":"2012-10-04T14:14:54Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"Hi,\n\nwith current trunk I get the following on an up-to-date Linux tree:\n\nmarkus@x4 linux % time git pull\nAlready up-to-date.\ngit pull 7.84s user 0.26s system 92% cpu 8.743 total\n\ngit version 1.7.12 is much quicker:\n\nmarkus@x4 linux % time git pull\nAlready up-to-date.\ngit pull 0.10s user 0.02s system 16% cpu 0.740 total\n\nperf shows for trunk:\n\n 22.11% git-merge libz.so.1.2.7 [.] 0x00000000000073bc \n 22.03% git libz.so.1.2.7 [.] 0x0000000000007338 \n 14.18% git libz.so.1.2.7 [.] inflate \n 13.70% git-merge libz.so.1.2.7 [.] inflate \n 9.18% git git [.] 0x00000000000ea391 \n 8.56% git-merge git-merge [.] 0x00000000000f0598 \n 1.58% git-merge libz.so.1.2.7 [.] adler32 \n 1.52% git libz.so.1.2.7 [.] adler32 \n 0.59% git [kernel.kallsyms] [k] clear_page_c\n\nand for 1.7.12:\n\n 39.29% git git [.] 0x00000000000b9fa8 \n 12.16% git libz.so.1.2.7 [.] inflate \n 8.67% git libz.so.1.2.7 [.] 0x000000000000a18e \n 8.49% git-merge git-merge [.] 0x00000000000efa15 \n 4.96% git libc-2.16.90.so [.] memcpy@@GLIBC_2.14 \n 2.63% git libc-2.16.90.so [.] _int_malloc \n 2.61% git-merge [kernel.kallsyms] [k] clear_page_c \n 2.32% git [kernel.kallsyms] [k] clear_page_c \n 2.04% git [kernel.kallsyms] [k] filemap_fault \n 1.87% git-merge libc-2.16.90.so [.] memcpy@@GLIBC_2.14 \n\n-- \nMarkus\n"},{"id":"200565","messageId":"20121004184314.GA15389@sigill.intra.peff.net","threadId":"31728","inReplyTo":"20121004141454.GA246@x4","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-04T18:43:14Z","receivedAt":"2012-10-04T18:43:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n\n> with current trunk I get the following on an up-to-date Linux tree:\n> \n> markus@x4 linux % time git pull\n> Already up-to-date.\n> git pull 7.84s user 0.26s system 92% cpu 8.743 total\n> \n> git version 1.7.12 is much quicker:\n> \n> markus@x4 linux % time git pull\n> Already up-to-date.\n> git pull 0.10s user 0.02s system 16% cpu 0.740 total\n\nYikes. I can easily reproduce here. Bisecting between master and\nv1.7.12 gives a curious result: the slowdown first occurs with the merge\ncommit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\nof its parents is slow. I don't see anything obviously suspect in the\nmerge, though.\n\n-Peff\n"},{"id":"200541","messageId":"20121004192621.GA244@x4","threadId":"31728","inReplyTo":"20121004184314.GA15389@sigill.intra.peff.net","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2012-10-04T19:26:21Z","receivedAt":"2012-10-04T19:26:21Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"On 2012.10.04 at 14:43 -0400, Jeff King wrote:\n> On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n> \n> > with current trunk I get the following on an up-to-date Linux tree:\n> > \n> > markus@x4 linux % time git pull\n> > Already up-to-date.\n> > git pull 7.84s user 0.26s system 92% cpu 8.743 total\n> > \n> > git version 1.7.12 is much quicker:\n> > \n> > markus@x4 linux % time git pull\n> > Already up-to-date.\n> > git pull 0.10s user 0.02s system 16% cpu 0.740 total\n> \n> Yikes. I can easily reproduce here. Bisecting between master and\n> v1.7.12 gives a curious result: the slowdown first occurs with the merge\n> commit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\n> of its parents is slow. I don't see anything obviously suspect in the\n> merge, though.\n\nActually commit f37d3c75 is responsible for this. When I revert it, the\nproblem goes away.\n\n-- \nMarkus\n"},{"id":"200551","messageId":"7vy5jmxc55.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"20121004184314.GA15389@sigill.intra.peff.net","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-04T19:36:38Z","receivedAt":"2012-10-04T19:36:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n>\n>> with current trunk I get the following on an up-to-date Linux tree:\n>> \n>> markus@x4 linux % time git pull\n>> Already up-to-date.\n>> git pull 7.84s user 0.26s system 92% cpu 8.743 total\n>> \n>> git version 1.7.12 is much quicker:\n>> \n>> markus@x4 linux % time git pull\n>> Already up-to-date.\n>> git pull 0.10s user 0.02s system 16% cpu 0.740 total\n>\n> Yikes. I can easily reproduce here. Bisecting between master and\n> v1.7.12 gives a curious result: the slowdown first occurs with the merge\n> commit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\n> of its parents is slow. I don't see anything obviously suspect in the\n> merge, though.\n\nThanks; we probably should revert the merge before the final and\nhandle the fallout later, unless we know why the integrated whole is\nslower than its parts.\n"},{"id":"200566","messageId":"7vhaqaxawh.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"20121004184314.GA15389@sigill.intra.peff.net","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-04T20:03:26Z","receivedAt":"2012-10-04T20:03:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n>\n>> with current trunk I get the following on an up-to-date Linux tree:\n>> \n>> markus@x4 linux % time git pull\n>> Already up-to-date.\n>> git pull 7.84s user 0.26s system 92% cpu 8.743 total\n>> \n>> git version 1.7.12 is much quicker:\n>> \n>> markus@x4 linux % time git pull\n>> Already up-to-date.\n>> git pull 0.10s user 0.02s system 16% cpu 0.740 total\n>\n> Yikes. I can easily reproduce here. Bisecting between master and\n> v1.7.12 gives a curious result: the slowdown first occurs with the merge\n> commit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\n> of its parents is slow. I don't see anything obviously suspect in the\n> merge, though.\n\nThere is one extra call site of reduce_heads() added by that merge\nrelative to 34f5130. It is likely that the updated reduce_heads()\nis totally broken from performance point of view, or at least it is\nnot optimized for the usage pattern at the new call site, in which\ncase it would be very plausible that both parents of the merge\nperform well while the merge result is sucky.\n\nIt gets more curious, though.\n\nI am getting ~9 seconds with the tip of master and ~0.4 seconds with\n1.7.12.\n\nWhen 34f5130^2 is reverted at the tip of master, I get ~0.35\nseconds. That matches Markus's observation.\n\nHowever.\n\nIf I revert 5802f81 that updated the implementation of fmt-merge-msg\non top of 'master', *without* reverting 34f5130^2, I get ~4.5 seconds.\nAs we are doing an \"Already up-to-date\" pull, I thought there is no\nneed to call fmt-merge-msg in the first place?\n\nWhich may indicate that \"git merge\" has been broken for a long time\nand making unnecessary calls.\n\nHrmmm...\n"},{"id":"200542","messageId":"7v8vbmx90p.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"7vhaqaxawh.fsf@alter.siamese.dyndns.org","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-04T20:44:06Z","receivedAt":"2012-10-04T20:44:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It gets more curious, though.\n> ...\n> However.\n>\n> If I revert 5802f81 that updated the implementation of fmt-merge-msg\n> on top of 'master', *without* reverting 34f5130^2, I get ~4.5 seconds.\n> As we are doing an \"Already up-to-date\" pull, I thought there is no\n> need to call fmt-merge-msg in the first place?\n>\n> Which may indicate that \"git merge\" has been broken for a long time\n> and making unnecessary calls.\n>\n> Hrmmm...\n\nActually there is nothing curious about this. \"git pull\" prepares\nthe merge message before it calls \"git rebase\" or \"git merge\", and\nthere is no fast-path that detects \"Already up-to-date\" in it.\n"},{"id":"200557","messageId":"7v391ux7im.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"20121004184314.GA15389@sigill.intra.peff.net","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-04T21:16:33Z","receivedAt":"2012-10-04T21:16:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n>\n>> with current trunk I get the following on an up-to-date Linux tree:\n>> \n>> markus@x4 linux % time git pull\n>> Already up-to-date.\n>> git pull 7.84s user 0.26s system 92% cpu 8.743 total\n>> \n>> git version 1.7.12 is much quicker:\n>> \n>> markus@x4 linux % time git pull\n>> Already up-to-date.\n>> git pull 0.10s user 0.02s system 16% cpu 0.740 total\n>\n> Yikes. I can easily reproduce here. Bisecting between master and\n> v1.7.12 gives a curious result: the slowdown first occurs with the merge\n> commit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\n> of its parents is slow. I don't see anything obviously suspect in the\n> merge, though.\n\nI think the following is likely to be the correct solution to this.\n\n commit.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git i/commit.c w/commit.c\nindex 0246767..e1dd5b9 100644\n--- i/commit.c\n+++ w/commit.c\n@@ -733,6 +733,9 @@ static int remove_redundant(struct commit **array, int cnt)\n \tint *filled_index;\n \tint i, j, filled;\n \n+\tif (cnt < 2)\n+\t\treturn cnt;\n+\n \twork = xcalloc(cnt, sizeof(*work));\n \tredundant = xcalloc(cnt, 1);\n \tfilled_index = xmalloc(sizeof(*filled_index) * (cnt - 1));\n"},{"id":"200555","messageId":"7vvceqvses.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"7v391ux7im.fsf@alter.siamese.dyndns.org","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-04T21:28:11Z","receivedAt":"2012-10-04T21:28:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n>>\n>>> with current trunk I get the following on an up-to-date Linux tree:\n>>> \n>>> markus@x4 linux % time git pull\n>>> Already up-to-date.\n>>> git pull 7.84s user 0.26s system 92% cpu 8.743 total\n>>> \n>>> git version 1.7.12 is much quicker:\n>>> \n>>> markus@x4 linux % time git pull\n>>> Already up-to-date.\n>>> git pull 0.10s user 0.02s system 16% cpu 0.740 total\n>>\n>> Yikes. I can easily reproduce here. Bisecting between master and\n>> v1.7.12 gives a curious result: the slowdown first occurs with the merge\n>> commit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\n>> of its parents is slow. I don't see anything obviously suspect in the\n>> merge, though.\n>\n> I think the following is likely to be the correct solution to this.\n\nNo, it is not. Sorry for the noise.\n"},{"id":"200554","messageId":"7vmx01x3s4.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"7vvceqvses.fsf@alter.siamese.dyndns.org","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-04T22:37:15Z","receivedAt":"2012-10-04T22:37:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Jeff King writes:\n>>\n>>> On Thu, Oct 04, 2012 at 04:14:54PM +0200, Markus Trippelsdorf wrote:\n>>>\n>>>> with current trunk I get the following on an up-to-date Linux tree:\n>>>> \n>>>> markus@x4 linux % time git pull\n>>>> Already up-to-date.\n>>>> git pull 7.84s user 0.26s system 92% cpu 8.743 total\n>>>> \n>>>> git version 1.7.12 is much quicker:\n>>>> \n>>>> markus@x4 linux % time git pull\n>>>> Already up-to-date.\n>>>> git pull 0.10s user 0.02s system 16% cpu 0.740 total\n>>>\n>>> Yikes. I can easily reproduce here. Bisecting between master and\n>>> v1.7.12 gives a curious result: the slowdown first occurs with the merge\n>>> commit 34f5130 (Merge branch 'jc/merge-bases', 2012-09-11). But neither\n>>> of its parents is slow. I don't see anything obviously suspect in the\n>>> merge, though.\n>>\n>> I think the following is likely to be the correct solution to this.\n>\n> No, it is not. Sorry for the noise.\n\nHere is a tested (in the sense that it passes the test suite, and\nalso in the sense that an empty pull in the kernel history gives\nquick turnaround) patch. As I do not think we would want to revert\n5802f81 (fmt-merge-msg: discard needless merge parents, 2012-04-18)\nwhich was a correctness fix, I think we would rather want to do\nsomething like this.\n\n-- >8 --\nSubject: paint_down_to_common(): parse commit before relying on its timestamp\n\nWhen refactoring the merge-base computation to reduce the pairwise\nO(n*(n-1)) traversals to parallel O(n) traversals, the code forgot\nthat timestamp based heuristics needs each commit to have been\nparsed. This caused an empty \"git pull\" to spend cycles, traversing\nthe history all the way down to 0 (because an unparsed commit object\nhas 0 timestamp, and any other commit object with positive timestamp\nwill be processed for its parents, all getting parsed), only to come\nup with a merge message to be used.\n\nSigned-off-by: Junio C Hamano \n---\n commit.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git i/commit.c w/commit.c\nindex 0246767..213bc98 100644\n--- i/commit.c\n+++ w/commit.c\n@@ -609,6 +609,7 @@ static struct commit *interesting(struct commit_list *list)\n \treturn NULL;\n }\n \n+/* all input commits in one and twos[] must have been parsed! */\n static struct commit_list *paint_down_to_common(struct commit *one, int n, struct commit **twos)\n {\n \tstruct commit_list *list = NULL;\n@@ -617,6 +618,8 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc\n \n \tone->object.flags |= PARENT1;\n \tcommit_list_insert_by_date(one, &list);\n+\tif (!n)\n+\t\treturn list;\n \tfor (i = 0; i < n; i++) {\n \t\ttwos[i]->object.flags |= PARENT2;\n \t\tcommit_list_insert_by_date(twos[i], &list);\n@@ -737,6 +740,8 @@ static int remove_redundant(struct commit **array, int cnt)\n \tredundant = xcalloc(cnt, 1);\n \tfilled_index = xmalloc(sizeof(*filled_index) * (cnt - 1));\n \n+\tfor (i = 0; i < cnt; i++)\n+\t\tparse_commit(array[i]);\n \tfor (i = 0; i < cnt; i++) {\n \t\tstruct commit_list *common;\n \n"},{"id":"200638","messageId":"7vehlcu091.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"7vmx01x3s4.fsf@alter.siamese.dyndns.org","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-05T20:34:02Z","receivedAt":"2012-10-05T20:34:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Here is a tested (in the sense that it passes the test suite, and\n> also in the sense that an empty pull in the kernel history gives\n> quick turnaround) patch. As I do not think we would want to revert\n> 5802f81 (fmt-merge-msg: discard needless merge parents, 2012-04-18)\n> which was a correctness fix, I think we would rather want to do\n> something like this.\n\nOK, I think I am convinced myself that this patch is the right fix.\n\nThe performance regression Markus saw is in fmt-merge-message, and\nit is caused by the updated remove_redundant() that is used by\nget_merge_bases_many() and reduce_heads(). On the topic branch, all\ncallers of reduce_heads() were passing commits that are already\nparsed, but before the topic was merged to 'master', we added one\nmore caller to reduce_heads() on the 'master' front that passed an\nunparsed commit, which is why the problem surfaced at that merge.\n\nIt might make sense to assert or die in commit_list_insert_by_date()\nwhen a caller mistakenly pass an unparsed commit object to prevent\nthis kind of breakages in the future.\n\n> -- >8 --\n> Subject: paint_down_to_common(): parse commit before relying on its timestamp\n>\n> When refactoring the merge-base computation to reduce the pairwise\n> O(n*(n-1)) traversals to parallel O(n) traversals, the code forgot\n> that timestamp based heuristics needs each commit to have been\n> parsed. This caused an empty \"git pull\" to spend cycles, traversing\n> the history all the way down to 0 (because an unparsed commit object\n> has 0 timestamp, and any other commit object with positive timestamp\n> will be processed for its parents, all getting parsed), only to come\n> up with a merge message to be used.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> commit.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git i/commit.c w/commit.c\n> index 0246767..213bc98 100644\n> --- i/commit.c\n> +++ w/commit.c\n> @@ -609,6 +609,7 @@ static struct commit *interesting(struct commit_list *list)\n> \treturn NULL;\n> }\n> \n> +/* all input commits in one and twos[] must have been parsed! */\n> static struct commit_list *paint_down_to_common(struct commit *one, int n, struct commit **twos)\n> {\n> \tstruct commit_list *list = NULL;\n> @@ -617,6 +618,8 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc\n> \n> \tone->object.flags |= PARENT1;\n> \tcommit_list_insert_by_date(one, &list);\n> +\tif (!n)\n> +\t\treturn list;\n> \tfor (i = 0; i < n; i++) {\n> \t\ttwos[i]->object.flags |= PARENT2;\n> \t\tcommit_list_insert_by_date(twos[i], &list);\n> @@ -737,6 +740,8 @@ static int remove_redundant(struct commit **array, int cnt)\n> \tredundant = xcalloc(cnt, 1);\n> \tfilled_index = xmalloc(sizeof(*filled_index) * (cnt - 1));\n> \n> +\tfor (i = 0; i < cnt; i++)\n> +\t\tparse_commit(array[i]);\n> \tfor (i = 0; i < cnt; i++) {\n> \t\tstruct commit_list *common;\n> \n"},{"id":"200646","messageId":"20121005232108.GA7996@sigill.intra.peff.net","threadId":"31728","inReplyTo":"7vehlcu091.fsf@alter.siamese.dyndns.org","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-05T23:21:08Z","receivedAt":"2012-10-05T23:21:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 05, 2012 at 01:34:02PM -0700, Junio C Hamano wrote:\n\n> OK, I think I am convinced myself that this patch is the right fix.\n> \n> The performance regression Markus saw is in fmt-merge-message, and\n> it is caused by the updated remove_redundant() that is used by\n> get_merge_bases_many() and reduce_heads(). On the topic branch, all\n> callers of reduce_heads() were passing commits that are already\n> parsed, but before the topic was merged to 'master', we added one\n> more caller to reduce_heads() on the 'master' front that passed an\n> unparsed commit, which is why the problem surfaced at that merge.\n\nThanks for tracking it down. That makes a lot of sense with the results\nwe are seeing.\n\n> It might make sense to assert or die in commit_list_insert_by_date()\n> when a caller mistakenly pass an unparsed commit object to prevent\n> this kind of breakages in the future.\n\nI wonder if it would be too much to just have commit_list_insert_by_date\ncall parse_commit. It is, after all, the exact moment when we need to\nhave the date valid (and by waiting until the last minute, we can\npotentially avoid parses that would not otherwise need to happen). The\noverhead in the common case should basically be the same as an assert:\nchecking that commit->object.parsed is true (we can always inline that\nbit of parse_commit if we have to).\n\nOf course, in this case it is not just commit_list_insert_by_date that\ncares. paint_down_to_common also want commit->parents to be valid; I'm\nsurprised that dealing with unparsed commits did not also reveal an\nerror there.\n\nIn an object-oriented world, we would always get the attributes of a\ncommit through accessors that made sure the object was parsed. That\nwould be nicer, but it would also mean paying for the \"if (parsed)\"\nconditional a lot more frequently.\n\n> > @@ -617,6 +618,8 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc\n> > \n> > \tone->object.flags |= PARENT1;\n> > \tcommit_list_insert_by_date(one, &list);\n> > +\tif (!n)\n> > +\t\treturn list;\n> > \tfor (i = 0; i < n; i++) {\n> > \t\ttwos[i]->object.flags |= PARENT2;\n> > \t\tcommit_list_insert_by_date(twos[i], &list);\n\nThis seems like an obvious optimization, but does it really have\nanything to do with the patch at hand?\n\n-Peff\n"},{"id":"200648","messageId":"7vobkgrxay.fsf@alter.siamese.dyndns.org","threadId":"31728","inReplyTo":"20121005232108.GA7996@sigill.intra.peff.net","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-06T05:20:37Z","receivedAt":"2012-10-06T05:20:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > @@ -617,6 +618,8 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc\n>> > \n>> > \tone->object.flags |= PARENT1;\n>> > \tcommit_list_insert_by_date(one, &list);\n>> > +\tif (!n)\n>> > +\t\treturn list;\n>> > \tfor (i = 0; i < n; i++) {\n>> > \t\ttwos[i]->object.flags |= PARENT2;\n>> > \t\tcommit_list_insert_by_date(twos[i], &list);\n>\n> This seems like an obvious optimization, but does it really have\n> anything to do with the patch at hand?\n\nThe function picks one and paints it against all others, but the\nlogic assumes there must be at least one other to paint against;\notherwise the traversal will not ever find a node that is painted\nwith both PARENT1 and PARENT2 to stop, leading us to traverse all\nthe way down to root.\n"},{"id":"200662","messageId":"20121006125735.GA11712@sigill.intra.peff.net","threadId":"31728","inReplyTo":"7vobkgrxay.fsf@alter.siamese.dyndns.org","subject":"Re: git pull takes ~8 seconds on up-to-date Linux git tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-06T12:57:35Z","receivedAt":"2012-10-06T12:57:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 05, 2012 at 10:20:37PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> > @@ -617,6 +618,8 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc\n> >> > \n> >> > \tone->object.flags |= PARENT1;\n> >> > \tcommit_list_insert_by_date(one, &list);\n> >> > +\tif (!n)\n> >> > +\t\treturn list;\n> >> > \tfor (i = 0; i < n; i++) {\n> >> > \t\ttwos[i]->object.flags |= PARENT2;\n> >> > \t\tcommit_list_insert_by_date(twos[i], &list);\n> >\n> > This seems like an obvious optimization, but does it really have\n> > anything to do with the patch at hand?\n> \n> The function picks one and paints it against all others, but the\n> logic assumes there must be at least one other to paint against;\n> otherwise the traversal will not ever find a node that is painted\n> with both PARENT1 and PARENT2 to stop, leading us to traverse all\n> the way down to root.\n\nAh, OK. I was thinking it was just a way to skip the further logic,\nwhich would come to the same answer (it does, just not quickly). Makes\nsense.\n\n-Peff\n"}]}