{"thread":{"id":"58462","subject":"[PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","startedAt":"2022-09-20T02:08:40Z","lastAt":"2022-09-20T20:36:12Z","messageCount":6,"participants":["Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463261","messageId":"9195a9ecd11a19f2c7fb1c70136d2d13fa308010.1663639662.git.me@ttaylorr.com","threadId":"58462","inReplyTo":null,"subject":"[PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T02:08:35Z","receivedAt":"2022-09-20T02:08:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `git multi-pack-index repack` command aggregates all objects in\ncertain packs contained in the MIDX, largely dictated by the value of\nits `--batch-size` parameter.\n\nTo create the new pack, the MIDX machinery spawns a `pack-objects`\nsub-process, which it feeds the object IDs in each of the pack(s) that\nit wants to aggregate.\n\nThis implementation, which dates back to ce1e4a105b (midx: implement\nmidx_repack(), 2019-06-10), predates the `--stdin-packs` mode in\n`pack-objects`. This mode (which was introduced a couple of years later\nin 339bce27f4 (builtin/pack-objects.c: add '--stdin-packs' option,\n2021-02-22)) allows `pack-objects` callers to specify the contents of\nthe output pack by indicating which packs to aggregate.\n\nThis patch replaces the pre-`--stdin-packs` invocation (where each\nobject is given to `pack-objects` one by one) with the more modern\n`--stdin-packs` option.\n\nThis allows us to avoid some CPU cycles serializing and deserializing\nevery object ID in all of the packs we're aggregating. It also avoids us\nhaving to send a potentially large amount of data down to\n`pack-objects`.\n\nBut more importantly, it generates slightly higher quality (read: more\ntightly compressed) packs, because of the reachability traversal that\n`--stdin-packs` does after the fact in order to gather namehash values\nwhich seed the delta selection process.\n\nIn practice, this seems to add a slight amount of overhead (on the order\nof a few seconds for git.git broken up into ~100 packs), in exchange for\na modest reduction (on the order of ~3.5%) in the resulting pack size.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nNoticed this while working on a semi-related series in:\n\n  https://lore.kernel.org/git/cover.1663638929.git.me@ttaylorr.com/T/\n\nthe savings here are pretty modest, but this is in line with the\nstrategy we use in the `--geometric` repack mode, which performs a\nsimilar task.\n\n midx.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex c27d0e5f15..c06f0f00e6 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1973,6 +1973,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tstruct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);\n\n \t/*\n@@ -1997,7 +1998,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n \trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n\n-\tstrvec_push(&cmd.args, \"pack-objects\");\n+\tstrvec_pushl(&cmd.args, \"pack-objects\", \"--stdin-packs\", NULL);\n\n \tstrbuf_addstr(&base_name, object_dir);\n \tstrbuf_addstr(&base_name, \"/pack/pack\");\n@@ -2026,17 +2027,17 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n\n \tcmd_in = xfdopen(cmd.in, \"w\");\n\n-\tfor (i = 0; i < m->num_objects; i++) {\n-\t\tstruct object_id oid;\n-\t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tstrbuf_reset(&scratch);\n\n-\t\tif (!include_pack[pack_int_id])\n-\t\t\tcontinue;\n+\t\tstrbuf_addstr(&scratch, m->pack_names[i]);\n+\t\tstrbuf_strip_suffix(&scratch, \".idx\");\n+\t\tstrbuf_addstr(&scratch, \".pack\");\n\n-\t\tnth_midxed_object_oid(&oid, m, i);\n-\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n+\t\tfprintf(cmd_in, \"%s%s\\n\", include_pack[i] ? \"\" : \"^\", scratch.buf);\n \t}\n \tfclose(cmd_in);\n+\tstrbuf_release(&scratch);\n\n \tif (finish_command(&cmd)) {\n \t\terror(_(\"could not finish pack-objects\"));\n--\n2.37.0.1.g1379af2e9d\n"},{"id":"463263","messageId":"YykiHrWvrktCLRCB@nand.local","threadId":"58462","inReplyTo":"9195a9ecd11a19f2c7fb1c70136d2d13fa308010.1663639662.git.me@ttaylorr.com","subject":"Re: [PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T02:14:54Z","receivedAt":"2022-09-20T02:15:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 19, 2022 at 10:08:35PM -0400, Taylor Blau wrote:\n> Noticed this while working on a semi-related series in:\n>\n>   https://lore.kernel.org/git/cover.1663638929.git.me@ttaylorr.com/T/\n>\n> the savings here are pretty modest, but this is in line with the\n> strategy we use in the `--geometric` repack mode, which performs a\n> similar task.\n\nTo expand on my setup a little more, I ran the following script:\n\n--- >8 ---\n#!/bin/sh\n\nrepack_into_n () {\n\trm -rf staging &&\n\tmkdir staging &&\n\n\tgit rev-list --first-parent HEAD |\n\tperl -e '\n\t\tmy $n = shift;\n\t\twhile (<>) {\n\t\t\tlast unless @commits < $n;\n\t\t\tpush @commits, $_ if $. % 5 == 1;\n\t\t}\n\t\tprint reverse @commits;\n\t' \"$1\" >pushes &&\n\n\t# create base packfile\n\tbase_pack=$(\n\t\thead -n 1 pushes |\n\t\tgit pack-objects --delta-base-offset --revs staging/pack\n\t) &&\n\texport base_pack &&\n\n\t# create an empty packfile\n\tempty_pack=$(git pack-objects staging/pack </dev/null) &&\n\texport empty_pack &&\n\n\t# and then incrementals between each pair of commits\n\tlast= &&\n\twhile read rev\n\tdo\n\t\tif test -n \"$last\"; then\n\t\t\t{\n\t\t\t\techo \"$rev\" &&\n\t\t\t\techo \"^$last\"\n\t\t\t} |\n\t\t\tgit pack-objects --delta-base-offset --revs \\\n\t\t\t\tstaging/pack || return 1\n\t\tfi\n\t\tlast=$rev\n\tdone <pushes &&\n\n\t(\n\t\tfind staging -type f -name 'pack-*.pack' |\n\t\t\txargs -n 1 basename | grep -v \"$base_pack\" &&\n\t\tprintf \"^pack-%s.pack\\n\" $base_pack\n\t) >stdin.packs\n\n\t# and install the whole thing\n\trm -f .git/objects/pack/* &&\n\tmv staging/* .git/objects/pack/\n}\n\n# Pretend we just have a single branch and no reflogs, and that everything is\n# in objects/pack; that makes our fake pack-building via repack_into_n()\n# much simpler.\nsimplify_reachability() {\n\ttip=$(git rev-parse --verify HEAD) &&\n\tgit for-each-ref --format=\"option no-deref%0adelete %(refname)\" |\n\tgit update-ref --stdin &&\n\trm -rf .git/logs &&\n\tgit update-ref refs/heads/master $tip &&\n\tgit symbolic-ref HEAD refs/heads/master\n}\n\nsimplify_reachability\n\nfor i in 100 1000 5000\ndo\n  echo >&2 \"==> $i pack(s)\"\n\n  repack_into_n $i\n\n  rm .git/objects/pack/multi-pack-index\n  find .git/objects/pack -type f | sort >before\n\n  hyperfine -p './prepare.sh' \\\n\t  'git multi-pack-index repack --batch-size=1G && ./report.sh' \\\n\t  'git.compile multi-pack-index repack --batch-size=1G && ./report.sh'\ndone\n--- 8< ---\n\n...where `git.compile` is has this patch and `git` does not. The two\nother scripts (prepare.sh, and report.sh, respectively) look as follows:\n\n--- >8 ---\n#!/bin/sh\n\nfind .git/objects/pack -type f | sort >after\ncomm -13 before after | xargs rm -f\nrm -f .git/objects/pack/multi-pack-index\ngit multi-pack-index write\n--- 8< ---\n\n...and report.sh:\n\n--- >8 ---\n#!/bin/sh\n\nfind .git/objects/pack -type f | sort >after\nfor new in $(comm -13 before after)\ndo\n\techo \"==> $new ($(wc -c <$new))\"\ndone\necho \"-------------\"\n--- 8< ---\n\nIn general, the timings on git.git packed into 100 packs look something\nlike:\n\n  Benchmark 1: git multi-pack-index repack --batch-size=1G\n    Time (mean ± σ):      4.342 s ±  0.087 s    [User: 12.864 s, System: 0.396 s]\n    Range (min … max):    4.235 s …  4.517 s    10 runs\n\n  Benchmark 2: git.compile multi-pack-index repack --batch-size=1G\n    Time (mean ± σ):      7.016 s ±  0.119 s    [User: 11.170 s, System: 0.469 s]\n    Range (min … max):    6.858 s …  7.233 s    10 runs\n\nBut if I rip out the traversal pass towards the end of\n`read_packs_list_from_stdin()` in `builtin/pack-objects.c`, the two\ntimings are equal. So the slow-down here really is from the traversal\npass.\n\nThe savings are modest, probably because we're already starting with a\npretty well packed baseline, since we're feeding objects in pack order.\nOn average, I was able to see around a ~3.5% reduction in pack size or\nso.\n\nSo, not amazing, but mirroring the behavior of `git repack\n--geometric=<n>` is worthwhile for all of the reasons that we do this\nthere.\n\nI should also mention that this applies cleanly against `master`, and\ndoesn't depend on or interact with my changes in the series above.\n\nThanks,\nTaylor\n"},{"id":"463324","messageId":"YyoUZb90HeJnOuAV@coredump.intra.peff.net","threadId":"58462","inReplyTo":"9195a9ecd11a19f2c7fb1c70136d2d13fa308010.1663639662.git.me@ttaylorr.com","subject":"Re: [PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-20T19:28:37Z","receivedAt":"2022-09-20T19:28:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2022 at 10:08:35PM -0400, Taylor Blau wrote:\n\n> This patch replaces the pre-`--stdin-packs` invocation (where each\n> object is given to `pack-objects` one by one) with the more modern\n> `--stdin-packs` option.\n> \n> This allows us to avoid some CPU cycles serializing and deserializing\n> every object ID in all of the packs we're aggregating. It also avoids us\n> having to send a potentially large amount of data down to\n> `pack-objects`.\n\nMakes sense. Just playing devil's advocate for a moment: is there any\nway that getting the list of packs could be worse? I'm thinking\nparticularly of a race condition where a pack goes away while we're\nrunning, but if we had the actual object list, we could fall back to\nfinding it elsewhere.\n\nI think that could only happen if we had two gc's running\nsimultaneously, which is something we try to avoid already. And the\nworst case would be that one would say \"oops, this pack went away\" and\nbail, and not any kind of corruption.\n\nSo I think it's fine, but just trying to talk through any unexpected\nimplications.\n\n> But more importantly, it generates slightly higher quality (read: more\n> tightly compressed) packs, because of the reachability traversal that\n> `--stdin-packs` does after the fact in order to gather namehash values\n> which seed the delta selection process.\n\nI think we _could_ do that same traversal even in objects mode. Or do\n--stdin-packs without it. If we were starting from scratch, it might be\nnice for the two features to be orthogonal so we could evaluate the\nchanges independently. But I don't think it's worth going back and\ntrying to split them out now. Although...\n\n> In practice, this seems to add a slight amount of overhead (on the order\n> of a few seconds for git.git broken up into ~100 packs), in exchange for\n> a modest reduction (on the order of ~3.5%) in the resulting pack size.\n\nHmm. I thought we'd have some code to reuse the cached name-hashes in\nthe .bitmap file, if one is present. But I don't see any such code in\nthe stdin-packs feature. I think for \"repack --geometric\" it doesn't\nmatter. There the \"main\" pack with the bitmap would also be excluded\nfrom the rollup (unless we are rolling all-into-one, in which case we do\nthe full from-scratch repack with a traversal).\n\nIs that true also of \"multi-pack-index repack\"? I guess it would depend\non how you invoke it. I admit I don't think I've ever used it myself,\nsince the new \"repack --geometric --write-midx\" approach matches my\nmental model. I'm not sure when you'd actually run the \"multi-pack-index\nrepack\" command. But if you did it with --batch-size=0 (the default), I\nthink we'd end up traversing every object in history.\n\n>  midx.c | 17 +++++++++--------\n>  1 file changed, 9 insertions(+), 8 deletions(-)\n\nThe patch itself is mostly as expected, but I did have one question:\n\n> @@ -2026,17 +2027,17 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n> \n>  \tcmd_in = xfdopen(cmd.in, \"w\");\n> \n> -\tfor (i = 0; i < m->num_objects; i++) {\n> -\t\tstruct object_id oid;\n> -\t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n> +\tfor (i = 0; i < m->num_packs; i++) {\n> +\t\tstrbuf_reset(&scratch);\n\nThe old code went in object order within the midx. Is this sorted by\nsha1, or the pack pseudo-order? If the former, then that will yield a\ndifferent order of objects inside pack-objects (since it is seeing the\npacks in order of our m->pack_names array). I don't _think_ it matters,\nbut I just wanted to double check.\n\n-Peff\n"},{"id":"463326","messageId":"YyoZM1V5S53dz6U6@nand.local","threadId":"58462","inReplyTo":"YyoUZb90HeJnOuAV@coredump.intra.peff.net","subject":"Re: [PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T19:49:07Z","receivedAt":"2022-09-20T19:49:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 20, 2022 at 03:28:37PM -0400, Jeff King wrote:\n> On Mon, Sep 19, 2022 at 10:08:35PM -0400, Taylor Blau wrote:\n>\n> > This patch replaces the pre-`--stdin-packs` invocation (where each\n> > object is given to `pack-objects` one by one) with the more modern\n> > `--stdin-packs` option.\n> >\n> > This allows us to avoid some CPU cycles serializing and deserializing\n> > every object ID in all of the packs we're aggregating. It also avoids us\n> > having to send a potentially large amount of data down to\n> > `pack-objects`.\n>\n> Makes sense. Just playing devil's advocate for a moment: is there any\n> way that getting the list of packs could be worse? I'm thinking\n> particularly of a race condition where a pack goes away while we're\n> running, but if we had the actual object list, we could fall back to\n> finding it elsewhere.\n>\n> I think that could only happen if we had two gc's running\n> simultaneously, which is something we try to avoid already. And the\n> worst case would be that one would say \"oops, this pack went away\" and\n> bail, and not any kind of corruption.\n>\n> So I think it's fine, but just trying to talk through any unexpected\n> implications.\n\nYour assumption is right. We perform those pack validity checks pretty\nearly these days, see: 5045759de8 (builtin/pack-objects.c: ensure\nincluded `--stdin-packs` exist, 2022-05-24).\n\nWe *could* handle the case where we know the object names, but the pack\nfile has gone away (either we could open the .idx but not the .pack, or\nwe opened both but then had to close the .pack because of hitting the\nmax open file-descriptor limit).\n\n...But it gets tricky in practice, and 5045759de8 has some of those\ndetails. At worst we'd complain that one of the packs listed is gone,\nand then fail to repack (while maintaining the non-corruptedness of the\nrepository).\n\n> > But more importantly, it generates slightly higher quality (read: more\n> > tightly compressed) packs, because of the reachability traversal that\n> > `--stdin-packs` does after the fact in order to gather namehash values\n> > which seed the delta selection process.\n>\n> I think we _could_ do that same traversal even in objects mode. Or do\n> --stdin-packs without it. If we were starting from scratch, it might be\n> nice for the two features to be orthogonal so we could evaluate the\n> changes independently. But I don't think it's worth going back and\n> trying to split them out now. Although...\n\nIt's relatively easy to do `--stdin-packs` without the traversal. I\nwouldn't be opposed to doing that here.\n\n> > In practice, this seems to add a slight amount of overhead (on the order\n> > of a few seconds for git.git broken up into ~100 packs), in exchange for\n> > a modest reduction (on the order of ~3.5%) in the resulting pack size.\n>\n> Hmm. I thought we'd have some code to reuse the cached name-hashes in\n> the .bitmap file, if one is present. But I don't see any such code in\n> the stdin-packs feature. I think for \"repack --geometric\" it doesn't\n> matter. There the \"main\" pack with the bitmap would also be excluded\n> from the rollup (unless we are rolling all-into-one, in which case we do\n> the full from-scratch repack with a traversal).\n\nRight.\n\n> Is that true also of \"multi-pack-index repack\"? I guess it would depend\n> on how you invoke it. I admit I don't think I've ever used it myself,\n> since the new \"repack --geometric --write-midx\" approach matches my\n> mental model. I'm not sure when you'd actually run the \"multi-pack-index\n> repack\" command. But if you did it with --batch-size=0 (the default), I\n> think we'd end up traversing every object in history.\n\nWe could probably benefit from it, but only if there is a MIDX bitmap\naround to begin with. For instance, you could first try and lookup each\nobject you're missing a namehash for and then read its value out of the\nhashcache extension in the MIDX bitmap (assuming the MIDX bitmap exists,\nand has a hashcache).\n\nBut if you don't have a MIDX bitmap, or it has a poor selection of\ncommits, then you're out of luck.\n\n> > @@ -2026,17 +2027,17 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n> >\n> >  \tcmd_in = xfdopen(cmd.in, \"w\");\n> >\n> > -\tfor (i = 0; i < m->num_objects; i++) {\n> > -\t\tstruct object_id oid;\n> > -\t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n> > +\tfor (i = 0; i < m->num_packs; i++) {\n> > +\t\tstrbuf_reset(&scratch);\n>\n> The old code went in object order within the midx. Is this sorted by\n> sha1, or the pack pseudo-order? If the former, then that will yield a\n> different order of objects inside pack-objects (since it is seeing the\n> packs in order of our m->pack_names array). I don't _think_ it matters,\n> but I just wanted to double check.\n\nGood point. This ends up ordering the packs based on their SHA-1\nchecksum, and probably should stick to the pack mtimes instead.\n\nUnfortunately, we discard that information by the time we get to this\npoint in midx_repack(). We don't even have it written durably in the\nMIDX, either, so we reconstruct it on-the-fly in\nfill_included_packs_batch() (see the `QSORT()` call there with\n`compare_by_mtime()`).\n\nI agree that it probably doesn't matter in practice, but it's worth\ntrying to match the existing behavior, at least.\n\nThanks,\nTaylor\n"},{"id":"463329","messageId":"YyodQg5diyr/UYK1@coredump.intra.peff.net","threadId":"58462","inReplyTo":"YyoZM1V5S53dz6U6@nand.local","subject":"Re: [PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-20T20:06:26Z","receivedAt":"2022-09-20T20:06:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 20, 2022 at 03:49:07PM -0400, Taylor Blau wrote:\n\n> > Is that true also of \"multi-pack-index repack\"? I guess it would depend\n> > on how you invoke it. I admit I don't think I've ever used it myself,\n> > since the new \"repack --geometric --write-midx\" approach matches my\n> > mental model. I'm not sure when you'd actually run the \"multi-pack-index\n> > repack\" command. But if you did it with --batch-size=0 (the default), I\n> > think we'd end up traversing every object in history.\n> \n> We could probably benefit from it, but only if there is a MIDX bitmap\n> around to begin with. For instance, you could first try and lookup each\n> object you're missing a namehash for and then read its value out of the\n> hashcache extension in the MIDX bitmap (assuming the MIDX bitmap exists,\n> and has a hashcache).\n> \n> But if you don't have a MIDX bitmap, or it has a poor selection of\n> commits, then you're out of luck.\n\nYou could also use a pack bitmap if there is one (and it's one of the\nincluded packs). But yes, if you have neither, it's no help.\n\nMostly I'm just concerned that this could have an outsized negative\nperformance effect if you have a setup like:\n\n  - you have a gigantic repository, say that takes 15 minutes to do a\n    full \"rev-list --objects\" on (like linux.git with all its forks)\n\n  - most of that is in one big pack, but you acquire new packs\n    occasionally via pushes, etc\n\n  - doing \"git repack --geometric\" rolls up the new packs, nicely\n    traversing just the new objects\n\n  - doing \"git multi-pack-index repack\" before your patch is fast-ish.\n    It stuffs all the objects into a new pack. But after your patch, it\n    does that 15-minute traversal.\n\nBut I don't know if that's even realistic, because I'm still wondering\nwhy somebody would run \"git multi-pack-index repack\" in the first place.\nAnd if they'd always do so with --batch-size anyway, which would\nmitigate this (because it gives a geometric-ish approach where we leave\nthe huge pack untouched).\n\nIf it is, then one thing to consider is tying the \"do the extra\ntraversal\" feature to the presence / size of excluded packs. And\npossibly considering the presence of a bitmap to indicate that it's\nworth doing (assuming the optimization there is implemented).\n\nBut that sounds like a lot of work to get right, and again, I'm not\nreally sure of the benefit.\n\n> > The old code went in object order within the midx. Is this sorted by\n> > sha1, or the pack pseudo-order? If the former, then that will yield a\n> > different order of objects inside pack-objects (since it is seeing the\n> > packs in order of our m->pack_names array). I don't _think_ it matters,\n> > but I just wanted to double check.\n> \n> Good point. This ends up ordering the packs based on their SHA-1\n> checksum, and probably should stick to the pack mtimes instead.\n> \n> Unfortunately, we discard that information by the time we get to this\n> point in midx_repack(). We don't even have it written durably in the\n> MIDX, either, so we reconstruct it on-the-fly in\n> fill_included_packs_batch() (see the `QSORT()` call there with\n> `compare_by_mtime()`).\n> \n> I agree that it probably doesn't matter in practice, but it's worth\n> trying to match the existing behavior, at least.\n\nYeah, sorting the packs by mtime might be sensible. I know in the final\nmidx, we use object order to find the \"preferred\" pack. And you could\niterate the objects here, passing along their de-duped pack name. But I\ndon't think we have the objects here in that useful order; that is\nreally the order of the midx's .rev file, IIRC, and this is probably the\nactual sha1 order.\n\n-Peff\n"},{"id":"463339","messageId":"YyokIf/Sd7SYztKQ@nand.local","threadId":"58462","inReplyTo":"YyodQg5diyr/UYK1@coredump.intra.peff.net","subject":"Re: [PATCH] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T20:35:45Z","receivedAt":"2022-09-20T20:36:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 20, 2022 at 04:06:26PM -0400, Jeff King wrote:\n> On Tue, Sep 20, 2022 at 03:49:07PM -0400, Taylor Blau wrote:\n>\n> > > Is that true also of \"multi-pack-index repack\"? I guess it would depend\n> > > on how you invoke it. I admit I don't think I've ever used it myself,\n> > > since the new \"repack --geometric --write-midx\" approach matches my\n> > > mental model. I'm not sure when you'd actually run the \"multi-pack-index\n> > > repack\" command. But if you did it with --batch-size=0 (the default), I\n> > > think we'd end up traversing every object in history.\n> >\n> > We could probably benefit from it, but only if there is a MIDX bitmap\n> > around to begin with. For instance, you could first try and lookup each\n> > object you're missing a namehash for and then read its value out of the\n> > hashcache extension in the MIDX bitmap (assuming the MIDX bitmap exists,\n> > and has a hashcache).\n> >\n> > But if you don't have a MIDX bitmap, or it has a poor selection of\n> > commits, then you're out of luck.\n>\n> You could also use a pack bitmap if there is one (and it's one of the\n> included packs). But yes, if you have neither, it's no help.\n\nGood point. But, yeah, you have to have them to begin with.\n\n> Mostly I'm just concerned that this could have an outsized negative\n> performance effect if you have a setup like:\n>\n>   - you have a gigantic repository, say that takes 15 minutes to do a\n>     full \"rev-list --objects\" on (like linux.git with all its forks)\n>\n>   - most of that is in one big pack, but you acquire new packs\n>     occasionally via pushes, etc\n>\n>   - doing \"git repack --geometric\" rolls up the new packs, nicely\n>     traversing just the new objects\n>\n>   - doing \"git multi-pack-index repack\" before your patch is fast-ish.\n>     It stuffs all the objects into a new pack. But after your patch, it\n>     does that 15-minute traversal.\n>\n> But I don't know if that's even realistic, because I'm still wondering\n> why somebody would run \"git multi-pack-index repack\" in the first place.\n> And if they'd always do so with --batch-size anyway, which would\n> mitigate this (because it gives a geometric-ish approach where we leave\n> the huge pack untouched).\n\nYeah, the `--geometric` path(s) don't have this problem, because the big\npack will already be covered by either a pack or MIDX bitmap, and we can\nread out all of the namehash values from there.\n\nBut I tend to agree that this is pretty unrealistic, so I'm hopeful that\nit isn't a huge deal. If it is, though, we can always just \"turn off\"\nthe traversal parts. (Though I have to imagine that a repository large\nenough to care about the existence of namehash values probably isn't\nusing `git multi-pack-index repack` anyway).\n\n> Yeah, sorting the packs by mtime might be sensible. I know in the final\n> midx, we use object order to find the \"preferred\" pack. And you could\n> iterate the objects here, passing along their de-duped pack name. But I\n> don't think we have the objects here in that useful order; that is\n> really the order of the midx's .rev file, IIRC, and this is probably the\n> actual sha1 order.\n\nWe already need the sorted order in order to compute the rollup for\nnon-zero `--batch-size` arguments, so using that to construct the pack\nis just a matter of dragging the sort out of the function to compute the\nrollup itself (and into `midx_repack()` instead).\n\nPatches incoming... ;-)\n\nThanks,\nTaylor\n"}]}