Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
> There is no logical reason for this test to be here. At the caller we
> might be able to figure out its meaning.
>
> Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
> ---
I do not think this change is justified, *unless* the caller later
in the series gains a better heuristics than what can be done with
information in the object_array (namely, alloc and nr) to decide
when to trigger gc.
And I was hoping to see such a cleverness added to the caller, but I
do not think I saw any.
I would have to say gc_boundary() knows better when it needs to gc
with the code at this point in the series, and that is true also in
the final code after all the patches in this series.
If we keep the "when to gc" logic inside "gc", in 11/17 this caller
can no longer call directly to object_array_filter(). It should
call gc_boundary(), but I see it as a merit, not a downside. The
"gc function can later be taught the high/low watermark logic you
alluded to in 10/17, and the growth/shrinkage characteristic you
would take advantage of while doing "gc" is specific to this
codepath. And the logic still does not have to have access to
anything only the caller has access to; "gc" can work on what can be
read from the object_array->{alloc,nr} that is given to it.
revision.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 8ac88d6..2e0992b 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2437,23 +2437,19 @@ static struct commit *get_revision_1(struct rev_info *revs)
>
> static void gc_boundary(struct object_array *array)
> {
> - unsigned nr = array->nr;
> - unsigned alloc = array->alloc;
> + unsigned nr = array->nr, i, j;
> struct object_array_entry *objects = array->objects;
>
> - if (alloc <= nr) {
> - unsigned i, j;
> - for (i = j = 0; i < nr; i++) {
> - if (objects[i].item->flags & SHOWN)
> - continue;
> - if (i != j)
> - objects[j] = objects[i];
> - j++;
> - }
> - for (i = j; i < nr; i++)
> - objects[i].item = NULL;
> - array->nr = j;
> + for (i = j = 0; i < nr; i++) {
> + if (objects[i].item->flags & SHOWN)
> + continue;
> + if (i != j)
> + objects[j] = objects[i];
> + j++;
> }
> + for (i = j; i < nr; i++)
> + objects[i].item = NULL;
> + array->nr = j;
> }
>
> static void create_boundary_commit_list(struct rev_info *revs)
> @@ -2577,7 +2573,8 @@ static struct commit *get_revision_internal(struct rev_info *revs)
> if (p->flags & (CHILD_SHOWN | SHOWN))
> continue;
> p->flags |= CHILD_SHOWN;
> - gc_boundary(&revs->boundary_commits);
> + if (revs->boundary_commits.alloc <= revs->boundary_commits.nr)
> + gc_boundary(&revs->boundary_commits);
> add_object_array(p, NULL, &revs->boundary_commits);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html