| 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
This series enables pack reverse-indexes to be written to disk by default instead of computed on-the-fly in memory. For repositories with large packs, this can have a significant benefit for both end-users and Git forges. Extensive performance tests appear in the fifth and sixth commit, but here are some highlights: - The time it takes to generate a pack containing objects for the 10 most-recent commits in linux.git (starting from 68047c48b228) drops from ~540ms to ~240ms, yielding a ~2.22x improvement. - The time it takes to compute the on-disk object size of a single object drops from ~300ms to 4ms, yielding a ~76.96x improvement. Reverse indexes are a trade-off between the time they take to be computed versus the time it takes to access a single record. In-memory reverse indexes take time to generate proportional to the number of packed objects in the repository, but have instantaneous lookup time. On-disk reverse indexes can be "generated" instantly (the time it takes to generate them is a one-time cost, loading them is instantaneous). But individual record lookup time is slower, since it involves multiple disk I/O operations. 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. 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. Thanks in advance for your review. I'm really excited to get this in the hands of users after a couple of years of running this at GitHub (and being opt-in otherwise). Taylor Blau (7): pack-write.c: plug a leak in stage_tmp_packfiles() t5325: mark as leak-free pack-revindex: make `load_pack_revindex` take a repository pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK pack-revindex: introduce `pack.readReverseIndex` config: enable `pack.writeReverseIndex` by default t: invert `GIT_TEST_WRITE_REV_INDEX` Documentation/config/pack.txt | 8 +++++++- builtin/index-pack.c | 5 +++-- builtin/pack-objects.c | 5 +++-- ci/run-build-and-tests.sh | 1 - pack-bitmap.c | 5 +++-- pack-revindex.c | 12 +++++++++--- pack-revindex.h | 6 ++++-- pack-write.c | 2 ++ packfile.c | 2 +- repo-settings.c | 1 + repository.h | 1 + t/README | 2 +- t/perf/p5312-pack-bitmaps-revs.sh | 3 +-- t/t5325-reverse-index.sh | 16 +++++++++++++++- 14 files changed, 51 insertions(+), 18 deletions(-)