Re: [PATCH 09/17] gc_boundary(): move the check "alloc <= nr" to caller

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2013, 18:02 UTC
Message-ID: <7vd2shfub9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <519DC0B7.7080401@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 6 quoted lines

> 1. The function name gc_boundary() suggests that it will do a garbage
> collection unconditionally.  In fact, it might or might not depending on
> this test.  At the caller, this made it look like a gc was happening
> each time through the loop, which made me nervous about the performance.
>  The new version makes it clear at the caller that the gc is only
> happening occasionally.

Perhaps.

Show 13 quoted lines

> 2. Even assuming that gc_boundary() were renamed to maybe_gc_boundary(),
> the function has hopelessly little information on which to base its
> decision whether or not to gc, and the choice of policy can only be
> justified based on some implicit knowledge about how the array is grown
> and shrunk.  But the growing/shrinking is done at the layer of the
> caller, and therefore the choice of gc policy also belongs at the layer
> of the caller.
>
> 3. As you point out, if the gc policy is ever to be made more
> intelligent, then gc_boundary() is unlikely to have enough information
> to implement the new policy (e.g., it would have no place to record
> low/high water marks).  Separating concerns at the correct level would
> make a change like that easier.

These two depend on how you look at the API hierarchy. You seem to think that the ideal end result is

	get_revision_internal()
          have an open coded "when to gc" logic in this function
          call object_array_filter()

My suggestion was based on a different view, which is:

	get_revision_internal()
          call gc_boundary()

        gc_boundary()
          make decision on when and how to gc
          if decided to do so
            call object_array_fitler()

You can obviously rename gc_boundary() to auto_gc_boundary() if that makes it easier to understand, but these two belong to the same codepath that deals with the object array used specifically for keeping track of boundary commits. I view "who has what information" as secondary---if the decision to gc is not the primary thing get_revision_internal() should be worrying about (and I do not think it is), it would be a better code structure to have a helper specific for doing so, i.e. gc_boundary(), and delegate that part of job to it.

Obviously, the caller needs to supply sufficient information to that helper if the helper needs more than what it currently gets by adding parameters to it, but that goes without saying.

> At the moment I am not interested in improving the gc policy of this
> code.

I am not either; the only effect we get from removing gc_boudnary() and calling directly to object_array_filter() is to lose the above abstraction and make it easy to let get_revision_internal() become more cluttered when somebody else later decides to improve the gc policy, it seems.