{"thread":{"id":"52235","subject":"[PATCH] pack-objects: avoid pointless oe_map_new_pack() calls","startedAt":"2019-11-11T11:12:52Z","lastAt":"2019-11-11T14:28:02Z","messageCount":3,"participants":["Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"385902","messageId":"20191111111249.GA31169@sigill.intra.peff.net","threadId":"52235","inReplyTo":null,"subject":"[PATCH] pack-objects: avoid pointless oe_map_new_pack() calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T11:12:49Z","receivedAt":"2019-11-11T11:12:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch fixes an extreme slowdown in pack-objects when you have more\nthan 1023 packs. See below for numbers.\n\nSince 43fa44fa3b (pack-objects: move in_pack out of struct object_entry,\n2018-04-14), we use a complicated system to save some per-object memory.\n\nEach object_entry structs gets a 10-bit field to store the index of the\npack it's in. We map those indices into pointers using\npacking_data->in_pack_by_idx, which we initialize at the start of the\nprogram. If we have 2^10 or more packs, then we instead create an array\nof pack pointers, one per object. This is packing_data->in_pack.\n\nSo far so good. But there's one other tricky case: if a new pack arrives\nafter we've initialized in_pack_by_idx, it won't have an index yet. We\nsolve that by calling oe_map_new_pack(), which just switches on the fly\nto the less-optimal in_pack mechanism, allocating the array and\nback-filling it for already-seen objects.\n\nBut that logic kicks in even when we've switched to it already (whether\nbecause we really did see a new pack, or because we had too many packs\nin the first place). The result doesn't produce a wrong outcome, but\nit's very slow. What happens is this:\n\n  - imagine you have a repo with 500k objects and 2000 packs that you\n    want to repack.\n\n  - before looking at any objects, we call prepare_in_pack_by_idx(). It\n    starts allocating an index for each pack. On the 1024th pack, it\n    sees there are too many, so it bails, leaving in_pack_by_idx as\n    NULL.\n\n  - while actually adding objects to the packing list, we call\n    oe_set_in_pack(), which checks whether the pack already has an\n    index. If it's one of the packs after the first 1023, then it\n    doesn't have one, and we'll call oe_map_new_pack().\n\n    But there's no useful work for that function to do. We're already\n    using in_pack, so it just uselessly walks over the complete list of\n    objects, trying to backfill in_pack.\n\n    And we end up doing this for almost 1000 packs (each of which may be\n    triggered by more than one object). And each time it triggers, we\n    may iterate over up to 500k objects. So in the absolute worst case,\n    this is quadratic in the number of objects.\n\nThe solution is simple: we don't need to bother checking whether the\npack has an index if we've already converted to using in_pack, since by\ndefinition we're not going to use it. So we can just push the \"does the\npack have a valid index\" check down into that half of the conditional,\nwhere we know we're going to use it.\n\nThe current test in p5303 sadly doesn't notice this problem, since it\nmaxes out at 1000 packs. If we add a new test to it at 2000 packs, it\ndoes show the improvement:\n\n  Test                      HEAD^               HEAD\n  ----------------------------------------------------------------------\n  5303.12: repack (2000)    26.72(39.68+0.67)   15.70(28.70+0.66) -41.2%\n\nHowever, these many-pack test cases are rather expensive to run, so\nadding larger and larger numbers isn't appealing. Instead, we can show\nit off more easily by using GIT_TEST_FULL_IN_PACK_ARRAY, which forces us\ninto the absolute worst case: no pack has an index, so we'll trigger\noe_map_new_pack() pointlessly for every single object, making it truly\nquadratic.\n\nHere are the numbers (on git.git) with the included change to p5303:\n\n  Test                      HEAD^               HEAD\n  ----------------------------------------------------------------------\n  5303.3: rev-list (1)      2.05(1.98+0.06)     2.06(1.99+0.06) +0.5%\n  5303.4: repack (1)        33.45(33.46+0.19)   2.75(2.73+0.22) -91.8%\n  5303.6: rev-list (50)     2.07(2.01+0.06)     2.06(2.01+0.05) -0.5%\n  5303.7: repack (50)       34.21(35.18+0.16)   3.49(4.50+0.12) -89.8%\n  5303.9: rev-list (1000)   2.87(2.78+0.08)     2.88(2.80+0.07) +0.3%\n  5303.10: repack (1000)    41.26(51.30+0.47)   10.75(20.75+0.44) -73.9%\n\nAgain, those improvements aren't realistic for the 1-pack case (because\nin the real world, the full-array solution doesn't kick in), but it's\nmore useful to be testing the more-complicated code path.\n\nWhile we're looking at this issue, we'll tweak one more thing: in\noe_map_new_pack(), we call REALLOC_ARRAY(pack->in_pack). But we'd never\nexpect to get here unless we're back-filling it for the first time, in\nwhich case it would be NULL. So let's switch that to ALLOC_ARRAY() for\nclarity, and add a BUG() to document the expectation. Unfortunately this\ncode isn't well-covered in the test suite because it's inherently racy\n(it only kicks in if somebody else adds a new pack while we're in the\nmiddle of repacking).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-objects.c             |  5 ++++-\n pack-objects.h             | 19 ++++++++++++++-----\n t/perf/p5303-many-packs.sh |  1 +\n 3 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-objects.c b/pack-objects.c\nindex c6250d77f4..5e5a3c62d9 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -119,7 +119,10 @@ void oe_map_new_pack(struct packing_data *pack)\n {\n \tuint32_t i;\n \n-\tREALLOC_ARRAY(pack->in_pack, pack->nr_alloc);\n+\tif (pack->in_pack)\n+\t\tBUG(\"packing_data has already been converted to pack array\");\n+\n+\tALLOC_ARRAY(pack->in_pack, pack->nr_alloc);\n \n \tfor (i = 0; i < pack->nr_objects; i++)\n \t\tpack->in_pack[i] = oe_in_pack(pack, pack->objects + i);\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 6fe6ae5ee8..d3975e079b 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -251,12 +251,21 @@ static inline void oe_set_in_pack(struct packing_data *pack,\n \t\t\t\t  struct object_entry *e,\n \t\t\t\t  struct packed_git *p)\n {\n-\tif (!p->index)\n+\tif (pack->in_pack_by_idx) {\n+\t\tif (p->index) {\n+\t\t\te->in_pack_idx = p->index;\n+\t\t\treturn;\n+\t\t}\n+\t\t/*\n+\t\t * We're accessing packs by index, but this pack doesn't have\n+\t\t * an index (e.g., because it was added since we created the\n+\t\t * in_pack_by_idx array). Bail to oe_map_new_pack(), which\n+\t\t * will convert us to using the full in_pack array, and then\n+\t\t * fall through to our in_pack handling.\n+\t\t */\n \t\toe_map_new_pack(pack);\n-\tif (pack->in_pack_by_idx)\n-\t\te->in_pack_idx = p->index;\n-\telse\n-\t\tpack->in_pack[e - pack->objects] = p;\n+\t}\n+\tpack->in_pack[e - pack->objects] = p;\n }\n \n static inline struct object_entry *oe_delta(\ndiff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh\nindex 3779851941..a369152c47 100755\n--- a/t/perf/p5303-many-packs.sh\n+++ b/t/perf/p5303-many-packs.sh\n@@ -77,6 +77,7 @@ do\n \t# actual pack generation, without smudging the on-disk setup\n \t# between trials.\n \ttest_perf \"repack ($nr_packs)\" '\n+\t\tGIT_TEST_FULL_IN_PACK_ARRAY=1 \\\n \t\tgit pack-objects --keep-true-parents \\\n \t\t  --honor-pack-keep --non-empty --all \\\n \t\t  --reflog --indexed-objects --delta-base-offset \\\n-- \n2.24.0.739.gb5632e4929\n"},{"id":"385904","messageId":"01605e2d-5ea7-037d-b25f-3774b88d059d@gmail.com","threadId":"52235","inReplyTo":"20191111111249.GA31169@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: avoid pointless oe_map_new_pack() calls","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-11-11T13:42:47Z","receivedAt":"2019-11-11T13:42:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/11/2019 6:12 AM, Jeff King wrote:\n> This patch fixes an extreme slowdown in pack-objects when you have more\n> than 1023 packs. See below for numbers.\n> \n> Since 43fa44fa3b (pack-objects: move in_pack out of struct object_entry,\n> 2018-04-14), we use a complicated system to save some per-object memory.\n> \n> Each object_entry structs gets a 10-bit field to store the index of the\n> pack it's in. We map those indices into pointers using\n> packing_data->in_pack_by_idx, which we initialize at the start of the\n> program. If we have 2^10 or more packs, then we instead create an array\n> of pack pointers, one per object. This is packing_data->in_pack.\n> \n> So far so good. But there's one other tricky case: if a new pack arrives\n> after we've initialized in_pack_by_idx, it won't have an index yet. We\n> solve that by calling oe_map_new_pack(), which just switches on the fly\n> to the less-optimal in_pack mechanism, allocating the array and\n> back-filling it for already-seen objects.\n> \n> But that logic kicks in even when we've switched to it already (whether\n> because we really did see a new pack, or because we had too many packs\n> in the first place). The result doesn't produce a wrong outcome, but\n> it's very slow. What happens is this:\n> \n>   - imagine you have a repo with 500k objects and 2000 packs that you\n>     want to repack.\n> \n>   - before looking at any objects, we call prepare_in_pack_by_idx(). It\n>     starts allocating an index for each pack. On the 1024th pack, it\n>     sees there are too many, so it bails, leaving in_pack_by_idx as\n>     NULL.\n> \n>   - while actually adding objects to the packing list, we call\n>     oe_set_in_pack(), which checks whether the pack already has an\n>     index. If it's one of the packs after the first 1023, then it\n>     doesn't have one, and we'll call oe_map_new_pack().\n> \n>     But there's no useful work for that function to do. We're already\n>     using in_pack, so it just uselessly walks over the complete list of\n>     objects, trying to backfill in_pack.\n> \n>     And we end up doing this for almost 1000 packs (each of which may be\n>     triggered by more than one object). And each time it triggers, we\n>     may iterate over up to 500k objects. So in the absolute worst case,\n>     this is quadratic in the number of objects.\n\nQuadratic is bad!\n\n> The solution is simple: we don't need to bother checking whether the\n> pack has an index if we've already converted to using in_pack, since by\n> definition we're not going to use it. So we can just push the \"does the\n> pack have a valid index\" check down into that half of the conditional,\n> where we know we're going to use it.\n> \n> The current test in p5303 sadly doesn't notice this problem, since it\n> maxes out at 1000 packs. If we add a new test to it at 2000 packs, it\n> does show the improvement:\n> \n>   Test                      HEAD^               HEAD\n>   ----------------------------------------------------------------------\n>   5303.12: repack (2000)    26.72(39.68+0.67)   15.70(28.70+0.66) -41.2%\n\nAnd these numbers don't really show a quadratic improvement, probably due\nto the test itself needing to do the work to create 2000 packs.\n\n> However, these many-pack test cases are rather expensive to run, so\n> adding larger and larger numbers isn't appealing. Instead, we can show\n> it off more easily by using GIT_TEST_FULL_IN_PACK_ARRAY, which forces us\n> into the absolute worst case: no pack has an index, so we'll trigger\n> oe_map_new_pack() pointlessly for every single object, making it truly\n> quadratic.\n> \n> Here are the numbers (on git.git) with the included change to p5303:\n> \n>   Test                      HEAD^               HEAD\n>   ----------------------------------------------------------------------\n>   5303.3: rev-list (1)      2.05(1.98+0.06)     2.06(1.99+0.06) +0.5%\n>   5303.4: repack (1)        33.45(33.46+0.19)   2.75(2.73+0.22) -91.8%\n>   5303.6: rev-list (50)     2.07(2.01+0.06)     2.06(2.01+0.05) -0.5%\n>   5303.7: repack (50)       34.21(35.18+0.16)   3.49(4.50+0.12) -89.8%\n>   5303.9: rev-list (1000)   2.87(2.78+0.08)     2.88(2.80+0.07) +0.3%\n>   5303.10: repack (1000)    41.26(51.30+0.47)   10.75(20.75+0.44) -73.9%\n\nBut these tests do!\n\n> Again, those improvements aren't realistic for the 1-pack case (because\n> in the real world, the full-array solution doesn't kick in), but it's\n> more useful to be testing the more-complicated code path.\n\nI think this is a reasonable adjustment to demonstrate the specific problem\nat hand right now.\n\n> While we're looking at this issue, we'll tweak one more thing: in\n> oe_map_new_pack(), we call REALLOC_ARRAY(pack->in_pack). But we'd never\n> expect to get here unless we're back-filling it for the first time, in\n> which case it would be NULL. So let's switch that to ALLOC_ARRAY() for\n> clarity, and add a BUG() to document the expectation. Unfortunately this\n> code isn't well-covered in the test suite because it's inherently racy\n> (it only kicks in if somebody else adds a new pack while we're in the\n> middle of repacking).\n\nMakes sense. We can't test everything.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  pack-objects.c             |  5 ++++-\n>  pack-objects.h             | 19 ++++++++++++++-----\n>  t/perf/p5303-many-packs.sh |  1 +\n>  3 files changed, 19 insertions(+), 6 deletions(-)\n> \n> diff --git a/pack-objects.c b/pack-objects.c\n> index c6250d77f4..5e5a3c62d9 100644\n> --- a/pack-objects.c\n> +++ b/pack-objects.c\n> @@ -119,7 +119,10 @@ void oe_map_new_pack(struct packing_data *pack)\n>  {\n>  \tuint32_t i;\n>  \n> -\tREALLOC_ARRAY(pack->in_pack, pack->nr_alloc);\n> +\tif (pack->in_pack)\n> +\t\tBUG(\"packing_data has already been converted to pack array\");\n> +\n> +\tALLOC_ARRAY(pack->in_pack, pack->nr_alloc);\n>  \n>  \tfor (i = 0; i < pack->nr_objects; i++)\n>  \t\tpack->in_pack[i] = oe_in_pack(pack, pack->objects + i);\n> diff --git a/pack-objects.h b/pack-objects.h\n> index 6fe6ae5ee8..d3975e079b 100644\n> --- a/pack-objects.h\n> +++ b/pack-objects.h\n> @@ -251,12 +251,21 @@ static inline void oe_set_in_pack(struct packing_data *pack,\n>  \t\t\t\t  struct object_entry *e,\n>  \t\t\t\t  struct packed_git *p)\n>  {\n> -\tif (!p->index)\n> +\tif (pack->in_pack_by_idx) {\n> +\t\tif (p->index) {\n> +\t\t\te->in_pack_idx = p->index;\n> +\t\t\treturn;\n> +\t\t}\n> +\t\t/*\n> +\t\t * We're accessing packs by index, but this pack doesn't have\n> +\t\t * an index (e.g., because it was added since we created the\n> +\t\t * in_pack_by_idx array). Bail to oe_map_new_pack(), which\n> +\t\t * will convert us to using the full in_pack array, and then\n> +\t\t * fall through to our in_pack handling.\n> +\t\t */\n>  \t\toe_map_new_pack(pack);\n> -\tif (pack->in_pack_by_idx)\n> -\t\te->in_pack_idx = p->index;\n> -\telse\n> -\t\tpack->in_pack[e - pack->objects] = p;\n> +\t}\n> +\tpack->in_pack[e - pack->objects] = p;\n>  }\n\nThis change looks good, and matches the explanation. There is no way one\ncould look only at this method body and realize it would cause the performance\nproblem this change is solving.\n\n>  \n>  static inline struct object_entry *oe_delta(\n> diff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh\n> index 3779851941..a369152c47 100755\n> --- a/t/perf/p5303-many-packs.sh\n> +++ b/t/perf/p5303-many-packs.sh\n> @@ -77,6 +77,7 @@ do\n>  \t# actual pack generation, without smudging the on-disk setup\n>  \t# between trials.\n>  \ttest_perf \"repack ($nr_packs)\" '\n> +\t\tGIT_TEST_FULL_IN_PACK_ARRAY=1 \\\n>  \t\tgit pack-objects --keep-true-parents \\\n>  \t\t  --honor-pack-keep --non-empty --all \\\n>  \t\t  --reflog --indexed-objects --delta-base-offset \\\n> \n\nReviewed-by: Derrick Stolee <dstolee@microsoft.com>\n"},{"id":"385909","messageId":"20191111142800.GA1934@sigill.intra.peff.net","threadId":"52235","inReplyTo":"01605e2d-5ea7-037d-b25f-3774b88d059d@gmail.com","subject":"Re: [PATCH] pack-objects: avoid pointless oe_map_new_pack() calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-11T14:28:00Z","receivedAt":"2019-11-11T14:28:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2019 at 08:42:47AM -0500, Derrick Stolee wrote:\n\n> > The current test in p5303 sadly doesn't notice this problem, since it\n> > maxes out at 1000 packs. If we add a new test to it at 2000 packs, it\n> > does show the improvement:\n> > \n> >   Test                      HEAD^               HEAD\n> >   ----------------------------------------------------------------------\n> >   5303.12: repack (2000)    26.72(39.68+0.67)   15.70(28.70+0.66) -41.2%\n> \n> And these numbers don't really show a quadratic improvement, probably due\n> to the test itself needing to do the work to create 2000 packs.\n\nNo, the 2000-pack setup happens in a separate (untimed) test.\n\nThe reason this improvement is less extreme is that we're only hitting\nthe extra oe_map_new_pack() call for a subset of the objects. The first\n1023 packs have an index field (and because of the way we generate the\npacks, I think that includes the \"big\" initial pack), so not every\nobject triggers the problem.\n\nThis was spurred by a couple of real cases where maintenance had fallen\nbehind and we had accrued a lot of packs (and of course we wanted to\nrepack to get out of the bad situation, which became a chicken and egg).\nOne of those real-world cases with 15,000 packs dropped from ~11 hours\nto ~5 minutes.\n\nI'm actually amazed we didn't notice this before (the problem started in\nv2.18, which we deployed at GitHub over a year ago). But we try to avoid\nending up with that many packs in the first place, and even when we did,\nautomated maintenance _usually_ dug them out by itself if you were\npatient enough.\n\n> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>\n\nThanks!\n\n-Peff\n"}]}