| Message ID | 20181010193235.17359-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gc: remove redundant check for gc_auto_threshold | expand |
On Wed, Oct 10, 2018 at 12:32 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > Checking gc_auto_threshold in too_many_loose_objects() was added in > 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.", > 2007-09-17) when need_to_gc() itself was also reliant on > gc_auto_pack_limit before its early return: > > gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0 > > When that check was simplified to just checking "gc_auto_threshold <= > 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by > assigning 0 to gc.auto", 2008-03-19) this unreachable code should have > been removed. We only call too_many_loose_objects() from within > need_to_gc() itself, which will return if this condition holds, and in > cmd_gc() which will return before ever getting to "auto_gc && > too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true > earlier in the function. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > I had this in my tree as part of some general gc cleanups I was > working on, but since it's trivially considered as a stand-alone topic > and unlikely to conflict with anything I or anyone else has planned > I'm sending it as a one-off. Hmm, yeah you're right that the check seems to be redundant for the current uses of too_many_loose_objects(). I don't feel strongly about it, but I think an argument could be made that it makes sense for too_many_loose_object() and too_many_packs() to each inspect the configuration variable that controls them and detect when they're disabled, rather than having one of them require that the check be done beforehand. Again, I don't feel strongly about it, but I'm not sure this change actually improves the code. -Brandon
Brandon Casey <drafnel@gmail.com> writes: > ... Again, I don't feel strongly about it, but I'm not > sure this change actually improves the code. Yeah, in the context of the current caller, this is a safe change that does not break anybody and reduces the number of instructions executed in this codepath. A mistaken caller may be added in the future that fails to check auto-threashold beforehand, but that won't lead to anything bad like looping for a large number of times, so as long as the API contract into this helper function is clear that callers are responsible to check beforehand, it is still not too bad. So, I'd throw this into "Meh - I won't regret applying it, but it is not the end of the world if I forget to apply it, either" pile. I _think_ a change that actually improves the code would be to restructure so that there is a helper that is responsible for guestimating the number of loose objects, and another that uses the helper to see if there are too many loose objects. The latter is the only one tha needs to know about auto-threashold. But we are not in immdiate need for such a clean-up, I guess, unless somebody is actively looking into revamping how auto-gc works and doing a preparatory clean-up.
On Wed, Oct 10, 2018 at 4:38 PM Junio C Hamano <gitster@pobox.com> wrote: > > Brandon Casey <drafnel@gmail.com> writes: > > > ... Again, I don't feel strongly about it, but I'm not > > sure this change actually improves the code. > > Yeah, in the context of the current caller, this is a safe change > that does not break anybody and reduces the number of instructions > executed in this codepath. A mistaken caller may be added in the > future that fails to check auto-threashold beforehand, but that > won't lead to anything bad like looping for a large number of times, > so as long as the API contract into this helper function is clear > that callers are responsible to check beforehand, it is still not > too bad. > > So, I'd throw this into "Meh - I won't regret applying it, but it is > not the end of the world if I forget to apply it, either" pile. > > I _think_ a change that actually improves the code would be to > restructure so that there is a helper that is responsible for > guestimating the number of loose objects, and another that uses the > helper to see if there are too many loose objects. The latter is > the only one tha needs to know about auto-threashold. But we are > not in immdiate need for such a clean-up, I guess, unless somebody > is actively looking into revamping how auto-gc works and doing a > preparatory clean-up. Agreed on all points, and as usual, said better than I could :-) -Brandon
On Wed, Oct 10 2018, Junio C Hamano wrote: > Brandon Casey <drafnel@gmail.com> writes: > >> ... Again, I don't feel strongly about it, but I'm not >> sure this change actually improves the code. > > Yeah, in the context of the current caller, this is a safe change > that does not break anybody and reduces the number of instructions > executed in this codepath. A mistaken caller may be added in the > future that fails to check auto-threashold beforehand, but that > won't lead to anything bad like looping for a large number of times, > so as long as the API contract into this helper function is clear > that callers are responsible to check beforehand, it is still not > too bad. > > So, I'd throw this into "Meh - I won't regret applying it, but it is > not the end of the world if I forget to apply it, either" pile. > > I _think_ a change that actually improves the code would be to > restructure so that there is a helper that is responsible for > guestimating the number of loose objects, and another that uses the > helper to see if there are too many loose objects. The latter is > the only one tha needs to know about auto-threashold. But we are > not in immdiate need for such a clean-up, I guess, unless somebody > is actively looking into revamping how auto-gc works and doing a > preparatory clean-up. Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a bit before I submit it to think about the best way to do things. So in the meantime I was sending out a few WIP bits that I expected could be reviewed stand-alone. So I'd prefer to have this applied. It's easy enough to understand that shouldn't take long to prove to be correct & trickle down to "master", and will make those subsequent patches easier to follow.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a > bit before I submit it to think about the best way to do things. > > So in the meantime I was sending out a few WIP bits that I expected > could be reviewed stand-alone. I dunno. Unless the real body of the changes that "depend" on this small change comes before people forget the connection between them, I think it is detrimental to churn the codebase like this. If the real body of the changes do not conflict with other topics in flight when it materializes, then having this small clean-up as a preparatory step in that real series would cost us nothing---that clean-up would not conflict with other things either. If the real thing would conflict and need to be adjusted to play well with other topics before submission, having this small clean-up as a preparatory step in that real series would cost us nothing, either.
diff --git a/builtin/gc.c b/builtin/gc.c index 2b592260e9..5f25a35dfc 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -157,9 +157,6 @@ static int too_many_loose_objects(void) int num_loose = 0; int needed = 0; - if (gc_auto_threshold <= 0) - return 0; - dir = opendir(git_path("objects/17")); if (!dir) return 0;
Checking gc_auto_threshold in too_many_loose_objects() was added in 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.", 2007-09-17) when need_to_gc() itself was also reliant on gc_auto_pack_limit before its early return: gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0 When that check was simplified to just checking "gc_auto_threshold <= 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by assigning 0 to gc.auto", 2008-03-19) this unreachable code should have been removed. We only call too_many_loose_objects() from within need_to_gc() itself, which will return if this condition holds, and in cmd_gc() which will return before ever getting to "auto_gc && too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true earlier in the function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- I had this in my tree as part of some general gc cleanups I was working on, but since it's trivially considered as a stand-alone topic and unlikely to conflict with anything I or anyone else has planned I'm sending it as a one-off. builtin/gc.c | 3 --- 1 file changed, 3 deletions(-)