| Message ID | e7ef420f321b3936185b2729460b1c28f5384438.1658342304.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bitmap: integrate a lookup table extension to the bitmap format | expand |
On Wed, Jul 20, 2022 at 06:38:23PM +0000, Abhradeep Chakraborty via GitGitGadget wrote: > From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> > > Enable `pack.writeReverseIndex` before running pack-bitmap related > performance tests. > > The performance difference with `pack.writeReverseIndex` enabled and > with disabled are given below - Thanks; this order of changes in the t/perf suite makes sense to me. One note, this sort of change where we're comparing all of the tests in a single t/perf file against themselves before and after some change, it is helpful to do (in t/perf) ./run HEAD . p5310-pack-bitmaps.sh which compares HEAD to what's in the current tree. You'll get the results side-by-side, which makes them a little easier to scan. You can also aggregate results together from multiple runs with the t/perf/aggregate.perl script. One gotcha (that has often bitten me in the past) is that when running the perf suite with `.` as your build target, it uses whatever git binary is sitting in your tree. So make sure that it is both (a) up-to-date, ie., that it is the result of compiling what's currently in your tree, and (b) that it is compiled with the same settings as what you built HEAD with. I have often scratched my head at why the result of running some perf suite on '.' seems much slower than it should be, only to realize that the "git" binary sitting in my tree was built with -O0 or something. Thanks, Taylor
On Mon, Jul 25 2022, Taylor Blau wrote: > On Wed, Jul 20, 2022 at 06:38:23PM +0000, Abhradeep Chakraborty via GitGitGadget wrote: >> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> >> >> Enable `pack.writeReverseIndex` before running pack-bitmap related >> performance tests. >> >> The performance difference with `pack.writeReverseIndex` enabled and >> with disabled are given below - > > Thanks; this order of changes in the t/perf suite makes sense to me. One > note, this sort of change where we're comparing all of the tests in a > single t/perf file against themselves before and after some change, it > is helpful to do (in t/perf) > > ./run HEAD . p5310-pack-bitmaps.sh > > which compares HEAD to what's in the current tree. You'll get the > results side-by-side, which makes them a little easier to scan. You can > also aggregate results together from multiple runs with the > t/perf/aggregate.perl script. > > One gotcha (that has often bitten me in the past) is that when running > the perf suite with `.` as your build target, it uses whatever git > binary is sitting in your tree. So make sure that it is both (a) > up-to-date, ie., that it is the result of compiling what's currently in > your tree, and (b) that it is compiled with the same settings as what > you built HEAD with. > > I have often scratched my head at why the result of running some perf > suite on '.' seems much slower than it should be, only to realize that > the "git" binary sitting in my tree was built with -O0 or something. Rather than comparing HEAD to your current tree it's generally better to do something like: GIT_PERF_MAKE_OPTS='-j3' ./run HEAD~ HEAD [...]
On 7/26/2022 3:15 AM, Ævar Arnfjörð Bjarmason wrote: > Rather than comparing HEAD to your current tree it's generally better > to do something like: > > GIT_PERF_MAKE_OPTS='-j3' ./run HEAD~ HEAD [...] Using the 'run' script fixes the perf test in the worktree and tests different versions of the 'git' executable. That doesn't work when the change is in the performance test itself. Thanks, -Stolee
On Tue, Jul 26 2022, Derrick Stolee wrote: > On 7/26/2022 3:15 AM, Ævar Arnfjörð Bjarmason wrote: >> Rather than comparing HEAD to your current tree it's generally better >> to do something like: >> >> GIT_PERF_MAKE_OPTS='-j3' ./run HEAD~ HEAD [...] > > Using the 'run' script fixes the perf test in the worktree and tests > different versions of the 'git' executable. > > That doesn't work when the change is in the performance test itself. Thanks, I'm clearly wrong about that. I didn't look enough at the context. But then we're losing the perf test coverage for the case where we don't have the *.rev files. Isn't it better to run both with & without *.rev, perhaps by splitting up the test file? We could make it a function in perf/lib-bitmap.sh that we call both with & without the wanted *.rev repack config. I suspect that's also subtly broken, in that t/perf assumes that it can re-use the repo for a given <rev>, but this is modifying that repo, so if you run e.g. test Y after this Y, that Y will unexpectedly get a repack'd repo ... But we could just start the test with a git clone . "$TEST_NAME" or whatever, then repack that with whatever options we want...
On Tue, Jul 26, 2022 at 7:26 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > But then we're losing the perf test coverage for the case where we don't > have the *.rev files. Isn't it better to run both with & without *.rev, > perhaps by splitting up the test file? We could make it a function in > perf/lib-bitmap.sh that we call both with & without the wanted *.rev > repack config. Ok. > I suspect that's also subtly broken, in that t/perf assumes that it can > re-use the repo for a given <rev>, but this is modifying that repo, so > if you run e.g. test Y after this Y, that Y will unexpectedly get a > repack'd repo ... Thanks Ævar! This is the problem that I informed Taylor off-list. Will update it. > But we could just start the test with a git clone . "$TEST_NAME" or > whatever, then repack that with whatever options we want... Thanks :)
diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh index 7ad4f237bc3..6e8abcd5b21 100755 --- a/t/perf/p5310-pack-bitmaps.sh +++ b/t/perf/p5310-pack-bitmaps.sh @@ -13,7 +13,8 @@ test_perf_large_repo # We intentionally use the deprecated pack.writebitmaps # config so that we can test against older versions of git. test_expect_success 'setup bitmap config' ' - git config pack.writebitmaps true + git config pack.writebitmaps true && + git config pack.writeReverseIndex true ' # we need to create the tag up front such that it is covered by the repack and