Re: [PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables

[Patrick Steinhardt <ps@pks.im>]

On Sat, Dec 21, 2024 at 09:51:34AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> In order to compact tables we need at least two tables. Bail out early
> >> from `reftable_stack_auto_compact()` in case we have less than two
> >> tables.
> >
> > While that is very true, bailing out on "< 2" would change the
> > behaviour.  Where there were only one table, earlier we still went
> > ahead and exercised compaction code path, but now we no longer do.
> > The end result of having just a single table might logically be the
> > same with this change, but if we were relying on some side effects
> > of exercising the compaction code path, do we know that the rest of
> > the code is OK?
> >
> > That's the kind of questions I would ask, if this were somebody who
> > hasn't been deeply involved in the reftable code and came this deep
> > in the pre-release period.  But since we all know you have been the
> > main driver for this effort, we'd take your word for it ;-)
> >
> > Thanks, will queue.
> 
> And with code inspection, it can trivially seen that this change is
> perfectly fine.
> 
> In the original, when "== 1", stack_table_sizes_for_compaction(st)
> yields an array with a single element, suggest_compaction_segment()
> gives back segment with .start and .end both set to 0, for which
> segment_size() returns 0 hence we do not call stack_compact_range().
> The original code happens to do the same when "== 0" (and 0-sized
> allocation does not give back NULL).
> 
> So bypassing all of these when "== 1" is a no-op change, an
> optimization to avoid allocating a single-element array and then
> immediately freeing it.  Doing so when "== 0" is a strict
> improvement on a platform with malloc that returns NULL for 0-sized
> allocation.  So bypassing when "< 2" is totally safe and justifyable
> change.

Indeed. I'll include an explanation in v2 of this patch series.

Patrick