{"thread":{"id":"53104","subject":"[PATCH] p5310: stop timing non-bitmap pack-to-disk","startedAt":"2020-03-26T07:54:40Z","lastAt":"2020-03-26T11:12:06Z","messageCount":3,"participants":["Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394058","messageId":"20200326075436.GA2199958@coredump.intra.peff.net","threadId":"53104","inReplyTo":null,"subject":"[PATCH] p5310: stop timing non-bitmap pack-to-disk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-26T07:54:36Z","receivedAt":"2020-03-26T07:54:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 645c432d61 (pack-objects: use reachability bitmap index when\ngenerating non-stdout pack, 2016-09-10) added two timing tests for\npacking to an on-disk file, both with and without bitmaps. However, the\nnon-bitmap one isn't interesting to have as part of p5310's regression\nsuite. It _could_ be used as a baseline to show off the improvement in\nthe bitmap case, but:\n\n  - the point of the t/perf suite is to find performance regressions,\n    and it won't help with that. We don't compare the numbers between\n    two tests (which the perf suite has no idea are even related), and\n    any change in its numbers would have nothing to do with bitmaps.\n\n  - it did show off the improvement in the commit message of 645c432d61,\n    but it wasn't even necessary there. The bitmap case already shows an\n    improvement (because before the patch, it behaved the same as the\n    non-bitmap case), and the perf suite is even able to show the\n    difference between the before and after measurements.\n\nOn top of that, it's one of the most expensive tests in the suite,\nclocking in around 60s for linux.git on my machine (as compared to 16s\nfor the bitmapped version). And by default when using \"./run\", we'd run\nit three times!\n\nSo let's just drop it. It's not useful and is adding minutes to perf\nruns.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI happened to be running p5310 today and was annoyed by this.\n\nThe other really expensive part is the actual \"git repack\" to generate\nthe bitmaps. We can't avoid running that (since we need the bitmaps it\ngenerates), but it's mildly annoying that it runs all three times.\nHowever, I think it's worth keeping as a timed test, as it would help us\nfind a performance regression in the bitmap generation code. It's too\nbad we have to do the full repack for each trial, but we don't have a\nway to just write bitmaps for an existing pack (it's conceptually\nsimple, but the code is pretty tied into pack-objects).\n\n t/perf/p5310-pack-bitmaps.sh | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh\nindex 7743f4f4c9..80c53edca7 100755\n--- a/t/perf/p5310-pack-bitmaps.sh\n+++ b/t/perf/p5310-pack-bitmaps.sh\n@@ -31,10 +31,6 @@ test_perf 'simulated fetch' '\n \t} | git pack-objects --revs --stdout >/dev/null\n '\n \n-test_perf 'pack to file' '\n-\tgit pack-objects --all pack1 </dev/null >/dev/null\n-'\n-\n test_perf 'pack to file (bitmap)' '\n \tgit pack-objects --use-bitmap-index --all pack1b </dev/null >/dev/null\n '\n-- \n2.26.0.576.gb87790c3c1\n"},{"id":"394081","messageId":"cf64c47a-481e-3802-3ce6-a2a583cd7c75@gmail.com","threadId":"53104","inReplyTo":"20200326075436.GA2199958@coredump.intra.peff.net","subject":"Re: [PATCH] p5310: stop timing non-bitmap pack-to-disk","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-26T10:48:37Z","receivedAt":"2020-03-26T10:48:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/26/2020 3:54 AM, Jeff King wrote:\n> Commit 645c432d61 (pack-objects: use reachability bitmap index when\n> generating non-stdout pack, 2016-09-10) added two timing tests for\n> packing to an on-disk file, both with and without bitmaps. However, the\n> non-bitmap one isn't interesting to have as part of p5310's regression\n> suite. It _could_ be used as a baseline to show off the improvement in\n> the bitmap case, but:\n> \n>   - the point of the t/perf suite is to find performance regressions,\n>     and it won't help with that. We don't compare the numbers between\n>     two tests (which the perf suite has no idea are even related), and\n>     any change in its numbers would have nothing to do with bitmaps.\n\nThis does make me think if there is a way to adjust \"./run\" to test two\ndifferent config settings or command-line options instead of just two\ndifferent build versions. Perhaps something like\n\n ./run \"HEAD -c core.commitGraph=true\" \"HEAD -c core.commitGraph=false\" -- p4200-line-log.sh\n\nBut that's just musing on my part.\n\n>   - it did show off the improvement in the commit message of 645c432d61,\n>     but it wasn't even necessary there. The bitmap case already shows an\n>     improvement (because before the patch, it behaved the same as the\n>     non-bitmap case), and the perf suite is even able to show the\n>     difference between the before and after measurements.\n> \n> On top of that, it's one of the most expensive tests in the suite,\n> clocking in around 60s for linux.git on my machine (as compared to 16s\n> for the bitmapped version). And by default when using \"./run\", we'd run\n> it three times!\n> \n> So let's just drop it. It's not useful and is adding minutes to perf\n> runs.\n\nI agree with your reasoning. This is not a critical path for clients,\nand all servers should be using bitmaps.\n\nThanks,\n-Stolee\n"},{"id":"394083","messageId":"20200326111201.GA4193632@coredump.intra.peff.net","threadId":"53104","inReplyTo":"cf64c47a-481e-3802-3ce6-a2a583cd7c75@gmail.com","subject":"Re: [PATCH] p5310: stop timing non-bitmap pack-to-disk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-26T11:12:01Z","receivedAt":"2020-03-26T11:12:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 26, 2020 at 06:48:37AM -0400, Derrick Stolee wrote:\n\n> >   - the point of the t/perf suite is to find performance regressions,\n> >     and it won't help with that. We don't compare the numbers between\n> >     two tests (which the perf suite has no idea are even related), and\n> >     any change in its numbers would have nothing to do with bitmaps.\n> \n> This does make me think if there is a way to adjust \"./run\" to test two\n> different config settings or command-line options instead of just two\n> different build versions. Perhaps something like\n> \n>  ./run \"HEAD -c core.commitGraph=true\" \"HEAD -c core.commitGraph=false\" -- p4200-line-log.sh\n> \n> But that's just musing on my part.\n\nI think that's the right direction, but it would require tests knowing\nhow to make use of those extra parameters. What I'd _really_ like is a\nway to parameterize a whole bunch of things:\n\n - test repo\n - git version\n - particular config settings\n\ndo runs with whatever combinations of parameters you choose, and then\ncreate tables showing differences across various settings of a\nparameter. Some parameters would be automatically set up, but test\nscripts would have to advertise other parameters they know about.\n\nI don't think it's conceptually hard, but the existing perf code is\npretty hacked-together and relies on the filesystem for storage. So the\nnotion that we'd only ever compare different versions runs deep; that's\nhow the on-disk storage is organized.\n\n> > So let's just drop it. It's not useful and is adding minutes to perf\n> > runs.\n> \n> I agree with your reasoning. This is not a critical path for clients,\n> and all servers should be using bitmaps.\n\nYeah, I should have noted that we're not actually testing the\nperformance of a stock repack anywhere else. It wouldn't belong in\np5310, but in theory it might be of interest in another script. But I\nagree for the reasons you gave that it's not all that interesting a\ntiming.\n\n-Peff\n"}]}