{"thread":{"id":"54249","subject":"approximate_object_count_valid never set?","startedAt":"2020-09-17T08:20:19Z","lastAt":"2020-09-17T18:29:39Z","messageCount":6,"participants":["Rasmus Villemoes","Ævar Arnfjörð Bjarmason","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"405733","messageId":"4a018cb9-da40-a98f-a1b9-73be30ae79ec@rasmusvillemoes.dk","threadId":"54249","inReplyTo":null,"subject":"approximate_object_count_valid never set?","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2020-09-17T08:20:03Z","receivedAt":"2020-09-17T08:20:19Z","isPatch":false,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"Hi,\n\nWhile poking around the code, I noticed that it seems\n->approximate_object_count_valid is never set to 1, and it never has\nbeen, not even back when it was a global variable. So perhaps it can\njust be removed and the logic depending on it simplified? Or am I\nmissing some preprocessor trickery.\n\nNobody seems to have noticed the lack of caching - and actually setting\nit to 1 after the count has been computed might be a little dangerous\nunless one takes care to invalidate the cache anywhere that might be\nrelevant.\n\nRasmus\n"},{"id":"405741","messageId":"87pn6khay1.fsf@evledraar.gmail.com","threadId":"54249","inReplyTo":"4a018cb9-da40-a98f-a1b9-73be30ae79ec@rasmusvillemoes.dk","subject":"Re: approximate_object_count_valid never set?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-09-17T11:58:14Z","receivedAt":"2020-09-17T11:59:05Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 17 2020, Rasmus Villemoes wrote:\n\n> Hi,\n>\n> While poking around the code, I noticed that it seems\n> ->approximate_object_count_valid is never set to 1, and it never has\n> been, not even back when it was a global variable. So perhaps it can\n> just be removed and the logic depending on it simplified? Or am I\n> missing some preprocessor trickery.\n>\n> Nobody seems to have noticed the lack of caching - and actually setting\n> it to 1 after the count has been computed might be a little dangerous\n> unless one takes care to invalidate the cache anywhere that might be\n> relevant.\n\nThere's some previous discussion about this in\nhttps://public-inbox.org/git/20180226085508.GA30343@sigill.intra.peff.net/\n"},{"id":"405742","messageId":"20200917125333.GA3024501@coredump.intra.peff.net","threadId":"54249","inReplyTo":"4a018cb9-da40-a98f-a1b9-73be30ae79ec@rasmusvillemoes.dk","subject":"Re: approximate_object_count_valid never set?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T12:53:33Z","receivedAt":"2020-09-17T12:53:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 10:20:03AM +0200, Rasmus Villemoes wrote:\n\n> Hi,\n> \n> While poking around the code, I noticed that it seems\n> ->approximate_object_count_valid is never set to 1, and it never has\n> been, not even back when it was a global variable. So perhaps it can\n> just be removed and the logic depending on it simplified? Or am I\n> missing some preprocessor trickery.\n> \n> Nobody seems to have noticed the lack of caching - and actually setting\n> it to 1 after the count has been computed might be a little dangerous\n> unless one takes care to invalidate the cache anywhere that might be\n> relevant.\n\nWe should be able to construct a test where it matters. The main cost\nthat the flag is overcoming is the iteration through the packs. So we'd\nwant a lot of packs. And the primary place the function would get called\na lot is when abbreviating commits. So doing:\n\n  for i in $(seq 1000); do\n    echo blob\n    echo 'data <<EOF'\n    echo $i\n    echo EOF\n    echo checkpoint\n  done | git -c transfer.unpacklimit=0 fast-import\n\nwill get us a lot of packs. I tried that in linux.git. And then this\nshould get us a baseline for how much it costs to traverse and print out\nobject names:\n\n  $ time git rev-list --format=%H HEAD >/dev/null\n  real\t0m6.636s\n  user\t0m6.492s\n  sys\t0m0.144s\n\nAnd now let's see how long it takes with abbreviation:\n\n  $ time git rev-list --format=%h HEAD  >/dev/null\n  real\t0m34.518s\n  user\t0m34.253s\n  sys\t0m0.264s\n\nYow. That's a lot. But part of the cost is that we have to look up each\nabbreviated hash in each pack to see if it's present there, so we'd\nexpect it to be a lot more expensive. But let's try it with the caching\nflag:\n\ndiff --git a/packfile.c b/packfile.c\nindex 9ef27508f2..e69012e7f2 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -923,6 +923,7 @@ unsigned long repo_approximate_object_count(struct repository *r)\n \t\t\tcount += p->num_objects;\n \t\t}\n \t\tr->objects->approximate_object_count = count;\n+\t\tr->objects->approximate_object_count_valid = 1;\n \t}\n \treturn r->objects->approximate_object_count;\n }\n\n  $ time git rev-list --format=%h HEAD  >/dev/null\n  real\t0m29.411s\n  user\t0m29.150s\n  sys\t0m0.260s\n\nStill not great, but caching the count did save us 15%. That seems worth\nit to me (1000 packs is more than we'd hope for, but not uncommon in a\npoorly maintained repo). The failure to set the flag is just a bug;\nlooks like mine from 8e3f52d778 (find_unique_abbrev: move logic out of\nget_short_sha1(), 2016-10-03).\n\nYou're right that caching runs the risk of the cache being invalidated.\nBut in this case I think we're covered. We'd generally modify packed_git\nonly via reprepare_packed_git(), prepare_packed_git(), and we do reset\nthe flag there. Plus count is only meant to be approximate, so even if\nit ended up stale within a single process, I don't think it would be\nthat big a deal.\n\n-Peff\n"},{"id":"405769","messageId":"20200917164743.GA3731633@coredump.intra.peff.net","threadId":"54249","inReplyTo":"20200917125333.GA3024501@coredump.intra.peff.net","subject":"[PATCH] packfile: actually set approximate_object_count_valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T16:47:43Z","receivedAt":"2020-09-17T16:48:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 08:53:33AM -0400, Jeff King wrote:\n\n> Still not great, but caching the count did save us 15%. That seems worth\n> it to me (1000 packs is more than we'd hope for, but not uncommon in a\n> poorly maintained repo). The failure to set the flag is just a bug;\n> looks like mine from 8e3f52d778 (find_unique_abbrev: move logic out of\n> get_short_sha1(), 2016-10-03).\n> \n> You're right that caching runs the risk of the cache being invalidated.\n> But in this case I think we're covered. We'd generally modify packed_git\n> only via reprepare_packed_git(), prepare_packed_git(), and we do reset\n> the flag there. Plus count is only meant to be approximate, so even if\n> it ended up stale within a single process, I don't think it would be\n> that big a deal.\n\nSo here it is wrapped up as a patch. I think it's worth fixing (as\nopposed to dropping the unused flag code). Thanks for finding it.\n\n-- >8 --\nSubject: [PATCH] packfile: actually set approximate_object_count_valid\n\nThe approximate_object_count() function tries to compute the count only\nonce per process. But ever since it was introduced in 8e3f52d778\n(find_unique_abbrev: move logic out of get_short_sha1(), 2016-10-03), we\nfailed to actually set the \"valid\" flag, meaning we'd compute it fresh\non every call.\n\nThis turns out not to be _too_ bad, because we're only iterating through\nthe packed_git list, and not making any system calls. But since it may\nget called for every abbreviated hash we output, even this can add up if\nyou have many packs.\n\nHere are before-and-after timings for a new perf test which just asks\nrev-list to abbreviate each commit hash (the test repo is linux.git,\nwith commit-graphs):\n\n  Test                            origin              HEAD\n  ----------------------------------------------------------------------------\n  5303.3: rev-list (1)            28.91(28.46+0.44)   29.03(28.65+0.38) +0.4%\n  5303.4: abbrev-commit (1)       1.18(1.06+0.11)     1.17(1.02+0.14) -0.8%\n  5303.7: rev-list (50)           28.95(28.56+0.38)   29.50(29.17+0.32) +1.9%\n  5303.8: abbrev-commit (50)      3.67(3.56+0.10)     3.57(3.42+0.15) -2.7%\n  5303.11: rev-list (1000)        30.34(29.89+0.43)   30.82(30.35+0.46) +1.6%\n  5303.12: abbrev-commit (1000)   86.82(86.52+0.29)   77.82(77.59+0.22) -10.4%\n  5303.15: load 10,000 packs      0.08(0.02+0.05)     0.08(0.02+0.06) +0.0%\n\nIt doesn't help at all when we have 1 pack (5303.4), but we get a 10%\nspeedup when there are 1000 packs (5303.12). That's a modest speedup for\na case that's already slow and we'd hope to avoid in general (note how\nslow it is even after, because we have to look in each of those packs\nfor abbreviations). But it's a one-line change that clearly matches the\noriginal intent, so it seems worth doing.\n\nThe included perf test may also be useful for keeping an eye on any\nregressions in the overall abbreviation code.\n\nReported-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c                 | 1 +\n t/perf/p5303-many-packs.sh | 4 ++++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 9ef27508f2..e69012e7f2 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -923,6 +923,7 @@ unsigned long repo_approximate_object_count(struct repository *r)\n \t\t\tcount += p->num_objects;\n \t\t}\n \t\tr->objects->approximate_object_count = count;\n+\t\tr->objects->approximate_object_count_valid = 1;\n \t}\n \treturn r->objects->approximate_object_count;\n }\ndiff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh\nindex 7ee791669a..f4c2ab0584 100755\n--- a/t/perf/p5303-many-packs.sh\n+++ b/t/perf/p5303-many-packs.sh\n@@ -73,6 +73,10 @@ do\n \t\tgit rev-list --objects --all >/dev/null\n \t'\n \n+\ttest_perf \"abbrev-commit ($nr_packs)\" '\n+\t\tgit rev-list --abbrev-commit HEAD >/dev/null\n+\t'\n+\n \t# This simulates the interesting part of the repack, which is the\n \t# actual pack generation, without smudging the on-disk setup\n \t# between trials.\n-- \n2.28.0.982.gdd163d6eb1\n\n"},{"id":"405770","messageId":"20200917165300.GA15187@nand.local","threadId":"54249","inReplyTo":"20200917164743.GA3731633@coredump.intra.peff.net","subject":"Re: [PATCH] packfile: actually set approximate_object_count_valid","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-17T16:53:00Z","receivedAt":"2020-09-17T16:53:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 17, 2020 at 12:47:43PM -0400, Jeff King wrote:\n> So here it is wrapped up as a patch. I think it's worth fixing (as\n> opposed to dropping the unused flag code). Thanks for finding it.\n\nYup, after reading the patch and performance timings below, I agree that\nthis is worth fixing and keeping instead of dropping.\n\n> It doesn't help at all when we have 1 pack (5303.4), but we get a 10%\n> speedup when there are 1000 packs (5303.12). That's a modest speedup for\n> a case that's already slow and we'd hope to avoid in general (note how\n> slow it is even after, because we have to look in each of those packs\n> for abbreviations). But it's a one-line change that clearly matches the\n> original intent, so it seems worth doing.\n\nExcellent.\n\n> The included perf test may also be useful for keeping an eye on any\n> regressions in the overall abbreviation code.\n>\n> Reported-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  packfile.c                 | 1 +\n>  t/perf/p5303-many-packs.sh | 4 ++++\n>  2 files changed, 5 insertions(+)\n>\n> diff --git a/packfile.c b/packfile.c\n> index 9ef27508f2..e69012e7f2 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -923,6 +923,7 @@ unsigned long repo_approximate_object_count(struct repository *r)\n>  \t\t\tcount += p->num_objects;\n>  \t\t}\n>  \t\tr->objects->approximate_object_count = count;\n> +\t\tr->objects->approximate_object_count_valid = 1;\n>  \t}\n>  \treturn r->objects->approximate_object_count;\n>  }\n> diff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh\n> index 7ee791669a..f4c2ab0584 100755\n> --- a/t/perf/p5303-many-packs.sh\n> +++ b/t/perf/p5303-many-packs.sh\n> @@ -73,6 +73,10 @@ do\n>  \t\tgit rev-list --objects --all >/dev/null\n>  \t'\n>\n> +\ttest_perf \"abbrev-commit ($nr_packs)\" '\n> +\t\tgit rev-list --abbrev-commit HEAD >/dev/null\n> +\t'\n> +\n>  \t# This simulates the interesting part of the repack, which is the\n>  \t# actual pack generation, without smudging the on-disk setup\n>  \t# between trials.\n> --\n> 2.28.0.982.gdd163d6eb1\n\nLooks all very good to me. Thanks.\n\nThanks,\nTaylor\n"},{"id":"405790","messageId":"xmqqy2l8qmya.fsf@gitster.c.googlers.com","threadId":"54249","inReplyTo":"20200917164743.GA3731633@coredump.intra.peff.net","subject":"Re: [PATCH] packfile: actually set approximate_object_count_valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T18:26:21Z","receivedAt":"2020-09-17T18:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] packfile: actually set approximate_object_count_valid\n>\n> The approximate_object_count() function tries to compute the count only\n> once per process. But ever since it was introduced in 8e3f52d778\n> (find_unique_abbrev: move logic out of get_short_sha1(), 2016-10-03), we\n> failed to actually set the \"valid\" flag, meaning we'd compute it fresh\n> on every call.\n> ...\n>   Test                            origin              HEAD\n>   ----------------------------------------------------------------------------\n>   5303.3: rev-list (1)            28.91(28.46+0.44)   29.03(28.65+0.38) +0.4%\n>   5303.4: abbrev-commit (1)       1.18(1.06+0.11)     1.17(1.02+0.14) -0.8%\n>   5303.7: rev-list (50)           28.95(28.56+0.38)   29.50(29.17+0.32) +1.9%\n>   5303.8: abbrev-commit (50)      3.67(3.56+0.10)     3.57(3.42+0.15) -2.7%\n>   5303.11: rev-list (1000)        30.34(29.89+0.43)   30.82(30.35+0.46) +1.6%\n>   5303.12: abbrev-commit (1000)   86.82(86.52+0.29)   77.82(77.59+0.22) -10.4%\n\nYuk, this is quite extreme.\n\n>   5303.15: load 10,000 packs      0.08(0.02+0.05)     0.08(0.02+0.06) +0.0%\n>\n> It doesn't help at all when we have 1 pack (5303.4), but we get a 10%\n> speedup when there are 1000 packs (5303.12). That's a modest speedup for\n> a case that's already slow and we'd hope to avoid in general (note how\n> slow it is even after, because we have to look in each of those packs\n> for abbreviations). But it's a one-line change that clearly matches the\n> original intent, so it seems worth doing.\n>\n> The included perf test may also be useful for keeping an eye on any\n> regressions in the overall abbreviation code.\n>\n> Reported-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  packfile.c                 | 1 +\n>  t/perf/p5303-many-packs.sh | 4 ++++\n>  2 files changed, 5 insertions(+)\n\nThanks for finding and fixing.  I agree that this is worth doing.\n"}]}