Message ID | cover.1681166596.git.me@ttaylorr.com (mailing list archive) |
---|---|
Headers | show |
Series | pack-revindex: enable on-disk reverse indexes by default | expand |
On 4/10/2023 6:53 PM, Taylor Blau wrote: > 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. > 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
On Tue, Apr 11, 2023 at 09:54:08AM -0400, Derrick Stolee wrote: > On 4/10/2023 6:53 PM, Taylor Blau wrote: > > 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. Sweet; I'm glad that you agree. FWIW for non-GitHub folks, observing a slow-down here has never been an issue for us. So much so that I wrote the pack.readReverseIndex knob yesterday for the purpose of sending this series. That said, I think that including it here is still worthwhile, since the cases where performance really suffers (e.g., `git cat-file --batch-all-objects --batch-check='%(objectsize:disk)'`) isn't something that GitHub runs regularly if at all. To accommodate different workflows, I think having the option to opt-out of reading the on-disk ".rev" files is worthwhile. > 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 for taking a look. Based on your review, there are only a couple of things on my mind: - I amended the last patch to more clearly state when we would want to run the suite GIT_TEST_NO_WRITE_REV_INDEXES=1 set, and kept it in the linux-TEST-vars configuration. - How do you want to handle that extra patch? As I noted in [1], I think squashing the two together in one way or another makes sense. So really we have to figure out (a) if you think that is the right way to go, and (b) if so, how to handle attribution / the commit message. Thanks, Taylor [1]: https://lore.kernel.org/git/ZDXRajRky5XtFenU@nand.local/
On 4/11/2023 5:40 PM, Taylor Blau wrote: > On Tue, Apr 11, 2023 at 09:54:08AM -0400, Derrick Stolee wrote: >> On 4/10/2023 6:53 PM, Taylor Blau wrote: >>> 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. > > Sweet; I'm glad that you agree. > > FWIW for non-GitHub folks, observing a slow-down here has never been an > issue for us. So much so that I wrote the pack.readReverseIndex knob > yesterday for the purpose of sending this series. > > That said, I think that including it here is still worthwhile, since the > cases where performance really suffers (e.g., `git cat-file > --batch-all-objects --batch-check='%(objectsize:disk)'`) isn't something > that GitHub runs regularly if at all. > > To accommodate different workflows, I think having the option to opt-out > of reading the on-disk ".rev" files is worthwhile. The only thing I can think of that would actually use this kind of behavior is git-sizer, but even that doesn't actually report the on-disk size (yet) and instead inflates all deltas when reporting size counts. The difference in performance here is likely minimal for that tool. >> 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 for taking a look. Based on your review, there are only a couple > of things on my mind: > > - I amended the last patch to more clearly state when we would want to > run the suite GIT_TEST_NO_WRITE_REV_INDEXES=1 set, and kept it in > the linux-TEST-vars configuration. I think this is the right thing to do. Thanks. > - How do you want to handle that extra patch? As I noted in [1], I > think squashing the two together in one way or another makes sense. > So really we have to figure out (a) if you think that is the right > way to go, and (b) if so, how to handle attribution / the commit > message. Squashing makes sense. You could make me a co-author, or not. It's the natural thing to do once the problem to solve is identified. Thanks, -Stolee