Re: [PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 11, 2023, 13:54 UTC
Message-ID: <bfa4c23d-8b38-7505-fb57-a02e0a028292@github.com>
In-Reply-To: <cover.1681166596.git.me@ttaylorr.com>

On 4/10/2023 6:53 PM, Taylor Blau wrote:

Show 5 quoted lines

> In the vast majority of cases, this trade-off favors the on-disk ".rev"
> files. But in certain cases, the in-memory variant performs more
> favorably. Since these cases are narrow, and performance is machine- and
> repository-dependent, this series also introduces a new configuration
> option to disable reading ".rev" files in the third commit.

I agree that the performance trade-off indicates that having the .rev
files is preferred. It makes operations that _can_ be very fast as fast
as possible (inspecting a small number of objects is much faster because
we don't generate the in-memory index in O(N) time) and you create a knob
for disabling it in the case that we are already doing something that
inspects almost-all objects.

Show 8 quoted lines

> The series is structured as follows:
> 
>   - A couple of cleanup patches to plug a leak in stage_tmp_packfiles().
>   - Three patches to enable `pack.readReverseIndex`.
>   - Another patch to change the default of `pack.writeReverseIndex` from
>     "false" to "true".
>   - A final patch to enable the test suite to be run in a mode that does
>     not use on-disk ".rev" files.

This was an easy series to read. I applied the patches locally and poked around in the resulting code as I went along. This led to a couple places where I recommend a few changes, including a new patch that wires repository pointers through a few more method layers.

Thanks, -Stolee