| Message ID | 040bb40548017bae807c1d349fa078c21ac46725.1631657157.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-bitmap: permute existing namehash values | expand |
On Tue, Sep 14, 2021 at 06:06:14PM -0400, Taylor Blau wrote: > To help test the performance of permuting the contents of the hash-cache > when generating a MIDX bitmap, we need a bitmap which has its hash-cache > populated. > > And since multi-pack bitmaps don't add *new* values to the hash-cache, > we have to rely on a single-pack bitmap to generate those values for us. > > Therefore, generate a pack bitmap before the MIDX one in order to ensure > that the MIDX bitmap has entries in its hash-cache. Makes sense. This is a little more contrived of a setup than the original, but an utterly realistic one. If you are using midx bitmaps, you are probably interspersing them with occasional full pack bitmaps. It might be interesting to also do: rm -f .git/objects/pack/pack-*.bitmap after generating the midx bitmap. That would confirm the further timing tests are using the midx bitmap, and not ever "cheating" by looking at the pack one (having poked in this direction before, I know that this all works, so it would be a future-proofing thing). > diff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh > index a9c5499537..38557859b7 100755 > --- a/t/perf/p5326-multi-pack-bitmaps.sh > +++ b/t/perf/p5326-multi-pack-bitmaps.sh > @@ -13,7 +13,7 @@ test_expect_success 'create tags' ' > ' > > test_perf 'setup multi-pack index' ' > - git repack -ad && > + git repack -adb && > git multi-pack-index write --bitmap > ' This sort-of existed before your series, but I think is a bit "worse" now: we are timing both "repack" and "multi-pack-index" write together. So: - the timing for the midx write that we are interested in timing will be diluted by the much-bigger full-repack - we'll actually do _three_ full repacks (the default GIT_PERF_REPEAT_COUNT for the "run" script), since it's inside a test_perf() So: test_expect_success 'start with bitmapped pack' ' git repack -adb ' test_perf 'setup multi-pack index' ' git multi-pack-index write --bitmap ' would run faster and give us more interesting timings. Possibly you'd want to drop the midx and its bitmaps as part of that test_perf, too. The first run will be using the pack bitmap, and the others will use the midx. I doubt it makes much difference either way, though. And of course if you want to take my earlier suggestion, then it's easy to add: test_expect_success 'drop pack bitmap' ' rm -f .git/objects/pack/pack-*.bitmap ' afterwards; you wouldn't want to do it inside the test_perf() call because of the repeat-count. -Peff
On Thu, Sep 16, 2021 at 06:45:33PM -0400, Jeff King wrote: > It might be interesting to also do: > > rm -f .git/objects/pack/pack-*.bitmap > > after generating the midx bitmap. That would confirm the further timing > tests are using the midx bitmap, and not ever "cheating" by looking at > the pack one (having poked in this direction before, I know that this > all works, so it would be a future-proofing thing). This and the suggestion to avoid timing the single pack bitmap generation are both good ones. I think to be totally accurate we would want to drop the pack bitmap before every MIDX bitmap generation, but I also think that in practice it does not matter much. The reuse code currently does not try too hard to recognize the situation of "oh, I selected the same exact commits and don't have to do any work". It kind of does eventually, but only after doing a lot of preparation. So I'm dubious as to whether we're really timing anything *that* useful, but it's probably worth keeping around anyway. Thanks, Taylor
diff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh index a9c5499537..38557859b7 100755 --- a/t/perf/p5326-multi-pack-bitmaps.sh +++ b/t/perf/p5326-multi-pack-bitmaps.sh @@ -13,7 +13,7 @@ test_expect_success 'create tags' ' ' test_perf 'setup multi-pack index' ' - git repack -ad && + git repack -adb && git multi-pack-index write --bitmap '
To help test the performance of permuting the contents of the hash-cache when generating a MIDX bitmap, we need a bitmap which has its hash-cache populated. And since multi-pack bitmaps don't add *new* values to the hash-cache, we have to rely on a single-pack bitmap to generate those values for us. Therefore, generate a pack bitmap before the MIDX one in order to ensure that the MIDX bitmap has entries in its hash-cache. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/perf/p5326-multi-pack-bitmaps.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)