# Test failure in p5332-multi-pack-reuse.sh 6 messages from 2025-04-22 to 2025-05-01. Participants: Philippe Blain, Junio C Hamano, Jeff King, Taylor Blau. Thread: https://gitlist.dev/t/63323 ## Philippe Blain, 2025-04-22 02:01 Subject: Test failure in p5332-multi-pack-reuse.sh Message-ID: <292ae7a3-2aad-1f22-2afe-739ec921d6b7@gmail.com> URL: https://gitlist.dev/e/292ae7a3-2aad-1f22-2afe-739ec921d6b7%40gmail.com ``` Hi Taylor, I noticed that p5332-multi-pack-reuse.sh, which you added in ba47d88795 (t/perf: add performance tests for multi-pack reuse, 2023-12-14) fails early on in the second test ("setup bitmaps for 1-pack scenario"). Since perf tests run with '--immediate', I do not know if further tests in that file also fail. It is reproducible on macOS [1] as well as Linux [2] (I don't know if these logs are public though). I also tested on Linux on version 2.44.0 which is the first release in which this test was added, and it also failed similarily. Sidenote: on GitHub CI, I could not demonstrate the failure on Linux because all Linux jobs run in containers, and the images we use do not have Git installed, such that actions/checkout@v4 uses the GitHub API to download the repository instead of cloning it [3]. This leads die_if_build_dir_not_repo from perf-lib.sh to fail with "No $GIT_PERF_REPO defined, and your build directory is not a repo" [4]. We could fix that by installing the 'git' package before the 'actions/checkout' step, but we would need to account for the different package managers of the distros we test on. Cheers, Philippe. [1] https://github.com/phil-blain/git/actions/runs/14580975799/job/40897421311#step:4:896 [2] https://gitlab.com/phil-blain/git/-/jobs/9780586827#L2889 [3] https://github.com/phil-blain/git/actions/runs/14580975799/job/40897421399#step:4:28 [4] https://github.com/phil-blain/git/actions/runs/14580975799/job/40897421399#step:8:838 ``` ## Junio C Hamano, 2025-04-22 04:06 Subject: Re: Test failure in p5332-multi-pack-reuse.sh Message-ID: URL: https://gitlist.dev/e/xmqqcyd46dsb.fsf%40gitster.g In-Reply-To: <292ae7a3-2aad-1f22-2afe-739ec921d6b7@gmail.com> ``` Philippe Blain writes: > Sidenote: on GitHub CI, I could not demonstrate the failure on Linux > because all Linux jobs run in containers, and the images we use do > not have Git installed, such that actions/checkout@v4 uses the GitHub > API to download the repository instead of cloning it [3]. This leads > die_if_build_dir_not_repo from perf-lib.sh to fail with > "No $GIT_PERF_REPO defined, and your build directory is not a repo" [4]. > We could fix that by installing the 'git' package before the 'actions/checkout' > step, but we would need to account for the different package managers of > the distros we test on. Not limited to this topic, but wouldn't it make more sense to first run install-dependencies (including "/usr/bin/git") and then invoke the actions/checkout thing, I have to wonder. We were bitten by a separate topic due to the same issue quite recently. Thanks. ``` ## Jeff King, 2025-04-22 11:16 Subject: [PATCH] p5332: drop "+" from --stdin-packs input Message-ID: <20250422111632.GA1855088@coredump.intra.peff.net> URL: https://gitlist.dev/e/20250422111632.GA1855088%40coredump.intra.peff.net In-Reply-To: <292ae7a3-2aad-1f22-2afe-739ec921d6b7@gmail.com> ``` On Mon, Apr 21, 2025 at 10:01:25PM -0400, Philippe Blain wrote: > I noticed that p5332-multi-pack-reuse.sh, which you added in > ba47d88795 (t/perf: add performance tests for multi-pack reuse, > 2023-12-14) fails early on in the second test ("setup bitmaps for > 1-pack scenario"). Since perf tests run with '--immediate', I do not > know if further tests in that file also fail. It is reproducible on macOS [1] as > well as Linux [2] (I don't know if these logs are public though). > > I also tested on Linux on version 2.44.0 which is the first release > in which this test was added, and it also failed similarily. I think the patch below is probably the right solution. With it I got the output I'd expect (multi-pack reuse with many packs yields a CPU speedup at the cost of increased size): Test this tree ---------------------------------------------------------------------------------- 5332.3: clone for 1-pack scenario (single-pack reuse) 6.66(37.73+0.19) 5332.4: clone size for 1-pack scenario (single-pack reuse) 117.0M 5332.5: clone for 1-pack scenario (multi-pack reuse) 6.89(38.71+0.25) 5332.6: clone size for 1-pack scenario (multi-pack reuse) 117.0M 5332.9: clone for 10-pack scenario (single-pack reuse) 5.67(35.65+0.37) 5332.10: clone size for 10-pack scenario (single-pack reuse) 125.1M 5332.11: clone for 10-pack scenario (multi-pack reuse) 2.47(5.71+0.15) 5332.12: clone size for 10-pack scenario (multi-pack reuse) 134.3M 5332.15: clone for 100-pack scenario (single-pack reuse) 14.50(130.54+0.55) 5332.16: clone size for 100-pack scenario (single-pack reuse) 224.2M 5332.17: clone for 100-pack scenario (multi-pack reuse) 3.34(3.69+0.18) 5332.18: clone size for 100-pack scenario (multi-pack reuse) 307.3M -- >8 -- Subject: [PATCH] p5332: drop "+" from --stdin-packs input This perf script creates a midx by running "git multi-pack-index write" with the "--stdin-packs" option. We feed that stdin by running "find" on .git/objects/pack, using sed to strip off everything but the basename. But that sed invocation also does something peculiar: it adds a "+" to the start of each pack name. This causes the multi-pack-index command to barf. The modified name does not match any pack it knows about, so it ends up with an empty list of packs to put in the midx. And thus nothing matches the --preferred-pack option we pass, which causes it die(). The fix is to remove the extra "+" (which also lets us simplify the sed invocation a bit, as it is now just stripping the leading directories). But that leaves the mystery of why it was ever there in the first place. The answer is that an earlier iteration of the patch series had a concept of "disjoint" packs in the midx. And one of its patches here: https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/ taught read_packs_from_stdin() to treat a leading "+" as marking a disjoint pack. But in the second version of the series, which was ultimately merged, that disjoint concept went away, and the code to parse "+" did likewise. The regular regression tests were adjusted to match, but this case in t/perf was forgotten. Signed-off-by: Jeff King --- t/perf/p5332-multi-pack-reuse.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/perf/p5332-multi-pack-reuse.sh b/t/perf/p5332-multi-pack-reuse.sh index d1c89a8b7d..0a2525db44 100755 --- a/t/perf/p5332-multi-pack-reuse.sh +++ b/t/perf/p5332-multi-pack-reuse.sh @@ -58,7 +58,7 @@ do ' test_expect_success "setup bitmaps for $nr_packs-pack scenario" ' - find $packdir -type f -name "*.idx" | sed -e "s/.*\/\(.*\)$/+\1/g" | + find $packdir -type f -name "*.idx" | sed -e "s/.*\///" | git multi-pack-index write --stdin-packs --bitmap \ --preferred-pack="$(find_pack $(git rev-parse HEAD))" ' -- 2.49.0.682.g886cb1c59a ``` ## Junio C Hamano, 2025-04-22 15:49 Subject: Re: [PATCH] p5332: drop "+" from --stdin-packs input Message-ID: URL: https://gitlist.dev/e/xmqqv7qw42na.fsf%40gitster.g In-Reply-To: <20250422111632.GA1855088@coredump.intra.peff.net> ``` Jeff King writes: > > -- >8 -- > Subject: [PATCH] p5332: drop "+" from --stdin-packs input > > This perf script creates a midx by running "git multi-pack-index write" > with the "--stdin-packs" option. We feed that stdin by running "find" on > .git/objects/pack, using sed to strip off everything but the basename. > > But that sed invocation also does something peculiar: it adds a "+" to > the start of each pack name. This causes the multi-pack-index command to > barf. The modified name does not match any pack it knows about, so it > ends up with an empty list of packs to put in the midx. And thus nothing > matches the --preferred-pack option we pass, which causes it die(). > > The fix is to remove the extra "+" (which also lets us simplify the sed > invocation a bit, as it is now just stripping the leading directories). > > But that leaves the mystery of why it was ever there in the first place. > The answer is that an earlier iteration of the patch series had a > concept of "disjoint" packs in the midx. And one of its patches here: > > https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/ > > taught read_packs_from_stdin() to treat a leading "+" as marking a > disjoint pack. But in the second version of the series, which was > ultimately merged, that disjoint concept went away, and the code to > parse "+" did likewise. The regular regression tests were adjusted to > match, but this case in t/perf was forgotten. > > Signed-off-by: Jeff King > --- > t/perf/p5332-multi-pack-reuse.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks. I wonder if we had some tools and mechanisms people were discussing to track changes on changsets, such a mishap could have been caught more easily. [jc: random folks from that discussion CC'ed, just in case they are interested]. > > diff --git a/t/perf/p5332-multi-pack-reuse.sh b/t/perf/p5332-multi-pack-reuse.sh > index d1c89a8b7d..0a2525db44 100755 > --- a/t/perf/p5332-multi-pack-reuse.sh > +++ b/t/perf/p5332-multi-pack-reuse.sh > @@ -58,7 +58,7 @@ do > ' > > test_expect_success "setup bitmaps for $nr_packs-pack scenario" ' > - find $packdir -type f -name "*.idx" | sed -e "s/.*\/\(.*\)$/+\1/g" | > + find $packdir -type f -name "*.idx" | sed -e "s/.*\///" | > git multi-pack-index write --stdin-packs --bitmap \ > --preferred-pack="$(find_pack $(git rev-parse HEAD))" > ' ``` ## Taylor Blau, 2025-04-22 17:24 Subject: Re: [PATCH] p5332: drop "+" from --stdin-packs input Message-ID: URL: https://gitlist.dev/e/aAfQwrhuLF7BysyE%40nand.local In-Reply-To: <20250422111632.GA1855088@coredump.intra.peff.net> ``` On Tue, Apr 22, 2025 at 07:16:32AM -0400, Jeff King wrote: > --- > t/perf/p5332-multi-pack-reuse.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) My apologies for the mistake in the first place, but thank you for digging and providing the fix. Acked-by: Taylor Blau Thanks, Taylor ``` ## Jeff King, 2025-05-01 16:03 Subject: Re: [PATCH] p5332: drop "+" from --stdin-packs input Message-ID: <20250501160344.GA1794891@coredump.intra.peff.net> URL: https://gitlist.dev/e/20250501160344.GA1794891%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Apr 22, 2025 at 08:49:45AM -0700, Junio C Hamano wrote: > > The fix is to remove the extra "+" (which also lets us simplify the sed > > invocation a bit, as it is now just stripping the leading directories). > > > > But that leaves the mystery of why it was ever there in the first place. > > The answer is that an earlier iteration of the patch series had a > > concept of "disjoint" packs in the midx. And one of its patches here: > > > > https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/ > > > > taught read_packs_from_stdin() to treat a leading "+" as marking a > > disjoint pack. But in the second version of the series, which was > > ultimately merged, that disjoint concept went away, and the code to > > parse "+" did likewise. The regular regression tests were adjusted to > > match, but this case in t/perf was forgotten. > > > > Signed-off-by: Jeff King > > --- > > t/perf/p5332-multi-pack-reuse.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks. I wonder if we had some tools and mechanisms people were > discussing to track changes on changsets, such a mishap could have > been caught more easily. [jc: random folks from that discussion > CC'ed, just in case they are interested]. IMHO it would probably not help that much, because the error was in the other direction. I.e., the issue was that something _didn't_ change between two versions of the series, but should have. In general I think we'd usually rely on tests or compiler analysis (e.g., leftover unused variables) to catch this kind of thing. It's just that hardly anybody actually runs the perf tests. I know there was some discussion about running them regularly in CI, but I'm skeptical that the CPU time / utility tradeoff is very good there. In some sense, this case was the process working as designed. Running the test _did_ catch the problem, but we didn't notice because nobody ran it for a while. So in the interim, nobody was hurt. ;) I'm mostly joking. It is certainly more convenient to catch these things earlier, but latent buggy code that nobody runs might not be that big a worry. -Peff ```