git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] gc: remove redundant check for gc_auto_threshold
@ 2018-10-10 19:32 Ævar Arnfjörð Bjarmason
  2018-10-10 22:29 ` Brandon Casey
  0 siblings, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-10-10 19:32 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Brandon Casey, Ævar Arnfjörð Bjarmason

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(-)

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;
-- 
2.19.1.390.gf3a00b506f


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gc: remove redundant check for gc_auto_threshold
  2018-10-10 19:32 [PATCH] gc: remove redundant check for gc_auto_threshold Ævar Arnfjörð Bjarmason
@ 2018-10-10 22:29 ` Brandon Casey
  2018-10-10 23:37   ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Brandon Casey @ 2018-10-10 22:29 UTC (permalink / raw)
  To: Ævar Arnfjörð; +Cc: git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gc: remove redundant check for gc_auto_threshold
  2018-10-10 22:29 ` Brandon Casey
@ 2018-10-10 23:37   ` Junio C Hamano
  2018-10-10 23:53     ` Brandon Casey
  2018-10-11  7:49     ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-10-10 23:37 UTC (permalink / raw)
  To: Brandon Casey; +Cc: Ævar Arnfjörð, git

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gc: remove redundant check for gc_auto_threshold
  2018-10-10 23:37   ` Junio C Hamano
@ 2018-10-10 23:53     ` Brandon Casey
  2018-10-11  7:49     ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 6+ messages in thread
From: Brandon Casey @ 2018-10-10 23:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ævar Arnfjörð, git

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gc: remove redundant check for gc_auto_threshold
  2018-10-10 23:37   ` Junio C Hamano
  2018-10-10 23:53     ` Brandon Casey
@ 2018-10-11  7:49     ` Ævar Arnfjörð Bjarmason
  2018-10-13  7:56       ` Junio C Hamano
  1 sibling, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-10-11  7:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Casey, git


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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gc: remove redundant check for gc_auto_threshold
  2018-10-11  7:49     ` Ævar Arnfjörð Bjarmason
@ 2018-10-13  7:56       ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-10-13  7:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Brandon Casey, git

Æ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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10 19:32 [PATCH] gc: remove redundant check for gc_auto_threshold Ævar Arnfjörð Bjarmason
2018-10-10 22:29 ` Brandon Casey
2018-10-10 23:37   ` Junio C Hamano
2018-10-10 23:53     ` Brandon Casey
2018-10-11  7:49     ` Ævar Arnfjörð Bjarmason
2018-10-13  7:56       ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox