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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 21, 2024, 17:51 UTC
Message-ID: <xmqqseqgc4bd.fsf@gitster.g>
In-Reply-To: <xmqqmsgpc6bf.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

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

Thanks.