{"thread":{"id":"49528","subject":"[PATCH] gc: remove redundant check for gc_auto_threshold","startedAt":"2018-10-10T19:32:48Z","lastAt":"2018-10-13T07:56:48Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Brandon Casey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360061","messageId":"20181010193235.17359-1-avarab@gmail.com","threadId":"49528","inReplyTo":null,"subject":"[PATCH] gc: remove redundant check for gc_auto_threshold","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-10T19:32:35Z","receivedAt":"2018-10-10T19:32:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Checking gc_auto_threshold in too_many_loose_objects() was added in\n17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n2007-09-17) when need_to_gc() itself was also reliant on\ngc_auto_pack_limit before its early return:\n\n gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n\nWhen that check was simplified to just checking \"gc_auto_threshold <=\n0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\nassigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\nbeen removed. We only call too_many_loose_objects() from within\nneed_to_gc() itself, which will return if this condition holds, and in\ncmd_gc() which will return before ever getting to \"auto_gc &&\ntoo_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\nearlier in the function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nI had this in my tree as part of some general gc cleanups I was\nworking on, but since it's trivially considered as a stand-alone topic\nand unlikely to conflict with anything I or anyone else has planned\nI'm sending it as a one-off.\n\n builtin/gc.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2b592260e9..5f25a35dfc 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -157,9 +157,6 @@ static int too_many_loose_objects(void)\n \tint num_loose = 0;\n \tint needed = 0;\n \n-\tif (gc_auto_threshold <= 0)\n-\t\treturn 0;\n-\n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n \t\treturn 0;\n-- \n2.19.1.390.gf3a00b506f\n\n"},{"id":"360100","messageId":"CA+sFfMeOpj4V+KszboVVQSoaW2yDgpfDNGwK9ZrNfGmOOAhPtQ@mail.gmail.com","threadId":"49528","inReplyTo":"20181010193235.17359-1-avarab@gmail.com","subject":"Re: [PATCH] gc: remove redundant check for gc_auto_threshold","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2018-10-10T22:29:35Z","receivedAt":"2018-10-10T22:29:49Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Oct 10, 2018 at 12:32 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> Checking gc_auto_threshold in too_many_loose_objects() was added in\n> 17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n> 2007-09-17) when need_to_gc() itself was also reliant on\n> gc_auto_pack_limit before its early return:\n>\n> gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n>\n> When that check was simplified to just checking \"gc_auto_threshold <=\n> 0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\n> assigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\n> been removed. We only call too_many_loose_objects() from within\n> need_to_gc() itself, which will return if this condition holds, and in\n> cmd_gc() which will return before ever getting to \"auto_gc &&\n> too_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\n> earlier in the function.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> I had this in my tree as part of some general gc cleanups I was\n> working on, but since it's trivially considered as a stand-alone topic\n> and unlikely to conflict with anything I or anyone else has planned\n> I'm sending it as a one-off.\n\nHmm, yeah you're right that the check seems to be redundant for the\ncurrent uses of too_many_loose_objects(). I don't feel strongly about\nit, but I think an argument could be made that it makes sense for\ntoo_many_loose_object() and too_many_packs() to each inspect the\nconfiguration variable that controls them and detect when they're\ndisabled, rather than having one of them require that the check be\ndone beforehand. Again, I don't feel strongly about it, but I'm not\nsure this change actually improves the code.\n\n-Brandon\n"},{"id":"360109","messageId":"xmqqk1mpjru2.fsf@gitster-ct.c.googlers.com","threadId":"49528","inReplyTo":"CA+sFfMeOpj4V+KszboVVQSoaW2yDgpfDNGwK9ZrNfGmOOAhPtQ@mail.gmail.com","subject":"Re: [PATCH] gc: remove redundant check for gc_auto_threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-10T23:37:57Z","receivedAt":"2018-10-10T23:38:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> ... Again, I don't feel strongly about it, but I'm not\n> sure this change actually improves the code.\n\nYeah, in the context of the current caller, this is a safe change\nthat does not break anybody and reduces the number of instructions\nexecuted in this codepath. A mistaken caller may be added in the\nfuture that fails to check auto-threashold beforehand, but that\nwon't lead to anything bad like looping for a large number of times,\nso as long as the API contract into this helper function is clear\nthat callers are responsible to check beforehand, it is still not\ntoo bad.\n\nSo, I'd throw this into \"Meh - I won't regret applying it, but it is\nnot the end of the world if I forget to apply it, either\" pile.\n\nI _think_ a change that actually improves the code would be to\nrestructure so that there is a helper that is responsible for\nguestimating the number of loose objects, and another that uses the\nhelper to see if there are too many loose objects. The latter is\nthe only one tha needs to know about auto-threashold. But we are\nnot in immdiate need for such a clean-up, I guess, unless somebody\nis actively looking into revamping how auto-gc works and doing a\npreparatory clean-up.\n\n"},{"id":"360113","messageId":"CA+sFfMcrCZCJwzVDdyDFg8tb1g1N+2p8i556C-AhGSuHMJqLmw@mail.gmail.com","threadId":"49528","inReplyTo":"xmqqk1mpjru2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gc: remove redundant check for gc_auto_threshold","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2018-10-10T23:53:00Z","receivedAt":"2018-10-10T23:53:14Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Oct 10, 2018 at 4:38 PM Junio C Hamano wrote:\n>\n> Brandon Casey writes:\n>\n> > ... Again, I don't feel strongly about it, but I'm not\n> > sure this change actually improves the code.\n>\n> Yeah, in the context of the current caller, this is a safe change\n> that does not break anybody and reduces the number of instructions\n> executed in this codepath. A mistaken caller may be added in the\n> future that fails to check auto-threashold beforehand, but that\n> won't lead to anything bad like looping for a large number of times,\n> so as long as the API contract into this helper function is clear\n> that callers are responsible to check beforehand, it is still not\n> too bad.\n>\n> So, I'd throw this into \"Meh - I won't regret applying it, but it is\n> not the end of the world if I forget to apply it, either\" pile.\n>\n> I _think_ a change that actually improves the code would be to\n> restructure so that there is a helper that is responsible for\n> guestimating the number of loose objects, and another that uses the\n> helper to see if there are too many loose objects. The latter is\n> the only one tha needs to know about auto-threashold. But we are\n> not in immdiate need for such a clean-up, I guess, unless somebody\n> is actively looking into revamping how auto-gc works and doing a\n> preparatory clean-up.\n\nAgreed on all points, and as usual, said better than I could :-)\n\n-Brandon\n"},{"id":"360146","messageId":"87h8hsexdm.fsf@evledraar.gmail.com","threadId":"49528","inReplyTo":"xmqqk1mpjru2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gc: remove redundant check for gc_auto_threshold","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-11T07:49:25Z","receivedAt":"2018-10-11T07:49:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 10 2018, Junio C Hamano wrote:\n\n> Brandon Casey writes:\n>\n>> ... Again, I don't feel strongly about it, but I'm not\n>> sure this change actually improves the code.\n>\n> Yeah, in the context of the current caller, this is a safe change\n> that does not break anybody and reduces the number of instructions\n> executed in this codepath. A mistaken caller may be added in the\n> future that fails to check auto-threashold beforehand, but that\n> won't lead to anything bad like looping for a large number of times,\n> so as long as the API contract into this helper function is clear\n> that callers are responsible to check beforehand, it is still not\n> too bad.\n>\n> So, I'd throw this into \"Meh - I won't regret applying it, but it is\n> not the end of the world if I forget to apply it, either\" pile.\n>\n> I _think_ a change that actually improves the code would be to\n> restructure so that there is a helper that is responsible for\n> guestimating the number of loose objects, and another that uses the\n> helper to see if there are too many loose objects. The latter is\n> the only one tha needs to know about auto-threashold. But we are\n> not in immdiate need for such a clean-up, I guess, unless somebody\n> is actively looking into revamping how auto-gc works and doing a\n> preparatory clean-up.\n\nYeah, that's me :) I have some WIP gc cleanup, but want to sit on it a\nbit before I submit it to think about the best way to do things.\n\nSo in the meantime I was sending out a few WIP bits that I expected\ncould be reviewed stand-alone.\n\nSo I'd prefer to have this applied. It's easy enough to understand that\nshouldn't take long to prove to be correct & trickle down to \"master\",\nand will make those subsequent patches easier to follow.\n"},{"id":"360380","messageId":"xmqq8t329t52.fsf@gitster-ct.c.googlers.com","threadId":"49528","inReplyTo":"87h8hsexdm.fsf@evledraar.gmail.com","subject":"Re: [PATCH] gc: remove redundant check for gc_auto_threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-13T07:56:41Z","receivedAt":"2018-10-13T07:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a\n> bit before I submit it to think about the best way to do things.\n>\n> So in the meantime I was sending out a few WIP bits that I expected\n> could be reviewed stand-alone.\n\nI dunno. Unless the real body of the changes that \"depend\" on this\nsmall change comes before people forget the connection between them,\nI think it is detrimental to churn the codebase like this. If the\nreal body of the changes do not conflict with other topics in flight\nwhen it materializes, then having this small clean-up as a\npreparatory step in that real series would cost us nothing---that\nclean-up would not conflict with other things either. If the real\nthing would conflict and need to be adjusted to play well with other\ntopics before submission, having this small clean-up as a\npreparatory step in that real series would cost us nothing, either.\n"}]}