{"thread":{"id":"55430","subject":"rather slow 'git repack' in 'blob:none' partial clones","startedAt":"2021-04-03T09:04:35Z","lastAt":"2021-04-21T19:35:00Z","messageCount":46,"participants":["SZEDER Gábor","Rafael Silva","Jeff King","Jonathan Tan","Bryan Turner","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"420926","messageId":"20210403090412.GH2271@szeder.dev","threadId":"55430","inReplyTo":null,"subject":"rather slow 'git repack' in 'blob:none' partial clones","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-03T09:04:25Z","receivedAt":"2021-04-03T09:04:35Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nhere are trace timings of running 'git gc' in a \"normal\" and in a\n'blob:none' partial clone:\n\n $ git clone --bare https://github.com/git/git git-full.git\n $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-full.git/ gc\n 10:35:24.007277 trace.c:487 performance: 0.001550225 s: git command: /usr/local/libexec/git-core/git pack-refs --all --prune\n 10:35:24.044641 trace.c:487 performance: 0.035631270 s: git command: /usr/local/libexec/git-core/git reflog expire --all\n 10:35:24.061070 read-cache.c:2315 performance: 0.000008506 s: read cache ./index\n Enumerating objects: 305283, done.\n Counting objects: 100% (305283/305283), done.\n Delta compression using up to 4 threads\n Compressing objects: 100% (75016/75016), done.\n Writing objects: 100% (305283/305283), done.\n Total 305283 (delta 227928), reused 305283 (delta 227928), pack-reused 0\n 10:35:32.604546 trace.c:487 performance: 8.555651283 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946975-pack --keep-true-parents --honor-pack-keep --non-empty --all --reflog --indexed-objects --unpack-unreachable=2.weeks.ago\n 10:35:32.680597 trace.c:487 performance: 8.633068356 s: git command: /usr/local/libexec/git-core/git repack -d -l -A --unpack-unreachable=2.weeks.ago\n 10:35:32.683130 trace.c:487 performance: 0.000959377 s: git command: /usr/local/libexec/git-core/git prune --expire 2.weeks.ago\n 10:35:32.684401 trace.c:487 performance: 0.000180173 s: git command: /usr/local/libexec/git-core/git worktree prune --expire 3.months.ago\n 10:35:32.685730 trace.c:487 performance: 0.000263898 s: git command: /usr/local/libexec/git-core/git rerere gc\n 10:35:33.514816 trace.c:487 performance: 9.511597988 s: git command: git -C git-full.git/ gc\n elapsed: 0:09.51 max RSS: 358964k\n\n $ git clone --bare --filter=blob:none https://github.com/git/git git-partial.git\n $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-partial.git/ gc\n 10:35:47.637735 trace.c:487 performance: 0.000872539 s: git command: /usr/local/libexec/git-core/git pack-refs --all --prune\n 10:35:47.675498 trace.c:487 performance: 0.036246403 s: git command: /usr/local/libexec/git-core/git reflog expire --all\n Enumerating objects: 188205, done.\n Counting objects: 100% (188205/188205), done.\n Delta compression using up to 4 threads\n Compressing objects: 100% (66520/66520), done.\n Writing objects: 100% (188205/188205), done.\n Total 188205 (delta 119967), reused 188205 (delta 119967), pack-reused 0\n 10:35:50.081709 trace.c:487 performance: 2.402625839 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack\n 10:35:50.100131 read-cache.c:2315 performance: 0.000009979 s: read cache ./index\n 10:37:04.973541 trace.c:487 performance: 74.885793630 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack --keep-true-parents --honor-pack-keep --non-empty --all --reflog --indexed-objects --exclude-promisor-objects --unpack-unreachable=2.weeks.ago\n Removing duplicate objects: 100% (256/256), done.\n 10:37:07.482791 trace.c:487 performance: 79.804973525 s: git command: /usr/local/libexec/git-core/git repack -d -l -A --unpack-unreachable=2.weeks.ago\n 10:37:07.549333 trace.c:487 performance: 0.008025426 s: git command: /usr/local/libexec/git-core/git prune --expire 2.weeks.ago --exclude-promisor-objects\n 10:37:07.552499 trace.c:487 performance: 0.000362981 s: git command: /usr/local/libexec/git-core/git worktree prune --expire 3.months.ago\n 10:37:07.554521 trace.c:487 performance: 0.000273834 s: git command: /usr/local/libexec/git-core/git rerere gc\n 10:37:10.168233 trace.c:487 performance: 82.533331484 s: git command: git -C git-partial.git/ gc\n elapsed: 1:22.54 max RSS: 1891832k\n\nNotice the ~9s vs. 82s runtime and ~350M vs. 1.9G memory consumption\nincrease. What's going on here?\n\nAlso note that that second 'git pack-objects' invocation doesn't show\nany progress for ~75s.\n\nFWIW, doing the same in a 'tree:0' partial clone is fast.\n\n"},{"id":"420976","messageId":"gohp6ko8et3jdm.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"20210403090412.GH2271@szeder.dev","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-05T01:02:33Z","receivedAt":"2021-04-05T01:02:44Z","isPatch":false,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nSZEDER Gábor writes:\n\n> Hi,\n>\n> here are trace timings of running 'git gc' in a \"normal\" and in a\n> 'blob:none' partial clone:\n>\n> $ git clone --bare https://github.com/git/git git-full.git\n> $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-full.git/ gc\n> 10:35:24.007277 trace.c:487 performance: 0.001550225 s: git command: /usr/local/libexec/git-core/git pack-refs --all --prune\n> 10:35:24.044641 trace.c:487 performance: 0.035631270 s: git command: /usr/local/libexec/git-core/git reflog expire --all\n> 10:35:24.061070 read-cache.c:2315 performance: 0.000008506 s: read cache ./index\n> Enumerating objects: 305283, done.\n> Counting objects: 100% (305283/305283), done.\n> Delta compression using up to 4 threads\n> Compressing objects: 100% (75016/75016), done.\n> Writing objects: 100% (305283/305283), done.\n> Total 305283 (delta 227928), reused 305283 (delta 227928), pack-reused 0\n> 10:35:32.604546 trace.c:487 performance: 8.555651283 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946975-pack --keep-true-parents --honor-pack-keep --non-empty --all --reflog --indexed-objects --unpack-unreachable=2.weeks.ago\n> 10:35:32.680597 trace.c:487 performance: 8.633068356 s: git command: /usr/local/libexec/git-core/git repack -d -l -A --unpack-unreachable=2.weeks.ago\n> 10:35:32.683130 trace.c:487 performance: 0.000959377 s: git command: /usr/local/libexec/git-core/git prune --expire 2.weeks.ago\n> 10:35:32.684401 trace.c:487 performance: 0.000180173 s: git command: /usr/local/libexec/git-core/git worktree prune --expire 3.months.ago\n> 10:35:32.685730 trace.c:487 performance: 0.000263898 s: git command: /usr/local/libexec/git-core/git rerere gc\n> 10:35:33.514816 trace.c:487 performance: 9.511597988 s: git command: git -C git-full.git/ gc\n> elapsed: 0:09.51 max RSS: 358964k\n>\n> $ git clone --bare --filter=blob:none https://github.com/git/git git-partial.git\n> $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-partial.git/ gc\n> 10:35:47.637735 trace.c:487 performance: 0.000872539 s: git command: /usr/local/libexec/git-core/git pack-refs --all --prune\n> 10:35:47.675498 trace.c:487 performance: 0.036246403 s: git command: /usr/local/libexec/git-core/git reflog expire --all\n> Enumerating objects: 188205, done.\n> Counting objects: 100% (188205/188205), done.\n> Delta compression using up to 4 threads\n> Compressing objects: 100% (66520/66520), done.\n> Writing objects: 100% (188205/188205), done.\n> Total 188205 (delta 119967), reused 188205 (delta 119967), pack-reused 0\n> 10:35:50.081709 trace.c:487 performance: 2.402625839 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack\n> 10:35:50.100131 read-cache.c:2315 performance: 0.000009979 s: read cache ./index\n> 10:37:04.973541 trace.c:487 performance: 74.885793630 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack --keep-true-parents --honor-pack-keep --non-empty --all --reflog --indexed-objects --exclude-promisor-objects --unpack-unreachable=2.weeks.ago\n> Removing duplicate objects: 100% (256/256), done.\n> 10:37:07.482791 trace.c:487 performance: 79.804973525 s: git command: /usr/local/libexec/git-core/git repack -d -l -A --unpack-unreachable=2.weeks.ago\n> 10:37:07.549333 trace.c:487 performance: 0.008025426 s: git command: /usr/local/libexec/git-core/git prune --expire 2.weeks.ago --exclude-promisor-objects\n> 10:37:07.552499 trace.c:487 performance: 0.000362981 s: git command: /usr/local/libexec/git-core/git worktree prune --expire 3.months.ago\n> 10:37:07.554521 trace.c:487 performance: 0.000273834 s: git command: /usr/local/libexec/git-core/git rerere gc\n> 10:37:10.168233 trace.c:487 performance: 82.533331484 s: git command: git -C git-partial.git/ gc\n> elapsed: 1:22.54 max RSS: 1891832k\n>\n> Notice the ~9s vs. 82s runtime and ~350M vs. 1.9G memory consumption\n> increase. What's going on here?\n>\n> Also note that that second 'git pack-objects' invocation doesn't show\n> any progress for ~75s.\n>\n> FWIW, doing the same in a 'tree:0' partial clone is fast.\n\nI'm not expert on the area - by \"area\": the entire git code base :).\nHowever, I was intrigued by this performance numbers and decided to give\nit a try on the investigation, mostly for learning. While I'm not sure\nabout the solution of the problem, I decided to share it here with the\nhope that at least I'll be saving someone else time.\n\nWhen I was digging into the code and adding trace2_region_*() calls, I\nnotice most of the time spent on the `git gc` (for the reported\nsituation) was in:\n\n # In builtin/pack-objects.c\n static void get_object_list(int ac, const char **av)\n {\n ...\n if (unpack_unreachable)\n loosen_unused_packed_objects();\n ...\n }\n\nThe loosen_unused_packed_objects() will unpack unreachable objects as\nloose objects, and given that the partial cloned .pack file is\nincomplete, this result in writing a lot of loose objects in $GIT_DIR\nincreasing the execution time and memory consumption. This can be seen\nby watching the $GIT_DIR/objects/ during the `git gc` execution on the\npartial cloned repo. On the fully clone repository all the objects\nexist, at least on the fresh clone like in your report thus no object is\nloose from the .pack file.\n\nTo provide some insight in the magnitude of the written loose objects,\nI counted the number of objects that was being feed into\nforce_object_loose() with the following patch:\n\n-- >8 --\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 525c2d8552..f912b54a5f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3478,7 +3478,7 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n static void loosen_unused_packed_objects(void)\n {\n \tstruct packed_git *p;\n-\tuint32_t i;\n+\tuint32_t i, loosen_obj_counter = 0;\n \tstruct object_id oid;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n@@ -3492,10 +3492,13 @@ static void loosen_unused_packed_objects(void)\n \t\t\tnth_packed_object_id(&oid, p, i);\n \t\t\tif (!packlist_find(&to_pack, &oid) &&\n \t\t\t !has_sha1_pack_kept_or_nonlocal(&oid) &&\n-\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime))\n+\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime) &&\n+\t\t\t ++loosen_obj_counter)\n \t\t\t\tif (force_object_loose(&oid, p->mtime))\n \t\t\t\t\tdie(_(\"unable to force loose object\"));\n \t\t}\n+\t\tfprintf(stderr, \"loosen_unused_packed_objects() total of objects: %d\\n\", p->num_objects);\n+\t\tfprintf(stderr, \"loosen_unused_packed_objects() objects that is being loosed: %d\\n\", loosen_obj_counter);\n \t}\n }\n \n-- >8 --\n\nRunning on a fresh and fully cloned git.git repo, there are (obviously)\n0 unreachable objects that are being written as loose objects:\n\n $ bin-wrappers/git clone --bare https://github.com/git/git git-full.git\n $ time --format='elapsed: %E | max RSS: %Mk' bin-wrappers/git -C git-full.git gc\n loosen_unused_packed_objects() total of objects: 305292\n loosen_unused_packed_objects() objects that is being loosed: 0\n Enumerating objects: 305292, done.\n Counting objects: 100% (305292/305292), done.\n Delta compression using up to 8 threads\n Compressing objects: 100% (75035/75035), done.\n Writing objects: 100% (305292/305292), done.\n Selecting bitmap commits: 63006, done.\n Building bitmaps: 100% (317/317), done.\n Total 305292 (delta 227918), reused 305292 (delta 227918), pack-reused 0\n elapsed: 0:09.23 | max RSS: 438628k\n\nOn the other hand, when running on a fresh and partial cloned repo, we\ncan see that all the objects (at least according to my findings) are\nbeing move out of .pack file into a loose object.\n\n $ bin-wrappers/git clone --bare --filter=blob:none https://github.com/git/git git-partial.git\n $ time --format='elapsed: %E | max RSS: %Mk' bin-wrappers/git -C git-partial.git gc\n Enumerating objects: 188213, done.\n Counting objects: 100% (188213/188213), done.\n Delta compression using up to 8 threads\n Compressing objects: 100% (66524/66524), done.\n Writing objects: 100% (188213/188213), done.\n Total 188213 (delta 119971), reused 188213 (delta 119971), pack-reused 0\n loosen_unused_packed_objects() total of objects: 188213\n loosen_unused_packed_objects() objects that is being loosed: 188213\n loosen_unused_packed_objects() total of objects: 188213\n loosen_unused_packed_objects() objects that is being loosed: 376426\n Removing duplicate objects: 100% (256/256), done.\n elapsed: 3:24.86 | max RSS: 2085552k\n\nAnother interesting thing is, the loosen_unused_packed_objects()\nfunction is being called twice because the function loads all packs\nfiles, via get_all_packs(), which will return the .temp-*pack file that\nis created by the `git pack-objects` child process from `git gc`:\n\n git pack-objects ... --delta-base-offset objects/pack/.tmp-82853-pack ...\n\nFor this specific case, this make the situation worse as we end up\nprocessing the object from both packfiles. Also, the second\nexecution, which operates on the normal .pack file, is processing 2x\n\"objects\" (in terms of counting the execution of\nforce_object_loose()), I couldn't quite figure out why.\n\nI'm not entirely sure about this (not this late in the day), but it seems to\nme that we should simply skip the \"missing\" (promisor) files when\noperating on a partial clone.\n\nPerhaps something like:\n\n--- >8 ---\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 525c2d8552..fedf58323d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n {\n if (!unpack_unreachable_expiration)\n return 0;\n+ if (exclude_promisor_objects && is_promisor_object(oid))\n+ return 1;\n if (mtime > unpack_unreachable_expiration)\n return 0;\n if (oid_array_lookup(&recent_objects, oid) >= 0)\n--- >8 ---\n\nI'll try to prepare a patch for this change with proper testing, if this\nturns out to be proper way to handle partial clone repository.\n\nA quick benchmark did show some promising result:\n\n # built from: 2e36527f23 (The sixth batch, 2021-04-02)\n Benchmark #1: ./bin-wrappers/git -C git.git gc\n Time (mean ± σ): 135.669 s ± 0.665 s [User: 42.789 s, System: 91.332 s]\n Range (min … max): 134.905 s … 136.115 s 3 runs\n\n # built from: 2e36527f23 + minor patch (from above)\n Benchmark #2: ./bin-wrappers/git -C git.git gc\n Time (mean ± σ): 12.586 s ± 0.031 s [User: 11.462 s, System: 1.365 s]\n Range (min … max): 12.553 s … 12.616 s 3 runs\n\n Summary:\n 'Benchmark #2' ran 10.78 ± 0.06 times faster than 'Benchmark #1'\n\n\n-- \nThanks\nRafael\n"},{"id":"421154","messageId":"YG4hfge2y/AmcklZ@coredump.intra.peff.net","threadId":"55430","inReplyTo":"gohp6ko8et3jdm.fsf@cpm12071.fritz.box","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-07T21:17:50Z","receivedAt":"2021-04-07T21:17:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 05, 2021 at 03:02:33AM +0200, Rafael Silva wrote:\n\n> When I was digging into the code and adding trace2_region_*() calls, I\n> notice most of the time spent on the `git gc` (for the reported\n> situation) was in:\n> \n> # In builtin/pack-objects.c\n> static void get_object_list(int ac, const char **av)\n> {\n> ...\n> if (unpack_unreachable)\n> loosen_unused_packed_objects();\n> ...\n> }\n\nYeah, good find.\n\nThis is my first time looking at the repacking strategy for partial\nclones. It looks like we run an initial pack-objects to cover all the\npromisor objects, and then do the \"real\" repack for everything else,\nwith \"--exclude-promisor-objects\".\n\nThe purpose of loosen_unused_packed_objects() is to catch any objects\nthat will be lost when our caller deletes all of the packs. But in this\ncase, those promisor objects are in a pack which won't be deleted, so\nthey should not be included.\n\n> Another interesting thing is, the loosen_unused_packed_objects()\n> function is being called twice because the function loads all packs\n> files, via get_all_packs(), which will return the .temp-*pack file that\n> is created by the `git pack-objects` child process from `git gc`:\n> \n> git pack-objects ... --delta-base-offset objects/pack/.tmp-82853-pack ...\n\nYes, this is the \"promisor\" pack created by git-repack. It seems like\ngit-repack should tell pack-objects about the new pack with --keep-pack,\nso that we know it is not going to be deleted.\n\nThat would also solve the rest of the problem, I _think_. In your\nsuggestion here:\n\n> I'm not entirely sure about this (not this late in the day), but it seems to\n> me that we should simply skip the \"missing\" (promisor) files when\n> operating on a partial clone.\n> \n> Perhaps something like:\n> \n> --- >8 ---\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 525c2d8552..fedf58323d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n> {\n> if (!unpack_unreachable_expiration)\n> return 0;\n> + if (exclude_promisor_objects && is_promisor_object(oid))\n> + return 1;\n> if (mtime > unpack_unreachable_expiration)\n> return 0;\n> if (oid_array_lookup(&recent_objects, oid) >= 0)\n> --- >8 ---\n\nyou are avoiding writing out the file. But we should realize much\nearlier that it is not something we need to even consider loosening.\n\nIn the loop in loosen_unused_packed_objects(), we skip packs that are\nmarked as \"keep\", so we'd skip the new promisor pack entirely. But we'd\nstill see all these objects in the _old_ promisor pack. However, for\neach object there, we call has_sha1_pack_kept_or_nonlocal(), so that\nwould likewise realize that each object is already being kept in the\nother pack.\n\nSomething like this seems to work, but I only lightly tested it, and it\ncould probably use some refactoring to make it less horrible:\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex fdee8e4578..457525953a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -574,6 +574,23 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\trepack_promisor_objects(&po_args, &names);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n+\t\t\t/*\n+\t\t\t * tell pack-objects about our new promisor pack, which\n+\t\t\t * we will also be keeping\n+\t\t\t */\n+\t\t\tfor_each_string_list_item(item, &names) {\n+\t\t\t\t/*\n+\t\t\t\t * yuck, we seem to only have the name with the\n+\t\t\t\t * packdir prefixed\n+\t\t\t\t */\n+\t\t\t\tconst char *prefix;\n+\t\t\t\tif (!skip_prefix(packtmp, packdir, &prefix) ||\n+\t\t\t\t *prefix++ != '/')\n+\t\t\t\t\tBUG(\"confused by packtmp\");\n+\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n+\t\t\t\t\t prefix, item->string);\n+\t\t\t}\n+\n \t\t\tif (unpack_unreachable) {\n \t\t\t\tstrvec_pushf(&cmd.args,\n \t\t\t\t\t \"--unpack-unreachable=%s\",\n\nDo you want to try to work with that?\n\n> A quick benchmark did show some promising result:\n> \n> # built from: 2e36527f23 (The sixth batch, 2021-04-02)\n> Benchmark #1: ./bin-wrappers/git -C git.git gc\n> Time (mean ± σ): 135.669 s ± 0.665 s [User: 42.789 s, System: 91.332 s]\n> Range (min … max): 134.905 s … 136.115 s 3 runs\n> \n> # built from: 2e36527f23 + minor patch (from above)\n> Benchmark #2: ./bin-wrappers/git -C git.git gc\n> Time (mean ± σ): 12.586 s ± 0.031 s [User: 11.462 s, System: 1.365 s]\n> Range (min … max): 12.553 s … 12.616 s 3 runs\n> \n> Summary:\n> 'Benchmark #2' ran 10.78 ± 0.06 times faster than 'Benchmark #1'\n\nIt's still quite a bit slower than a non-partial clone because the\ntraversal with --exclude-promisor-objects is slow. I think that's\nbecause it has to open up all of the objects in the promisor pack to see\nwhat they refer to. I don't know if we can do better (and it's largely\nan orthogonal problem to what you're solving here, so it probably makes\nsense to just punt on it for now).\n\n-Peff\n"},{"id":"421189","messageId":"20210408000242.2465219-1-jonathantanmy@google.com","threadId":"55430","inReplyTo":"YG4hfge2y/AmcklZ@coredump.intra.peff.net","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-08T00:02:41Z","receivedAt":"2021-04-08T00:02:53Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Mon, Apr 05, 2021 at 03:02:33AM +0200, Rafael Silva wrote:\n> \n> > When I was digging into the code and adding trace2_region_*() calls, I\n> > notice most of the time spent on the `git gc` (for the reported\n> > situation) was in:\n> > \n> > # In builtin/pack-objects.c\n> > static void get_object_list(int ac, const char **av)\n> > {\n> > ...\n> > if (unpack_unreachable)\n> > loosen_unused_packed_objects();\n> > ...\n> > }\n> \n> Yeah, good find.\n\nAgreed!\n\n> This is my first time looking at the repacking strategy for partial\n> clones. It looks like we run an initial pack-objects to cover all the\n> promisor objects, and then do the \"real\" repack for everything else,\n> with \"--exclude-promisor-objects\".\n\nThat is correct - we need two separate packs because one of them needs\nto be accompanied by a separate \".promisor\" file to show that those\nobjects are from the promisor remote.\n\n> The purpose of loosen_unused_packed_objects() is to catch any objects\n> that will be lost when our caller deletes all of the packs. But in this\n> case, those promisor objects are in a pack which won't be deleted, so\n> they should not be included.\n\nMakes sense.\n\n> > I'm not entirely sure about this (not this late in the day), but it seems to\n> > me that we should simply skip the \"missing\" (promisor) files when\n> > operating on a partial clone.\n> > \n> > Perhaps something like:\n> > \n> > --- >8 ---\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 525c2d8552..fedf58323d 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n> > {\n> > if (!unpack_unreachable_expiration)\n> > return 0;\n> > + if (exclude_promisor_objects && is_promisor_object(oid))\n> > + return 1;\n> > if (mtime > unpack_unreachable_expiration)\n> > return 0;\n> > if (oid_array_lookup(&recent_objects, oid) >= 0)\n> > --- >8 ---\n\nI think this will work. The only thing we might need to watch out for is\nthat if we repack with --exclude-promisor-objects, some might say that\nevery excluded object should be loosened. I don't think that's true in\nthis case, because (1) the object is not unreachable and the argument\ntalks about loosening unreachable objects, (2) promisor objects can be\nre-obtained from the promisor remote anyway. So I think we're fine.\n\nBut I think Peff suggested something better below.\n\n> In the loop in loosen_unused_packed_objects(), we skip packs that are\n> marked as \"keep\", so we'd skip the new promisor pack entirely. But we'd\n> still see all these objects in the _old_ promisor pack. However, for\n> each object there, we call has_sha1_pack_kept_or_nonlocal(), so that\n> would likewise realize that each object is already being kept in the\n> other pack.\n> \n> Something like this seems to work, but I only lightly tested it, and it\n> could probably use some refactoring to make it less horrible:\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index fdee8e4578..457525953a 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -574,6 +574,23 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\trepack_promisor_objects(&po_args, &names);\n> \n> \t\tif (existing_packs.nr && delete_redundant) {\n> +\t\t\t/*\n> +\t\t\t * tell pack-objects about our new promisor pack, which\n> +\t\t\t * we will also be keeping\n> +\t\t\t */\n> +\t\t\tfor_each_string_list_item(item, &names) {\n> +\t\t\t\t/*\n> +\t\t\t\t * yuck, we seem to only have the name with the\n> +\t\t\t\t * packdir prefixed\n> +\t\t\t\t */\n> +\t\t\t\tconst char *prefix;\n> +\t\t\t\tif (!skip_prefix(packtmp, packdir, &prefix) ||\n> +\t\t\t\t *prefix++ != '/')\n> +\t\t\t\t\tBUG(\"confused by packtmp\");\n> +\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n> +\t\t\t\t\t prefix, item->string);\n> +\t\t\t}\n> +\n> \t\t\tif (unpack_unreachable) {\n> \t\t\t\tstrvec_pushf(&cmd.args,\n> \t\t\t\t\t \"--unpack-unreachable=%s\",\n> \n> Do you want to try to work with that?\n\nIt seems to me that this would work. The only part I was confused about\nis \"packtmp\", but that is just the pack directory plus a specific\nprefix, so \"pack-objects\" will indeed see that packfile as being part of\nthe repository - no problem.\n\nrepack_promisor_objects() might be able to be refactored to provide the\nnames in a format that we want, but looking at it, I don't think it's\npossible (it just uses \"packtmp\", so we have the same \"packtmp\"\nproblem).\n"},{"id":"421191","messageId":"YG5PyJHqk/BjeD84@coredump.intra.peff.net","threadId":"55430","inReplyTo":"20210408000242.2465219-1-jonathantanmy@google.com","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-08T00:35:20Z","receivedAt":"2021-04-08T00:35:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 07, 2021 at 05:02:41PM -0700, Jonathan Tan wrote:\n\n> It seems to me that this would work. The only part I was confused about\n> is \"packtmp\", but that is just the pack directory plus a specific\n> prefix, so \"pack-objects\" will indeed see that packfile as being part of\n> the repository - no problem.\n> \n> repack_promisor_objects() might be able to be refactored to provide the\n> names in a format that we want, but looking at it, I don't think it's\n> possible (it just uses \"packtmp\", so we have the same \"packtmp\"\n> problem).\n\nYeah, that was the refactoring I alluded to. I think the earlier code\nshould keep the \".tmp-%d\" portion in a separate string, and then\nconstruct packtmp from that. And then we don't have to try to recover it\nfrom the concatenated string.\n\n-Peff\n"},{"id":"421584","messageId":"20210411105903.GG2947267@szeder.dev","threadId":"55430","inReplyTo":"gohp6ko8et3jdm.fsf@cpm12071.fritz.box","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-11T10:59:03Z","receivedAt":"2021-04-11T10:59:09Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 05, 2021 at 03:02:33AM +0200, Rafael Silva wrote:\n> > here are trace timings of running 'git gc' in a \"normal\" and in a\n> > 'blob:none' partial clone:\n> >\n> > $ git clone --bare https://github.com/git/git git-full.git\n> > $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-full.git/ gc\n[...]\n> > elapsed: 0:09.51 max RSS: 358964k\n> >\n> > $ git clone --bare --filter=blob:none https://github.com/git/git git-partial.git\n> > $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-partial.git/ gc\n> > 10:35:47.637735 trace.c:487 performance: 0.000872539 s: git command: /usr/local/libexec/git-core/git pack-refs --all --prune\n> > 10:35:47.675498 trace.c:487 performance: 0.036246403 s: git command: /usr/local/libexec/git-core/git reflog expire --all\n> > Enumerating objects: 188205, done.\n> > Counting objects: 100% (188205/188205), done.\n> > Delta compression using up to 4 threads\n> > Compressing objects: 100% (66520/66520), done.\n> > Writing objects: 100% (188205/188205), done.\n> > Total 188205 (delta 119967), reused 188205 (delta 119967), pack-reused 0\n> > 10:35:50.081709 trace.c:487 performance: 2.402625839 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack\n> > 10:35:50.100131 read-cache.c:2315 performance: 0.000009979 s: read cache ./index\n> > 10:37:04.973541 trace.c:487 performance: 74.885793630 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack --keep-true-parents --honor-pack-keep --non-empty --all --reflog --indexed-objects --exclude-promisor-objects --unpack-unreachable=2.weeks.ago\n> > Removing duplicate objects: 100% (256/256), done.\n> > 10:37:07.482791 trace.c:487 performance: 79.804973525 s: git command: /usr/local/libexec/git-core/git repack -d -l -A --unpack-unreachable=2.weeks.ago\n> > 10:37:07.549333 trace.c:487 performance: 0.008025426 s: git command: /usr/local/libexec/git-core/git prune --expire 2.weeks.ago --exclude-promisor-objects\n> > 10:37:07.552499 trace.c:487 performance: 0.000362981 s: git command: /usr/local/libexec/git-core/git worktree prune --expire 3.months.ago\n> > 10:37:07.554521 trace.c:487 performance: 0.000273834 s: git command: /usr/local/libexec/git-core/git rerere gc\n> > 10:37:10.168233 trace.c:487 performance: 82.533331484 s: git command: git -C git-partial.git/ gc\n> > elapsed: 1:22.54 max RSS: 1891832k\n> >\n> > Notice the ~9s vs. 82s runtime and ~350M vs. 1.9G memory consumption\n> > increase. What's going on here?\n> >\n> > Also note that that second 'git pack-objects' invocation doesn't show\n> > any progress for ~75s.\n> >\n> > FWIW, doing the same in a 'tree:0' partial clone is fast.\n> \n> I'm not expert on the area - by \"area\": the entire git code base :).\n> However, I was intrigued by this performance numbers and decided to give\n> it a try on the investigation, mostly for learning.\n\nThat's the spirit!\n\n> While I'm not sure\n> about the solution of the problem, I decided to share it here with the\n> hope that at least I'll be saving someone else time.\n> \n> When I was digging into the code and adding trace2_region_*() calls, I\n> notice most of the time spent on the `git gc` (for the reported\n> situation) was in:\n> \n> # In builtin/pack-objects.c\n> static void get_object_list(int ac, const char **av)\n> {\n> ...\n> if (unpack_unreachable)\n> loosen_unused_packed_objects();\n> ...\n> }\n> \n> The loosen_unused_packed_objects() will unpack unreachable objects as\n> loose objects, and given that the partial cloned .pack file is\n> incomplete, this result in writing a lot of loose objects in $GIT_DIR\n> increasing the execution time and memory consumption. This can be seen\n> by watching the $GIT_DIR/objects/ during the `git gc` execution on the\n> partial cloned repo.\n\nIndeed, that 'blob:none' partial clone grew in size temporarily to\nover 1.3GB during repacking, but luckily all those unnecessarily\nloosened objects were removed at the end. I first noticed this issue\nwhile attempting to repack a considerably larger partial-cloned\nrepository, which I aborted because it ate up all the memory... I\nsuppose that even if it didn't use that much memory, it would\neventually run out of available disk space for all those loose objects\nanyway...\n\n\n> I'm not entirely sure about this (not this late in the day), but it seems to\n> me that we should simply skip the \"missing\" (promisor) files when\n> operating on a partial clone.\n> \n> Perhaps something like:\n> \n> --- >8 ---\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 525c2d8552..fedf58323d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n> {\n> if (!unpack_unreachable_expiration)\n> return 0;\n> + if (exclude_promisor_objects && is_promisor_object(oid))\n> + return 1;\n> if (mtime > unpack_unreachable_expiration)\n> return 0;\n> if (oid_array_lookup(&recent_objects, oid) >= 0)\n> --- >8 ---\n> \n> I'll try to prepare a patch for this change with proper testing, if this\n> turns out to be proper way to handle partial clone repository.\n> \n> A quick benchmark did show some promising result:\n> \n> # built from: 2e36527f23 (The sixth batch, 2021-04-02)\n> Benchmark #1: ./bin-wrappers/git -C git.git gc\n> Time (mean ± σ): 135.669 s ± 0.665 s [User: 42.789 s, System: 91.332 s]\n> Range (min … max): 134.905 s … 136.115 s 3 runs\n> \n> # built from: 2e36527f23 + minor patch (from above)\n> Benchmark #2: ./bin-wrappers/git -C git.git gc\n> Time (mean ± σ): 12.586 s ± 0.031 s [User: 11.462 s, System: 1.365 s]\n> Range (min … max): 12.553 s … 12.616 s 3 runs\n> \n> Summary:\n> 'Benchmark #2' ran 10.78 ± 0.06 times faster than 'Benchmark #1'\n\nI can confirm that you change speeds up repacking considerably and it\ncompletely eliminates that temporary repo size explision due to\nunpacked objects, but, alas, it doesn't seem to reduce the memory\nusage.\n\nThanks,\nGábor\n\n"},{"id":"421638","messageId":"gohp6kim4sf07b.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"YG4hfge2y/AmcklZ@coredump.intra.peff.net","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-12T07:09:46Z","receivedAt":"2021-04-12T07:09:52Z","isPatch":false,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJeff King writes:\n\n> On Mon, Apr 05, 2021 at 03:02:33AM +0200, Rafael Silva wrote:\n>\n>> I'm not entirely sure about this (not this late in the day), but it seems to\n>> me that we should simply skip the \"missing\" (promisor) files when\n>> operating on a partial clone.\n>> \n>> Perhaps something like:\n>> \n>> --- >8 ---\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index 525c2d8552..fedf58323d 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n>> {\n>> if (!unpack_unreachable_expiration)\n>> return 0;\n>> + if (exclude_promisor_objects && is_promisor_object(oid))\n>> + return 1;\n>> if (mtime > unpack_unreachable_expiration)\n>> return 0;\n>> if (oid_array_lookup(&recent_objects, oid) >= 0)\n>> --- >8 ---\n>\n> you are avoiding writing out the file. But we should realize much\n> earlier that it is not something we need to even consider loosening.\n>\n> In the loop in loosen_unused_packed_objects(), we skip packs that are\n> marked as \"keep\", so we'd skip the new promisor pack entirely. But we'd\n> still see all these objects in the _old_ promisor pack. However, for\n> each object there, we call has_sha1_pack_kept_or_nonlocal(), so that\n> would likewise realize that each object is already being kept in the\n> other pack.\n>\n\nAgreed. Realizing sooner that we shouldn't even consider loosening the\nobjects from the packfile it's better solution.\n\n> Something like this seems to work, but I only lightly tested it, and it\n> could probably use some refactoring to make it less horrible:\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index fdee8e4578..457525953a 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -574,6 +574,23 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\trepack_promisor_objects(&po_args, &names);\n> \n> \t\tif (existing_packs.nr && delete_redundant) {\n> +\t\t\t/*\n> +\t\t\t * tell pack-objects about our new promisor pack, which\n> +\t\t\t * we will also be keeping\n> +\t\t\t */\n> +\t\t\tfor_each_string_list_item(item, &names) {\n> +\t\t\t\t/*\n> +\t\t\t\t * yuck, we seem to only have the name with the\n> +\t\t\t\t * packdir prefixed\n> +\t\t\t\t */\n> +\t\t\t\tconst char *prefix;\n> +\t\t\t\tif (!skip_prefix(packtmp, packdir, &prefix) ||\n> +\t\t\t\t *prefix++ != '/')\n> +\t\t\t\t\tBUG(\"confused by packtmp\");\n> +\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n> +\t\t\t\t\t prefix, item->string);\n> +\t\t\t}\n> +\n> \t\t\tif (unpack_unreachable) {\n> \t\t\t\tstrvec_pushf(&cmd.args,\n> \t\t\t\t\t \"--unpack-unreachable=%s\",\n>\n> Do you want to try to work with that?\n>\n\nYes, I'll try to work with that, together with refactoring that you\nmentioned in the code and the other replies.\n\nThanks for the suggestion.\n\n>> A quick benchmark did show some promising result:\n>> \n>> # built from: 2e36527f23 (The sixth batch, 2021-04-02)\n>> Benchmark #1: ./bin-wrappers/git -C git.git gc\n>> Time (mean ± σ): 135.669 s ± 0.665 s [User: 42.789 s, System: 91.332 s]\n>> Range (min … max): 134.905 s … 136.115 s 3 runs\n>> \n>> # built from: 2e36527f23 + minor patch (from above)\n>> Benchmark #2: ./bin-wrappers/git -C git.git gc\n>> Time (mean ± σ): 12.586 s ± 0.031 s [User: 11.462 s, System: 1.365 s]\n>> Range (min … max): 12.553 s … 12.616 s 3 runs\n>> \n>> Summary:\n>> 'Benchmark #2' ran 10.78 ± 0.06 times faster than 'Benchmark #1'\n>\n> It's still quite a bit slower than a non-partial clone because the\n> traversal with --exclude-promisor-objects is slow. I think that's\n> because it has to open up all of the objects in the promisor pack to see\n> what they refer to. I don't know if we can do better (and it's largely\n> an orthogonal problem to what you're solving here, so it probably makes\n> sense to just punt on it for now).\n>\n> -Peff\n\nMake sense.\n\n-- \nThanks\nRafael\n"},{"id":"421639","messageId":"gohp6k4kgcdjm6.fsf@gmail.com","threadId":"55430","inReplyTo":"20210411105903.GG2947267@szeder.dev","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-12T07:53:23Z","receivedAt":"2021-04-12T07:53:31Z","isPatch":false,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nSZEDER Gábor writes:\n\n> On Mon, Apr 05, 2021 at 03:02:33AM +0200, Rafael Silva wrote:\n>> > here are trace timings of running 'git gc' in a \"normal\" and in a\n>> > 'blob:none' partial clone:\n>> >\n>> > $ git clone --bare https://github.com/git/git git-full.git\n>> > $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-full.git/ gc\n> [...]\n>> > elapsed: 0:09.51 max RSS: 358964k\n>> >\n>> > $ git clone --bare --filter=blob:none https://github.com/git/git git-partial.git\n>> > $ GIT_TRACE_PERFORMANCE=2 /usr/bin/time --format='elapsed: %E max RSS: %Mk' git -C git-partial.git/ gc\n>> > 10:35:47.637735 trace.c:487 performance: 0.000872539 s: git command: /usr/local/libexec/git-core/git pack-refs --all --prune\n>> > 10:35:47.675498 trace.c:487 performance: 0.036246403 s: git command: /usr/local/libexec/git-core/git reflog expire --all\n>> > Enumerating objects: 188205, done.\n>> > Counting objects: 100% (188205/188205), done.\n>> > Delta compression using up to 4 threads\n>> > Compressing objects: 100% (66520/66520), done.\n>> > Writing objects: 100% (188205/188205), done.\n>> > Total 188205 (delta 119967), reused 188205 (delta 119967), pack-reused 0\n>> > 10:35:50.081709 trace.c:487 performance: 2.402625839 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack\n>> > 10:35:50.100131 read-cache.c:2315 performance: 0.000009979 s: read cache ./index\n>> > 10:37:04.973541 trace.c:487 performance: 74.885793630 s: git command: /usr/local/libexec/git-core/git pack-objects --local --delta-base-offset objects/pack/.tmp-2946990-pack --keep-true-parents --honor-pack-keep --non-empty --all --reflog --indexed-objects --exclude-promisor-objects --unpack-unreachable=2.weeks.ago\n>> > Removing duplicate objects: 100% (256/256), done.\n>> > 10:37:07.482791 trace.c:487 performance: 79.804973525 s: git command: /usr/local/libexec/git-core/git repack -d -l -A --unpack-unreachable=2.weeks.ago\n>> > 10:37:07.549333 trace.c:487 performance: 0.008025426 s: git command: /usr/local/libexec/git-core/git prune --expire 2.weeks.ago --exclude-promisor-objects\n>> > 10:37:07.552499 trace.c:487 performance: 0.000362981 s: git command: /usr/local/libexec/git-core/git worktree prune --expire 3.months.ago\n>> > 10:37:07.554521 trace.c:487 performance: 0.000273834 s: git command: /usr/local/libexec/git-core/git rerere gc\n>> > 10:37:10.168233 trace.c:487 performance: 82.533331484 s: git command: git -C git-partial.git/ gc\n>> > elapsed: 1:22.54 max RSS: 1891832k\n>> >\n>> > Notice the ~9s vs. 82s runtime and ~350M vs. 1.9G memory consumption\n>> > increase. What's going on here?\n>> >\n>> > Also note that that second 'git pack-objects' invocation doesn't show\n>> > any progress for ~75s.\n>> >\n>> > FWIW, doing the same in a 'tree:0' partial clone is fast.\n>> \n>> I'm not expert on the area - by \"area\": the entire git code base :).\n>> However, I was intrigued by this performance numbers and decided to give\n>> it a try on the investigation, mostly for learning.\n>\n> That's the spirit!\n>\n\n:)\n\n>> While I'm not sure\n>> about the solution of the problem, I decided to share it here with the\n>> hope that at least I'll be saving someone else time.\n>> \n>> When I was digging into the code and adding trace2_region_*() calls, I\n>> notice most of the time spent on the `git gc` (for the reported\n>> situation) was in:\n>> \n>> # In builtin/pack-objects.c\n>> static void get_object_list(int ac, const char **av)\n>> {\n>> ...\n>> if (unpack_unreachable)\n>> loosen_unused_packed_objects();\n>> ...\n>> }\n>> \n>> The loosen_unused_packed_objects() will unpack unreachable objects as\n>> loose objects, and given that the partial cloned .pack file is\n>> incomplete, this result in writing a lot of loose objects in $GIT_DIR\n>> increasing the execution time and memory consumption. This can be seen\n>> by watching the $GIT_DIR/objects/ during the `git gc` execution on the\n>> partial cloned repo.\n>\n> Indeed, that 'blob:none' partial clone grew in size temporarily to\n> over 1.3GB during repacking, but luckily all those unnecessarily\n> loosened objects were removed at the end. I first noticed this issue\n> while attempting to repack a considerably larger partial-cloned\n> repository, which I aborted because it ate up all the memory... I\n> suppose that even if it didn't use that much memory, it would\n> eventually run out of available disk space for all those loose objects\n> anyway...\n>\n>\n>> I'm not entirely sure about this (not this late in the day), but it seems to\n>> me that we should simply skip the \"missing\" (promisor) files when\n>> operating on a partial clone.\n>> \n>> Perhaps something like:\n>> \n>> --- >8 ---\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index 525c2d8552..fedf58323d 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n>> {\n>> if (!unpack_unreachable_expiration)\n>> return 0;\n>> + if (exclude_promisor_objects && is_promisor_object(oid))\n>> + return 1;\n>> if (mtime > unpack_unreachable_expiration)\n>> return 0;\n>> if (oid_array_lookup(&recent_objects, oid) >= 0)\n>> --- >8 ---\n>> \n>> I'll try to prepare a patch for this change with proper testing, if this\n>> turns out to be proper way to handle partial clone repository.\n>> \n>> A quick benchmark did show some promising result:\n>> \n>> # built from: 2e36527f23 (The sixth batch, 2021-04-02)\n>> Benchmark #1: ./bin-wrappers/git -C git.git gc\n>> Time (mean ± σ): 135.669 s ± 0.665 s [User: 42.789 s, System: 91.332 s]\n>> Range (min … max): 134.905 s … 136.115 s 3 runs\n>> \n>> # built from: 2e36527f23 + minor patch (from above)\n>> Benchmark #2: ./bin-wrappers/git -C git.git gc\n>> Time (mean ± σ): 12.586 s ± 0.031 s [User: 11.462 s, System: 1.365 s]\n>> Range (min … max): 12.553 s … 12.616 s 3 runs\n>> \n>> Summary:\n>> 'Benchmark #2' ran 10.78 ± 0.06 times faster than 'Benchmark #1'\n>\n> I can confirm that you change speeds up repacking considerably and it\n> completely eliminates that temporary repo size explision due to\n> unpacked objects, but, alas, it doesn't seem to reduce the memory\n> usage.\n>\n> Thanks,\n> Gábor\n\nThanks for confirming it. The memory usage, indeed, is almost the same\nafter the changes. Nevertheless, Peff suggested a better approach to\naddressing this issue, by realizing earlier that we should even\nconsider loosening the objects from the pack.\n\nI'll try to work with that approach and see how this apply to memory\nusage.\n\n-- \nThanks\nRafael\n"},{"id":"421815","messageId":"20210412213653.GH2947267@szeder.dev","threadId":"55430","inReplyTo":"YG4hfge2y/AmcklZ@coredump.intra.peff.net","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-12T21:36:53Z","receivedAt":"2021-04-12T21:37:00Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Apr 07, 2021 at 05:17:50PM -0400, Jeff King wrote:\n> On Mon, Apr 05, 2021 at 03:02:33AM +0200, Rafael Silva wrote:\n> \n> > When I was digging into the code and adding trace2_region_*() calls, I\n> > notice most of the time spent on the `git gc` (for the reported\n> > situation) was in:\n> > \n> > # In builtin/pack-objects.c\n> > static void get_object_list(int ac, const char **av)\n> > {\n> > ...\n> > if (unpack_unreachable)\n> > loosen_unused_packed_objects();\n> > ...\n> > }\n> \n> Yeah, good find.\n> \n> This is my first time looking at the repacking strategy for partial\n> clones. It looks like we run an initial pack-objects to cover all the\n> promisor objects, and then do the \"real\" repack for everything else,\n> with \"--exclude-promisor-objects\".\n> \n> The purpose of loosen_unused_packed_objects() is to catch any objects\n> that will be lost when our caller deletes all of the packs. But in this\n> case, those promisor objects are in a pack which won't be deleted, so\n> they should not be included.\n> \n> > Another interesting thing is, the loosen_unused_packed_objects()\n> > function is being called twice because the function loads all packs\n> > files, via get_all_packs(), which will return the .temp-*pack file that\n> > is created by the `git pack-objects` child process from `git gc`:\n> > \n> > git pack-objects ... --delta-base-offset objects/pack/.tmp-82853-pack ...\n> \n> Yes, this is the \"promisor\" pack created by git-repack. It seems like\n> git-repack should tell pack-objects about the new pack with --keep-pack,\n> so that we know it is not going to be deleted.\n> \n> That would also solve the rest of the problem, I _think_. In your\n> suggestion here:\n> \n> > I'm not entirely sure about this (not this late in the day), but it seems to\n> > me that we should simply skip the \"missing\" (promisor) files when\n> > operating on a partial clone.\n> > \n> > Perhaps something like:\n> > \n> > --- >8 ---\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 525c2d8552..fedf58323d 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -3468,6 +3468,8 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n> > {\n> > if (!unpack_unreachable_expiration)\n> > return 0;\n> > + if (exclude_promisor_objects && is_promisor_object(oid))\n> > + return 1;\n> > if (mtime > unpack_unreachable_expiration)\n> > return 0;\n> > if (oid_array_lookup(&recent_objects, oid) >= 0)\n> > --- >8 ---\n> \n> you are avoiding writing out the file. But we should realize much\n> earlier that it is not something we need to even consider loosening.\n> \n> In the loop in loosen_unused_packed_objects(), we skip packs that are\n> marked as \"keep\", so we'd skip the new promisor pack entirely. But we'd\n> still see all these objects in the _old_ promisor pack. However, for\n> each object there, we call has_sha1_pack_kept_or_nonlocal(), so that\n> would likewise realize that each object is already being kept in the\n> other pack.\n> \n> Something like this seems to work, but I only lightly tested it, and it\n> could probably use some refactoring to make it less horrible:\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index fdee8e4578..457525953a 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -574,6 +574,23 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\trepack_promisor_objects(&po_args, &names);\n> \n> \t\tif (existing_packs.nr && delete_redundant) {\n> +\t\t\t/*\n> +\t\t\t * tell pack-objects about our new promisor pack, which\n> +\t\t\t * we will also be keeping\n> +\t\t\t */\n> +\t\t\tfor_each_string_list_item(item, &names) {\n> +\t\t\t\t/*\n> +\t\t\t\t * yuck, we seem to only have the name with the\n> +\t\t\t\t * packdir prefixed\n> +\t\t\t\t */\n> +\t\t\t\tconst char *prefix;\n> +\t\t\t\tif (!skip_prefix(packtmp, packdir, &prefix) ||\n> +\t\t\t\t *prefix++ != '/')\n> +\t\t\t\t\tBUG(\"confused by packtmp\");\n> +\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n> +\t\t\t\t\t prefix, item->string);\n> +\t\t\t}\n> +\n> \t\t\tif (unpack_unreachable) {\n> \t\t\t\tstrvec_pushf(&cmd.args,\n> \t\t\t\t\t \"--unpack-unreachable=%s\",\n> \n\nAll what you wrote above makes sense to me, but I've never looked at\nhow partial clones work, so it doesn't mean anything... In any case\nyour patch brings great speedups, but, unfortunately, the memory usage\nremains as high as it was:\n\n $ /usr/bin/time --format=elapsed: %E max RSS: %Mk /home/szeder/src/git/bin-wrappers/git -C git-partial.git/ gc\n Enumerating objects: 188450, done.\n Counting objects: 100% (188450/188450), done.\n Delta compression using up to 4 threads\n Compressing objects: 100% (66623/66623), done.\n Writing objects: 100% (188450/188450), done.\n Total 188450 (delta 120109), reused 188450 (delta 120109), pack-reused 0\n elapsed: 0:15.18 max RSS: 1888332k\n\nAnd git.git is not all that large, I wonder how much memory would be\nnecessary to 'gc' a 'blob:none' clone of e.g. chromium?! :)\n\nBTW, this high memory usage in a partial clone is not specific to\n'repack', 'fsck' suffers just as much:\n\n $ /usr/bin/time --format=elapsed: %E max RSS: %Mk git -C git-full.git/ fsck\n Checking object directories: 100% (256/256), done.\n warning in tag d6602ec5194c87b0fc87103ca4d67251c76f233a: missingTaggerEntry: invalid format - expected 'tagger' line\n Checking objects: 100% (305662/305662), done.\n Checking connectivity: 305662, done.\n Verifying commits in commit graph: 100% (65031/65031), done.\n elapsed: 0:33.99 max RSS: 281936k\n $ /usr/bin/time --format=elapsed: %E max RSS: %Mk git -C git-partial.git/ fsck\n Checking object directories: 100% (256/256), done.\n warning in tag d6602ec5194c87b0fc87103ca4d67251c76f233a: missingTaggerEntry: invalid format - expected 'tagger' line\n Checking objects: 100% (188450/188450), done.\n Verifying commits in commit graph: 100% (65031/65031), done.\n elapsed: 0:29.96 max RSS: 1877340k\n\n\nAnd a somewhat related issue: when the server doesn't support filters,\nthen 'git clone --filter=...' prints a warning and proceeds to clone\nthe full repo. Reading ba95710a3b ({fetch,upload}-pack: support\nfilter in protocol v2, 2018-05-03) this seems to be intentional and I\ntend to think that it makes sense (though I managed to overlook that\nwarning twice today... I surely wouldn't have overlooked a hard\nerror, but that would perhaps be too harsh in this case, dunno).\nHowever, the resulting full clone is still marked as partial:\n\n $ git clone --bare --filter=blob:none https://git.kernel.org/pub/scm/git/git.git git-not-really-partial.git\n Cloning into bare repository 'git-not-really-partial.git'...\n warning: filtering not recognized by server, ignoring\n remote: Enumerating objects: 591, done.\n remote: Counting objects: 100% (591/591), done.\n remote: Compressing objects: 100% (293/293), done.\n remote: Total 305662 (delta 372), reused 393 (delta 298), pack-reused 305071\n Receiving objects: 100% (305662/305662), 96.83 MiB | 2.10 MiB/s, done.\n Resolving deltas: 100% (228123/228123), done.\n $ ls -l git-not-really-partial.git/objects/pack/\n total 107568\n -r--r--r-- 1 szeder szeder 8559608 Apr 12 21:13 pack-53f3ee0dfeaa8cea65c78473cd5904bf5ddfaa20.idx\n -r--r--r-- 1 szeder szeder 101535430 Apr 12 21:13 pack-53f3ee0dfeaa8cea65c78473cd5904bf5ddfaa20.pack\n -rw------- 1 szeder szeder 49012 Apr 12 21:13 pack-53f3ee0dfeaa8cea65c78473cd5904bf5ddfaa20.promisor\n $ cat git-not-really-partial.git/config \n [core]\n \trepositoryformatversion = 1\n \tfilemode = true\n \tbare = true\n [remote \"origin\"]\n \turl = https://git.kernel.org/pub/scm/git/git.git\n \tpromisor = true\n \tpartialclonefilter = blob:none\n\nI wonder whether this is intentional, or that it is really the desired\nbehavior, considering that 'gc/repack/fsck' still treat it as a\npartial clone, and, consequently, are affected by this slowness and\nmuch higher memory usage, and since the repo now contains a lot more\nobjects than expected (all the blobs as well), they are much slower:\n\n $ /usr/bin/time --format=elapsed: %E max RSS: %Mk git -C git-not-really-partial.git/ gc\n Enumerating objects: 305662, done.\n Counting objects: 100% (305662/305662), done.\n Delta compression using up to 4 threads\n Compressing objects: 100% (75200/75200), done.\n Writing objects: 100% (305662/305662), done.\n Total 305662 (delta 228123), reused 305662 (delta 228123), pack-reused 0\n Removing duplicate objects: 100% (256/256), done.\n elapsed: 4:28.96 max RSS: 1985100k\n # with Peff's patch above:\n $ /usr/bin/time --format=elapsed: %E max RSS: %Mk /home/szeder/src/git/bin-wrappers/git -C git-not-really-partial.git/ gc\n Enumerating objects: 305662, done.\n Counting objects: 100% (305662/305662), done.\n Delta compression using up to 4 threads\n Compressing objects: 100% (75200/75200), done.\n Writing objects: 100% (305662/305662), done.\n Total 305662 (delta 228123), reused 305662 (delta 228123), pack-reused 0\n elapsed: 1:21.83 max RSS: 1959740k\n\n"},{"id":"421818","messageId":"CAGyf7-HTCDm_SB5CfQWJWjvuCVYuJ4=h65=zG-N1XTgNRs+j0w@mail.gmail.com","threadId":"55430","inReplyTo":"20210412213653.GH2947267@szeder.dev","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2021-04-12T21:49:00Z","receivedAt":"2021-04-12T21:49:13Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Mon, Apr 12, 2021 at 2:37 PM SZEDER Gábor wrote:\n>\n> And a somewhat related issue: when the server doesn't support filters,\n> then 'git clone --filter=...' prints a warning and proceeds to clone\n> the full repo. Reading ba95710a3b ({fetch,upload}-pack: support\n> filter in protocol v2, 2018-05-03) this seems to be intentional and I\n> tend to think that it makes sense (though I managed to overlook that\n> warning twice today... I surely wouldn't have overlooked a hard\n> error, but that would perhaps be too harsh in this case, dunno).\n> However, the resulting full clone is still marked as partial:\n>\n> $ git clone --bare --filter=blob:none https://git.kernel.org/pub/scm/git/git.git git-not-really-partial.git\n> Cloning into bare repository 'git-not-really-partial.git'...\n> warning: filtering not recognized by server, ignoring\n> remote: Enumerating objects: 591, done.\n> remote: Counting objects: 100% (591/591), done.\n> remote: Compressing objects: 100% (293/293), done.\n> remote: Total 305662 (delta 372), reused 393 (delta 298), pack-reused 305071\n> Receiving objects: 100% (305662/305662), 96.83 MiB | 2.10 MiB/s, done.\n> Resolving deltas: 100% (228123/228123), done.\n> $ ls -l git-not-really-partial.git/objects/pack/\n> total 107568\n> -r--r--r-- 1 szeder szeder 8559608 Apr 12 21:13 pack-53f3ee0dfeaa8cea65c78473cd5904bf5ddfaa20.idx\n> -r--r--r-- 1 szeder szeder 101535430 Apr 12 21:13 pack-53f3ee0dfeaa8cea65c78473cd5904bf5ddfaa20.pack\n> -rw------- 1 szeder szeder 49012 Apr 12 21:13 pack-53f3ee0dfeaa8cea65c78473cd5904bf5ddfaa20.promisor\n> $ cat git-not-really-partial.git/config\n> [core]\n> repositoryformatversion = 1\n> filemode = true\n> bare = true\n> [remote \"origin\"]\n> url = https://git.kernel.org/pub/scm/git/git.git\n> promisor = true\n> partialclonefilter = blob:none\n\nI ran into this same surprising behavior recently, too. I was adding\nsome automated testing to Bitbucket for partial clones and initially\ntried to use whether the repository was configured with a partial\nclone filter as one of my checks, only to find that even when filters\nweren't supported it was still set. The only way I could find to\ndetect that a partial clone that was requested didn't actually happen\nwas to parse the git clone output and look for the warning.\n\n>\n> I wonder whether this is intentional, or that it is really the desired\n> behavior, considering that 'gc/repack/fsck' still treat it as a\n> partial clone, and, consequently, are affected by this slowness and\n> much higher memory usage, and since the repo now contains a lot more\n> objects than expected (all the blobs as well), they are much slower:\n>\n> $ /usr/bin/time --format=elapsed: %E max RSS: %Mk git -C git-not-really-partial.git/ gc\n> Enumerating objects: 305662, done.\n> Counting objects: 100% (305662/305662), done.\n> Delta compression using up to 4 threads\n> Compressing objects: 100% (75200/75200), done.\n> Writing objects: 100% (305662/305662), done.\n> Total 305662 (delta 228123), reused 305662 (delta 228123), pack-reused 0\n> Removing duplicate objects: 100% (256/256), done.\n> elapsed: 4:28.96 max RSS: 1985100k\n> # with Peff's patch above:\n> $ /usr/bin/time --format=elapsed: %E max RSS: %Mk /home/szeder/src/git/bin-wrappers/git -C git-not-really-partial.git/ gc\n> Enumerating objects: 305662, done.\n> Counting objects: 100% (305662/305662), done.\n> Delta compression using up to 4 threads\n> Compressing objects: 100% (75200/75200), done.\n> Writing objects: 100% (305662/305662), done.\n> Total 305662 (delta 228123), reused 305662 (delta 228123), pack-reused 0\n> elapsed: 1:21.83 max RSS: 1959740k\n>\n"},{"id":"421824","messageId":"YHTcHY+P7RuZJGab@coredump.intra.peff.net","threadId":"55430","inReplyTo":"20210412213653.GH2947267@szeder.dev","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-12T23:47:41Z","receivedAt":"2021-04-12T23:47:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 12, 2021 at 11:36:53PM +0200, SZEDER Gábor wrote:\n\n> All what you wrote above makes sense to me, but I've never looked at\n> how partial clones work, so it doesn't mean anything... In any case\n> your patch brings great speedups, but, unfortunately, the memory usage\n> remains as high as it was:\n> \n> $ /usr/bin/time --format=elapsed: %E max RSS: %Mk /home/szeder/src/git/bin-wrappers/git -C git-partial.git/ gc\n> Enumerating objects: 188450, done.\n> Counting objects: 100% (188450/188450), done.\n> Delta compression using up to 4 threads\n> Compressing objects: 100% (66623/66623), done.\n> Writing objects: 100% (188450/188450), done.\n> Total 188450 (delta 120109), reused 188450 (delta 120109), pack-reused 0\n> elapsed: 0:15.18 max RSS: 1888332k\n> \n> And git.git is not all that large, I wonder how much memory would be\n> necessary to 'gc' a 'blob:none' clone of e.g. chromium?! :)\n> \n> BTW, this high memory usage in a partial clone is not specific to\n> 'repack', 'fsck' suffers just as much:\n\nI think the issue is in the exclude-promisor-object code paths of the\ntraversal. Try this:\n\n [two clones of git.git, one full and one partial]\n $ git clone --no-local --bare /path/to/git full.git\n $ git clone --no-local --bare --filter=blob:none /path/to/git partial.git\n\n [full clone is quick to traverse all objects]\n $ time git -C full.git rev-list --count --all\n 63215\n real\t0m0.369s\n user\t0m0.365s\n sys\t0m0.004s\n\n [partial is, too; it's the same amount of work because we're just\n looking at the commits here]\n $ time git -C partial.git rev-list --count --all\n 63215\n real\t0m0.373s\n user\t0m0.364s\n sys\t0m0.009s\n\n [but now ask it to exclude promisor objects, and it's much slower;\n this is because is_promisor_object() opens up each tree in the pack in\n order to see which \"promised\" objects it mentions]\n $ time git -C partial.git rev-list --exclude-promisor-objects --count --all\n 0\n real\t0m11.723s\n user\t0m11.354s\n sys\t0m0.369s\n\nAnd I think that is the source for the memory use, too. Without that\noption, we peak at 11MB heap. With it, 1.6GB. Oops. Looks like there\nmight be some \"leaks\" when we parse tree objects and then leave the\nbuffers connected to the structs (technically not a leak because we\nstill have the pointers, but obviously having every tree in memory at\nonce is not good).\n\nThe patch below drops the peak heap to 165MB. Still quite a bit more,\nbut I think it's a combination of delta-base cache (96MB) plus extra\nstructs for all the non-commit objects whose flags we marked.\n\nIt does seem like there's probably a good space/time tradeoff to be made\nhere (e.g., caching the list of \"promisor\" object ids for the pack\ninstead of inflating and reading all of the trees on the fly).\n\ndiff --git a/packfile.c b/packfile.c\nindex 8668345d93..b79cbc8cd4 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid,\n \t\t\treturn 0;\n \t\twhile (tree_entry_gently(&desc, &entry))\n \t\t\toidset_insert(set, &entry.oid);\n+\t\tfree_tree_buffer(tree);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\ndiff --git a/revision.c b/revision.c\nindex 553c0faa9b..fac2577748 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3271,8 +3271,15 @@ static int mark_uninteresting(const struct object_id *oid,\n \t\t\t void *cb)\n {\n \tstruct rev_info *revs = cb;\n+\t/*\n+\t * yikes, do we really need to parse here? maybe\n+\t * lookup_unknown_object() would be sufficient, or\n+\t * even oid_object_info() followed by the correct type\n+\t */\n \tstruct object *o = parse_object(revs->repo, oid);\n \to->flags |= UNINTERESTING | SEEN;\n+\tif (o->type == OBJ_TREE)\n+\t\tfree_tree_buffer((struct tree *)o);\n \treturn 0;\n }\n \n\n-Peff\n"},{"id":"421825","messageId":"YHTc8mqqDePlrOB8@coredump.intra.peff.net","threadId":"55430","inReplyTo":"CAGyf7-HTCDm_SB5CfQWJWjvuCVYuJ4=h65=zG-N1XTgNRs+j0w@mail.gmail.com","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-12T23:51:14Z","receivedAt":"2021-04-12T23:51:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 12, 2021 at 02:49:00PM -0700, Bryan Turner wrote:\n\n> I ran into this same surprising behavior recently, too. I was adding\n> some automated testing to Bitbucket for partial clones and initially\n> tried to use whether the repository was configured with a partial\n> clone filter as one of my checks, only to find that even when filters\n> weren't supported it was still set. The only way I could find to\n> detect that a partial clone that was requested didn't actually happen\n> was to parse the git clone output and look for the warning.\n\nI think the state of \"we have all the objects, but things are marked as\npartial\" is some place you could actually get into naturally: you start\nwith a partial clone, and then later fetch the objects you need, and you\njust happen to have all the objects). Or you could even start there if\nthe filter happens not to exclude any objects. So I don't think that\nstate is invalid in any way.\n\nBut I do agree that if the client _knows_ that the filter was not used\n(because the other side did not advertise filters and so we did not even\nsend it), then it is silly to create the client-side config marking us\nas partial. It is misleading at best, and makes things slower at worst.\n\n-Peff\n"},{"id":"421834","messageId":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","threadId":"55430","inReplyTo":"YHTcHY+P7RuZJGab@coredump.intra.peff.net","subject":"[PATCH 0/3] low-hanging performance fruit with promisor packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-13T07:12:54Z","receivedAt":"2021-04-13T07:12:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 12, 2021 at 07:47:41PM -0400, Jeff King wrote:\n\n> The patch below drops the peak heap to 165MB. Still quite a bit more,\n> but I think it's a combination of delta-base cache (96MB) plus extra\n> structs for all the non-commit objects whose flags we marked.\n\nI think we can do even better than that, after looking into the \"do we\nreally need to parse the objects?\" comment I left (spoiler: the answer\nis no, we do not need to, at least for that caller).\n\nHere are some cleaned-up patches that I think improve the situation\nquite a bit. This is just the low-hanging fruit from this part of the\ndiscussion; I'm sure there's more to do to make using partial clones\npleasant. In particular:\n\n - this does nothing for the \"oops, we turned all of the promisor\n objects loose and then deleted them\" problem. Hopefully Rafael\n will produce a nice patch for that\n\n - In is_promisor_object(), we still call parse_object(), because it\n really does look at the contents. But doing so for blobs is wasteful\n (it's a lot of bytes we push through sha1, and we don't even look at\n them). It might be worth using oid_object_info() to avoid this. This\n introduces a little overhead, but I think would be a net win (it\n would be really nice if we could amortize the object lookup work;\n i.e., if there was a way to call oid_object_info_extended() and say\n \"open the object and look at the type; only return the contents if\n it's a non-blob\").\n\n - I still think it's probably worth having a mode where we store the\n set of pointed-to objects we don't have, rather than parsing on the\n fly. This could easily be made optional (it's a good thing if you\n _have_ a lot of objects that point to a small or moderate number of\n missing objects, like a blob:limit or blob:none filter; it's a bad\n thing if you have very few objects but they point to a very large\n number).\n\nI didn't explore any of those here, and I don't plan to look into them\nanytime soon. I'm just documenting my findings for later.\n\nAnyway, here are the patches.\n\n [1/3]: is_promisor_object(): free tree buffer after parsing\n [2/3]: lookup_unknown_object(): take a repository argument\n [3/3]: revision: avoid parsing with --exclude-promisor-objects\n\n builtin/fsck.c | 2 +-\n builtin/pack-objects.c | 2 +-\n http-push.c | 2 +-\n object.c | 7 +++----\n object.h | 2 +-\n packfile.c | 1 +\n refs.c | 2 +-\n revision.c | 2 +-\n t/helper/test-example-decorate.c | 6 +++---\n t/perf/p5600-partial-clone.sh | 12 ++++++++++++\n upload-pack.c | 2 +-\n walker.c | 2 +-\n 12 files changed, 27 insertions(+), 15 deletions(-)\n\n-Peff\n"},{"id":"421835","messageId":"YHVFKgn7WN76QnRz@coredump.intra.peff.net","threadId":"55430","inReplyTo":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","subject":"[PATCH 1/3] is_promisor_object(): free tree buffer after parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-13T07:15:54Z","receivedAt":"2021-04-13T07:16:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To get the list of all promisor objects, we not only include all objects\nin promisor packs, but also parse each of those objects to see which\nobjects they reference. After parsing a tree object, the tree->buffer\nfield will remain populated until we explicitly free it. So in a partial\nclone of blob:none, for example, we are essentially reading every tree\nin the repository (since they're all in the initial promisor pack), and\nkeeping all of their uncompressed contents in memory at once.\n\nThis patch frees the tree buffers after we've finished marking all of\ntheir reachable objects. We shouldn't need to do this for any other\nobject type. While we are using some extra memory to store the structs,\nno other object type stores the whole contents in its parsed form (we do\nsometimes hold on to commit buffers, but less so these days due to\ncommit graphs, plus most commands which care about promisor objects turn\noff the save_commit_buffer global).\n\nEven for a moderate-sized repository like git.git, this patch drops the\npeak heap (as measured by massif) for git-fsck from ~1.7GB to ~138MB.\nFsck is a good candidate for measuring here because it doesn't interact\nwith the promisor code except to call is_promisor_object(), so we can\nisolate just this problem.\n\nThe added perf test shows only a tiny improvement on my machine for\ngit.git, since 1.7GB isn't enough to cause any real memory pressure:\n\n Test HEAD^ HEAD\n --------------------------------------------------------------------------------\n 5600.4: fsck 21.26(20.90+0.35) 20.84(20.79+0.04) -2.0%\n\nWith linux.git the absolute change is a bit bigger, though still a small\npercentage:\n\n Test HEAD^ HEAD\n -----------------------------------------------------------------------------\n 5600.4: fsck 262.26(259.13+3.12) 254.92(254.62+0.29) -2.8%\n\nI didn't have the patience to run it under massif with linux.git, but\nit's probably on the order of about 14GB improvement, since that's the\nsum of the sizes of all of the uncompressed trees (but still isn't\nenough to create memory pressure on this particular machine, which has\n64GB of RAM). Smaller machines would probably see a bigger effect on\nruntime (and sadly our perf suite does not measure peak heap).\n\nSigned-off-by: Jeff King \n---\n packfile.c | 1 +\n t/perf/p5600-partial-clone.sh | 4 ++++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 8668345d93..b79cbc8cd4 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid,\n \t\t\treturn 0;\n \t\twhile (tree_entry_gently(&desc, &entry))\n \t\t\toidset_insert(set, &entry.oid);\n+\t\tfree_tree_buffer(tree);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\ndiff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh\nindex 3e04bd2ae1..754aaec3dc 100755\n--- a/t/perf/p5600-partial-clone.sh\n+++ b/t/perf/p5600-partial-clone.sh\n@@ -23,4 +23,8 @@ test_perf 'checkout of result' '\n \tgit -C worktree checkout -f\n '\n \n+test_perf 'fsck' '\n+\tgit -C bare.git fsck\n+'\n+\n test_done\n-- \n2.31.1.659.g9b9913af63\n\n"},{"id":"421836","messageId":"YHVFVJXUbtZTgXeR@coredump.intra.peff.net","threadId":"55430","inReplyTo":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","subject":"[PATCH 2/3] lookup_unknown_object(): take a repository argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-13T07:16:36Z","receivedAt":"2021-04-13T07:16:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the other lookup_foo() functions take a repository argument, but\nlookup_unknown_object() was never converted, and it uses the_repository\ninternally. Let's fix that.\n\nWe could leave a wrapper that uses the_repository, but there aren't that\nmany calls, so we'll just convert them all. I looked briefly at each\nsite to see if we had a repository struct (besides the_repository) we\ncould pass, but none of them do (so this conversion to pass\nthe_repository is a pure noop in each case, though it does take us one\nstep closer to eventually getting rid of the_repository).\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 2 +-\n builtin/pack-objects.c | 2 +-\n http-push.c | 2 +-\n object.c | 7 +++----\n object.h | 2 +-\n refs.c | 2 +-\n t/helper/test-example-decorate.c | 6 +++---\n upload-pack.c | 2 +-\n walker.c | 2 +-\n 9 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 70ff95837a..e6a80e5404 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -725,7 +725,7 @@ static int fsck_cache_tree(struct cache_tree *it)\n \n static void mark_object_for_connectivity(const struct object_id *oid)\n {\n-\tstruct object *obj = lookup_unknown_object(oid);\n+\tstruct object *obj = lookup_unknown_object(the_repository, oid);\n \tobj->flags |= HAS_OBJ;\n }\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 525c2d8552..c1186f50a3 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3386,7 +3386,7 @@ static void add_objects_in_unpacked_packs(void)\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n \t\t\tnth_packed_object_id(&oid, p, i);\n-\t\t\to = lookup_unknown_object(&oid);\n+\t\t\to = lookup_unknown_object(the_repository, &oid);\n \t\t\tif (!(o->flags & OBJECT_ADDED))\n \t\t\t\tmark_in_pack_object(o, p, &in_pack);\n \t\t\to->flags |= OBJECT_ADDED;\ndiff --git a/http-push.c b/http-push.c\nindex b60d5fcc85..813123242e 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1436,7 +1436,7 @@ static void one_remote_ref(const char *refname)\n \t * may be required for updating server info later.\n \t */\n \tif (repo->can_update_info_refs && !has_object_file(&ref->old_oid)) {\n-\t\tobj = lookup_unknown_object(&ref->old_oid);\n+\t\tobj = lookup_unknown_object(the_repository, &ref->old_oid);\n \t\tfprintf(stderr,\t\" fetch %s for %s\\n\",\n \t\t\toid_to_hex(&ref->old_oid), refname);\n \t\tadd_fetch_request(obj);\ndiff --git a/object.c b/object.c\nindex 78343781ae..14188453c5 100644\n--- a/object.c\n+++ b/object.c\n@@ -177,12 +177,11 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n \t}\n }\n \n-struct object *lookup_unknown_object(const struct object_id *oid)\n+struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid)\n {\n-\tstruct object *obj = lookup_object(the_repository, oid);\n+\tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n-\t\tobj = create_object(the_repository, oid,\n-\t\t\t\t alloc_object_node(the_repository));\n+\t\tobj = create_object(r, oid, alloc_object_node(r));\n \treturn obj;\n }\n \ndiff --git a/object.h b/object.h\nindex 59daadce21..87a6da47c8 100644\n--- a/object.h\n+++ b/object.h\n@@ -145,7 +145,7 @@ struct object *parse_object_or_die(const struct object_id *oid, const char *name\n struct object *parse_object_buffer(struct repository *r, const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p);\n \n /** Returns the object, with potentially excess memory allocated. **/\n-struct object *lookup_unknown_object(const struct object_id *oid);\n+struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid);\n \n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t struct object_list **list_p);\ndiff --git a/refs.c b/refs.c\nindex 261fd82beb..1616c7554a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -337,7 +337,7 @@ static int filter_refs(const char *refname, const struct object_id *oid,\n \n enum peel_status peel_object(const struct object_id *name, struct object_id *oid)\n {\n-\tstruct object *o = lookup_unknown_object(name);\n+\tstruct object *o = lookup_unknown_object(the_repository, name);\n \n \tif (o->type == OBJ_NONE) {\n \t\tint type = oid_object_info(the_repository, name, NULL);\ndiff --git a/t/helper/test-example-decorate.c b/t/helper/test-example-decorate.c\nindex c8a1cde7d2..b9d1200eb9 100644\n--- a/t/helper/test-example-decorate.c\n+++ b/t/helper/test-example-decorate.c\n@@ -26,8 +26,8 @@ int cmd__example_decorate(int argc, const char **argv)\n \t * Add 2 objects, one with a non-NULL decoration and one with a NULL\n \t * decoration.\n \t */\n-\tone = lookup_unknown_object(&one_oid);\n-\ttwo = lookup_unknown_object(&two_oid);\n+\tone = lookup_unknown_object(the_repository, &one_oid);\n+\ttwo = lookup_unknown_object(the_repository, &two_oid);\n \tret = add_decoration(&n, one, &decoration_a);\n \tif (ret)\n \t\tBUG(\"when adding a brand-new object, NULL should be returned\");\n@@ -56,7 +56,7 @@ int cmd__example_decorate(int argc, const char **argv)\n \tret = lookup_decoration(&n, two);\n \tif (ret != &decoration_b)\n \t\tBUG(\"lookup should return added declaration\");\n-\tthree = lookup_unknown_object(&three_oid);\n+\tthree = lookup_unknown_object(the_repository, &three_oid);\n \tret = lookup_decoration(&n, three);\n \tif (ret)\n \t\tBUG(\"lookup for unknown object should return NULL\");\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e19583ae0f..5c1cd19612 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1153,7 +1153,7 @@ static void receive_needs(struct upload_pack_data *data,\n static int mark_our_ref(const char *refname, const char *refname_full,\n \t\t\tconst struct object_id *oid)\n {\n-\tstruct object *o = lookup_unknown_object(oid);\n+\tstruct object *o = lookup_unknown_object(the_repository, oid);\n \n \tif (ref_is_hidden(refname, refname_full)) {\n \t\to->flags |= HIDDEN_REF;\ndiff --git a/walker.c b/walker.c\nindex 4984bf8b3d..c5e2921979 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -298,7 +298,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\t\terror(\"Could not interpret response from server '%s' as something to pull\", target[i]);\n \t\t\tgoto done;\n \t\t}\n-\t\tif (process(walker, lookup_unknown_object(&oids[i])))\n+\t\tif (process(walker, lookup_unknown_object(the_repository, &oids[i])))\n \t\t\tgoto done;\n \t}\n \n-- \n2.31.1.659.g9b9913af63\n\n"},{"id":"421837","messageId":"YHVFnNvGim8Iduwq@coredump.intra.peff.net","threadId":"55430","inReplyTo":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","subject":"[PATCH 3/3] revision: avoid parsing with --exclude-promisor-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-13T07:17:48Z","receivedAt":"2021-04-13T07:17:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When --exclude-promisor-objects is given, before traversing any objects\nwe iterate over all of the objects in any promisor packs, marking them\nas UNINTERESTING and SEEN. We turn the oid we get from iterating the\npack into an object with parse_object(), but this has two problems:\n\n - it's slow; we are zlib inflating (and reconstructing from deltas)\n every byte of every object in the packfile\n\n - it leaves the tree buffers attached to their structs, which means\n our heap usage will grow to store every uncompressed tree\n simultaneously. This can be gigabytes.\n\nWe can obviously fix the second by freeing the tree buffers after we've\nparsed them. But we can observe that the function doesn't look at the\nobject contents at all! The only reason we call parse_object() is that\nwe need a \"struct object\" on which to set the flags. There are two\noptions here:\n\n - we can look up just the object type via oid_object_info(), and then\n call the appropriate lookup_foo() function\n\n - we can call lookup_unknown_object(), which gives us an OBJ_NONE\n struct (which will get auto-converted later by object_as_type() via\n calls to lookup_commit(), etc).\n\nThe first one is closer to the current code, but we do pay the price to\nlook up the type for each object. The latter should be more efficient in\nCPU, though it wastes a little bit of memory (the \"unknown\" object\nstructs are a union of all object types, so some of the structs are\nbigger than they need to be). It also runs the risk of triggering a\nlatent bug in code that calls lookup_object() directly but isn't ready\nto handle OBJ_NONE (such code would already be buggy, but we use\nlookup_unknown_object() infrequently enough that it might be hiding).\n\nI went with the second option here. I don't think the risk is high (and\nwe'd want to find and fix any such bugs anyway), and it should be more\nefficient overall.\n\nThe new tests in p5600 show off the improvement (this is on git.git):\n\n Test HEAD^ HEAD\n -------------------------------------------------------------------------------\n 5600.5: count commits 0.37(0.37+0.00) 0.38(0.38+0.00) +2.7%\n 5600.6: count non-promisor commits 11.74(11.37+0.37) 0.04(0.03+0.00) -99.7%\n\nThe improvement is particularly big in this script because _every_\nobject in the newly-cloned partial repo is a promisor object. So after\nmarking them all, there's nothing left to traverse.\n\nSigned-off-by: Jeff King \n---\n revision.c | 2 +-\n t/perf/p5600-partial-clone.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 553c0faa9b..7e73dafd96 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3271,7 +3271,7 @@ static int mark_uninteresting(const struct object_id *oid,\n \t\t\t void *cb)\n {\n \tstruct rev_info *revs = cb;\n-\tstruct object *o = parse_object(revs->repo, oid);\n+\tstruct object *o = lookup_unknown_object(revs->repo, oid);\n \to->flags |= UNINTERESTING | SEEN;\n \treturn 0;\n }\ndiff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh\nindex 754aaec3dc..ca785a3341 100755\n--- a/t/perf/p5600-partial-clone.sh\n+++ b/t/perf/p5600-partial-clone.sh\n@@ -27,4 +27,12 @@ test_perf 'fsck' '\n \tgit -C bare.git fsck\n '\n \n+test_perf 'count commits' '\n+\tgit -C bare.git rev-list --all --count\n+'\n+\n+test_perf 'count non-promisor commits' '\n+\tgit -C bare.git rev-list --all --count --exclude-promisor-objects\n+'\n+\n test_done\n-- \n2.31.1.659.g9b9913af63\n"},{"id":"421912","messageId":"20210413180552.GI2947267@szeder.dev","threadId":"55430","inReplyTo":"YHTcHY+P7RuZJGab@coredump.intra.peff.net","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-13T18:05:52Z","receivedAt":"2021-04-13T18:05:58Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 12, 2021 at 07:47:41PM -0400, Jeff King wrote:\n> On Mon, Apr 12, 2021 at 11:36:53PM +0200, SZEDER Gábor wrote:\n> \n> > All what you wrote above makes sense to me, but I've never looked at\n> > how partial clones work, so it doesn't mean anything... In any case\n> > your patch brings great speedups, but, unfortunately, the memory usage\n> > remains as high as it was:\n> > \n> > $ /usr/bin/time --format=elapsed: %E max RSS: %Mk /home/szeder/src/git/bin-wrappers/git -C git-partial.git/ gc\n> > Enumerating objects: 188450, done.\n> > Counting objects: 100% (188450/188450), done.\n> > Delta compression using up to 4 threads\n> > Compressing objects: 100% (66623/66623), done.\n> > Writing objects: 100% (188450/188450), done.\n> > Total 188450 (delta 120109), reused 188450 (delta 120109), pack-reused 0\n> > elapsed: 0:15.18 max RSS: 1888332k\n> > \n> > And git.git is not all that large, I wonder how much memory would be\n> > necessary to 'gc' a 'blob:none' clone of e.g. chromium?! :)\n> > \n> > BTW, this high memory usage in a partial clone is not specific to\n> > 'repack', 'fsck' suffers just as much:\n> \n> I think the issue is in the exclude-promisor-object code paths of the\n> traversal. Try this:\n> \n> [two clones of git.git, one full and one partial]\n> $ git clone --no-local --bare /path/to/git full.git\n> $ git clone --no-local --bare --filter=blob:none /path/to/git partial.git\n> \n> [full clone is quick to traverse all objects]\n> $ time git -C full.git rev-list --count --all\n> 63215\n> real\t0m0.369s\n> user\t0m0.365s\n> sys\t0m0.004s\n> \n> [partial is, too; it's the same amount of work because we're just\n> looking at the commits here]\n> $ time git -C partial.git rev-list --count --all\n> 63215\n> real\t0m0.373s\n> user\t0m0.364s\n> sys\t0m0.009s\n> \n> [but now ask it to exclude promisor objects, and it's much slower;\n> this is because is_promisor_object() opens up each tree in the pack in\n> order to see which \"promised\" objects it mentions]\n\nI don't understand this: 'git rev-list --count --all' only counts\ncommit objects, so why should it open any trees at all?\n\n> $ time git -C partial.git rev-list --exclude-promisor-objects --count --all\n> 0\n> real\t0m11.723s\n> user\t0m11.354s\n> sys\t0m0.369s\n> \n> And I think that is the source for the memory use, too. Without that\n> option, we peak at 11MB heap. With it, 1.6GB. Oops. Looks like there\n> might be some \"leaks\" when we parse tree objects and then leave the\n> buffers connected to the structs (technically not a leak because we\n> still have the pointers, but obviously having every tree in memory at\n> once is not good).\n> \n> The patch below drops the peak heap to 165MB. Still quite a bit more,\n> but I think it's a combination of delta-base cache (96MB) plus extra\n> structs for all the non-commit objects whose flags we marked.\n> \n> It does seem like there's probably a good space/time tradeoff to be made\n> here (e.g., caching the list of \"promisor\" object ids for the pack\n> instead of inflating and reading all of the trees on the fly).\n> \n> diff --git a/packfile.c b/packfile.c\n> index 8668345d93..b79cbc8cd4 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid,\n> \t\t\treturn 0;\n> \t\twhile (tree_entry_gently(&desc, &entry))\n> \t\t\toidset_insert(set, &entry.oid);\n> +\t\tfree_tree_buffer(tree);\n> \t} else if (obj->type == OBJ_COMMIT) {\n> \t\tstruct commit *commit = (struct commit *) obj;\n> \t\tstruct commit_list *parents = commit->parents;\n> diff --git a/revision.c b/revision.c\n> index 553c0faa9b..fac2577748 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -3271,8 +3271,15 @@ static int mark_uninteresting(const struct object_id *oid,\n> \t\t\t void *cb)\n> {\n> \tstruct rev_info *revs = cb;\n> +\t/*\n> +\t * yikes, do we really need to parse here? maybe\n\nHeh, a \"yikes\" here and a \"yuck\" in your previous patch... This issue\nwas worth reporting :)\n\n> +\t * lookup_unknown_object() would be sufficient, or\n> +\t * even oid_object_info() followed by the correct type\n> +\t */\n> \tstruct object *o = parse_object(revs->repo, oid);\n> \to->flags |= UNINTERESTING | SEEN;\n> +\tif (o->type == OBJ_TREE)\n> +\t\tfree_tree_buffer((struct tree *)o);\n> \treturn 0;\n> }\n> \n> \n> -Peff\n"},{"id":"421913","messageId":"20210413181004.GJ2947267@szeder.dev","threadId":"55430","inReplyTo":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] low-hanging performance fruit with promisor packs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-13T18:10:04Z","receivedAt":"2021-04-13T18:10:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Apr 13, 2021 at 03:12:54AM -0400, Jeff King wrote:\n> On Mon, Apr 12, 2021 at 07:47:41PM -0400, Jeff King wrote:\n> \n> > The patch below drops the peak heap to 165MB. Still quite a bit more,\n> > but I think it's a combination of delta-base cache (96MB) plus extra\n> > structs for all the non-commit objects whose flags we marked.\n> \n> I think we can do even better than that, after looking into the \"do we\n> really need to parse the objects?\" comment I left (spoiler: the answer\n> is no, we do not need to, at least for that caller).\n> \n> Here are some cleaned-up patches that I think improve the situation\n> quite a bit.\n\n> Anyway, here are the patches.\n> \n> [1/3]: is_promisor_object(): free tree buffer after parsing\n> [2/3]: lookup_unknown_object(): take a repository argument\n> [3/3]: revision: avoid parsing with --exclude-promisor-objects\n\nI tried these patches together with your first in this thread [1], and\nthen could finally 'gc' my 'blob:none' chromium clone in:\n\n elapsed: 3:23.64 max RSS: 3206552k\n\nThanks.\n\n\n[1] https://public-inbox.org/git/YG4hfge2y%2FAmcklZ@coredump.intra.peff.net/\n\n"},{"id":"421925","messageId":"xmqqtuoakkgc.fsf@gitster.g","threadId":"55430","inReplyTo":"YHVFKgn7WN76QnRz@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] is_promisor_object(): free tree buffer after parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-13T20:17:55Z","receivedAt":"2021-04-13T20:17:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The added perf test shows only a tiny improvement on my machine for\n> git.git, since 1.7GB isn't enough to cause any real memory pressure:\n>\n> Test HEAD^ HEAD\n> --------------------------------------------------------------------------------\n> 5600.4: fsck 21.26(20.90+0.35) 20.84(20.79+0.04) -2.0%\n>\n> With linux.git the absolute change is a bit bigger, though still a small\n> percentage:\n>\n> Test HEAD^ HEAD\n> -----------------------------------------------------------------------------\n> 5600.4: fsck 262.26(259.13+3.12) 254.92(254.62+0.29) -2.8%\n>\n> I didn't have the patience to run it under massif with linux.git, but\n> it's probably on the order of about 14GB improvement, since that's the\n> sum of the sizes of all of the uncompressed trees (but still isn't\n> enough to create memory pressure on this particular machine, which has\n> 64GB of RAM). Smaller machines would probably see a bigger effect on\n> runtime (and sadly our perf suite does not measure peak heap).\n>\n> Signed-off-by: Jeff King \n> ---\n> packfile.c | 1 +\n> t/perf/p5600-partial-clone.sh | 4 ++++\n> 2 files changed, 5 insertions(+)\n>\n> diff --git a/packfile.c b/packfile.c\n> index 8668345d93..b79cbc8cd4 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid,\n> \t\t\treturn 0;\n> \t\twhile (tree_entry_gently(&desc, &entry))\n> \t\t\toidset_insert(set, &entry.oid);\n> +\t\tfree_tree_buffer(tree);\n> \t} else if (obj->type == OBJ_COMMIT) {\n> \t\tstruct commit *commit = (struct commit *) obj;\n> \t\tstruct commit_list *parents = commit->parents;\n\nHmph, does an added free() without removing one later mean we've\nbeen leaking?\n\nNicely done. Thanks.\n\n\n> diff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh\n> index 3e04bd2ae1..754aaec3dc 100755\n> --- a/t/perf/p5600-partial-clone.sh\n> +++ b/t/perf/p5600-partial-clone.sh\n> @@ -23,4 +23,8 @@ test_perf 'checkout of result' '\n> \tgit -C worktree checkout -f\n> '\n> \n> +test_perf 'fsck' '\n> +\tgit -C bare.git fsck\n> +'\n> +\n> test_done\n"},{"id":"421926","messageId":"xmqqpmyykk8o.fsf@gitster.g","threadId":"55430","inReplyTo":"YHVFnNvGim8Iduwq@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] revision: avoid parsing with --exclude-promisor-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-13T20:22:31Z","receivedAt":"2021-04-13T20:22:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... The only reason we call parse_object() is that\n> we need a \"struct object\" on which to set the flags. There are two\n> options here:\n>\n> - we can look up just the object type via oid_object_info(), and then\n> call the appropriate lookup_foo() function\n>\n> - we can call lookup_unknown_object(), which gives us an OBJ_NONE\n> struct (which will get auto-converted later by object_as_type() via\n> calls to lookup_commit(), etc).\n>\n> The first one is closer to the current code, but we do pay the price to\n> look up the type for each object. The latter should be more efficient in\n> CPU, though it wastes a little bit of memory (the \"unknown\" object\n\nThat's clever. I like it.\n\n> 5600.5: count commits 0.37(0.37+0.00) 0.38(0.38+0.00) +2.7%\n> 5600.6: count non-promisor commits 11.74(11.37+0.37) 0.04(0.03+0.00) -99.7%\n>\n> The improvement is particularly big in this script because _every_\n> object in the newly-cloned partial repo is a promisor object. So after\n> marking them all, there's nothing left to traverse.\n\n;-).\n"},{"id":"421944","messageId":"YHZ6JvLBNpZVOqiX@coredump.intra.peff.net","threadId":"55430","inReplyTo":"20210413180552.GI2947267@szeder.dev","subject":"Re: rather slow 'git repack' in 'blob:none' partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-14T05:14:14Z","receivedAt":"2021-04-14T05:14:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 13, 2021 at 08:05:52PM +0200, SZEDER Gábor wrote:\n\n> > [but now ask it to exclude promisor objects, and it's much slower;\n> > this is because is_promisor_object() opens up each tree in the pack in\n> > order to see which \"promised\" objects it mentions]\n> \n> I don't understand this: 'git rev-list --count --all' only counts\n> commit objects, so why should it open any trees at all?\n\nBecause the promisor code is a bit over-eager. There's actually one\nsmall error in what I wrote above. In that particular rev-list, we're\nnot calling is_promisor_object() at all, because we'll already have\nexcluded all the promisor objects by marking them UNINTERESTING and\nSEEN.\n\nSo:\n\n - in is_promisor_object(), we load the _whole_ list of promisor\n objects, which requires opening trees to find out about referenced\n blobs. In theory it could know that we only care about commits, but\n it's not connected to a particular traversal, so it gets the whole\n list.\n\n - mark_uninteresting() is the code where we pre-mark the objects from\n the promisor pack as UNINTERESTING, and that was loading all of the\n trees in this case. And it could know that we are not looking at\n --objects, so there's no need to touch trees. But after my patches,\n we do not load the contents of _any_ objects at all in that\n function. We could avoid even creating \"struct object\" for\n non-commits there, too, but that would imply looking up the type of\n each object (so more CPU, though it would save us some memory when\n we only care about commits). I suspect in practice that most callers\n would generally pass --objects anyway, though (e.g., your original\n pack-objects that started this thread certainly cares about\n non-commits).\n\n> > +\t/*\n> > +\t * yikes, do we really need to parse here? maybe\n> \n> Heh, a \"yikes\" here and a \"yuck\" in your previous patch... This issue\n> was worth reporting :)\n\nYeah. I think the client side of a lot of this partial-clone / promisor\nstuff is not very mature. It's waiting on people to start using it and\nfinding all of these rough edges.\n\n-Peff\n"},{"id":"421945","messageId":"YHZ7Iy29FS9SjKjT@coredump.intra.peff.net","threadId":"55430","inReplyTo":"xmqqtuoakkgc.fsf@gitster.g","subject":"Re: [PATCH 1/3] is_promisor_object(): free tree buffer after parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-14T05:18:27Z","receivedAt":"2021-04-14T05:18:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 13, 2021 at 01:17:55PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/packfile.c b/packfile.c\n> > index 8668345d93..b79cbc8cd4 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid,\n> > \t\t\treturn 0;\n> > \t\twhile (tree_entry_gently(&desc, &entry))\n> > \t\t\toidset_insert(set, &entry.oid);\n> > +\t\tfree_tree_buffer(tree);\n> > \t} else if (obj->type == OBJ_COMMIT) {\n> > \t\tstruct commit *commit = (struct commit *) obj;\n> > \t\tstruct commit_list *parents = commit->parents;\n> \n> Hmph, does an added free() without removing one later mean we've\n> been leaking?\n\nYes. Though perhaps not technically a leak, in that we are still holding\non to the \"struct tree\" entries via the obj_hash table. But nobody was\nfreeing them at all until the end of the program.\n\nI actually think it may be a mistake for \"struct tree\" to have\nbuffer/len fields at all. It is a slight convenience to be able to pass\nthem around with the struct, but it makes the expected lifetime much\nmore confusing. In practice, all code wants to deal with one tree at a\ntime, then drop the buffer when it's done (we might hold several when\nrecursing through subtrees, but we'd never hold more than the distance\nfrom the leaf to the root, and each recursive invocation of something\nlike process_tree() is holding exactly one tree buffer).\n\nIt may not be worth the trouble to try to clean it up at this point,\nthough.\n\n-Peff\n"},{"id":"421975","messageId":"20210414171419.3980561-1-jonathantanmy@google.com","threadId":"55430","inReplyTo":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] low-hanging performance fruit with promisor packs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-14T17:14:19Z","receivedAt":"2021-04-14T17:14:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Here are some cleaned-up patches that I think improve the situation\n> quite a bit. This is just the low-hanging fruit from this part of the\n> discussion; I'm sure there's more to do to make using partial clones\n> pleasant. In particular:\n\n[snip]\n\nThanks, Peff. These patches look good to me.\n"},{"id":"421978","messageId":"20210414191403.4387-1-rafaeloliveira.cs@gmail.com","threadId":"55430","inReplyTo":"20210403090412.GH2271@szeder.dev","subject":"[PATCH 0/2] prevent `repack` to unpack and delete promisor objects","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-14T19:14:01Z","receivedAt":"2021-04-14T19:15:02Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"This series is built on top of jk/promisor-optim. It conflicts with\nchanges on p5600 otherwise.\n\nThe following patches fixes the issue where we unnecessarily turn loose\nall the promisor objects and deletes them right after when running\n`repack -A -d ..` (via `git gc) for a partial repository. \n\nSpecial thanks to Peff, for proposing a better approach for managing\nthe situation and for Jonathan Tan for earlier interaction on the\nsolution. Previously, I thought we should skip the promisor objects\nby just adding a check in loosened_object_can_be_discarded(). However,\nPeff pointed out that we can do better by realizing much sooner that\nwe should not even consider loosening the objects for the _old_ promisor\npacks.\n\nIt took me a bit to come up with the test because it seems `repack`\ndoesn't offer an option to skip the \"deletion of unpacked objects\",\nso this series adds a new option to `repack` for skip the\n`git prune-packed` execution thus allowing us to easily inspect the\nunpacked objects before they are removed and simplification of our\ntest suite. Furthermore, The test will now test the `repack` code\npath instead of performing the operations by calling\n`pack-objects`.\n\nRafael Silva (2):\n repack: teach --no-prune-packed to skip `git prune-packed`\n repack: avoid loosening promisor pack objects in partial clones\n\n Documentation/git-repack.txt | 5 +++++\n builtin/repack.c | 15 ++++++++++++---\n t/perf/p5600-partial-clone.sh | 4 ++++\n t/t5616-partial-clone.sh | 9 +++++++++\n t/t7700-repack.sh | 23 +++++++++++------------\n 5 files changed, 41 insertions(+), 15 deletions(-)\n\n-- \n2.31.0.565.gcc42f43761\n\n"},{"id":"421980","messageId":"20210414191403.4387-2-rafaeloliveira.cs@gmail.com","threadId":"55430","inReplyTo":"20210414191403.4387-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH 1/2] repack: teach --no-prune-packed to skip `git prune-packed`","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-14T19:14:02Z","receivedAt":"2021-04-14T19:15:13Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"The `git repack -d` command will remove any packfile that becomes\nredundant after repacking, and then call `git pruned-packed` for\npruning any unpacked objects. The command, however, does not offer\nan option to skip the `pruned-packed` execution in order to allow\ninspecting the objects before they are removed, forcing developers\nto either make some temporary code changes or to manually craft\nthe `pack-objects` command in order to skip their deletion:\n\n git pack-objects --honor-pack-keep --non-empty --all --reflog \\\n \t--unpack-unreachable\n\nLet’s teach `repack -d` to take --no-pruned-packed option that will\nsimply skip the call to `git prune-packed`, thus providing an easy\nway to debug the `repack` command. This also allows us to simplify\nour test suite by replacing calls to `pack-objects` with `repack`:\n\n git repack -A -d --no-prune-packed\n\nMoreover, using the repack command will actually test its code path\ninstead of just testing the real repack operation by calling\n`git pack-objects` directly. Let's refactor two tests in t7700 with\nthe new option.\n\nAside from improving our test suite, this new option will be used in\nan upcoming commit for testing the behavior of `repack` with partial\nclone repositories.\n\nSigned-off-by: Rafael Silva \n---\n Documentation/git-repack.txt | 5 +++++\n builtin/repack.c | 6 +++++-\n t/t7700-repack.sh | 23 +++++++++++------------\n 3 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 317d63cf0d..6ff7a1be16 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -86,6 +86,11 @@ to the new separate pack will be written.\n \tthis repository (or a direct copy of it)\n \tover HTTP or FTP. See linkgit:git-update-server-info[1].\n \n+--no-prune-packed::\n+\tOnly useful with `-d`. Do not remove redundant loose object\n+\tfiles by skipping the execution of `git prune-packed`.\n+\tSee linkgit:git-prune-packed[1].\n+\n --window=::\n --depth=::\n \tThese two options affect how the objects contained in the pack are\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2847fdfbab..6baaeb979c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -452,6 +452,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint keep_unreachable = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tint no_update_server_info = 0;\n+\tint no_prune_packed = 0;\n \tstruct pack_objects_args po_args = {NULL};\n \tint geometric_factor = 0;\n \n@@ -469,6 +470,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"pass --no-reuse-object to git-pack-objects\")),\n \t\tOPT_BOOL('n', NULL, &no_update_server_info,\n \t\t\t\tN_(\"do not run git-update-server-info\")),\n+\t\tOPT_BOOL(0, \"no-prune-packed\", &no_prune_packed,\n+\t\t\t\tN_(\"do not run git-prune-packed\")),\n \t\tOPT__QUIET(&po_args.quiet, N_(\"be quiet\")),\n \t\tOPT_BOOL('l', \"local\", &po_args.local,\n \t\t\t\tN_(\"pass --local to git-pack-objects\")),\n@@ -707,7 +710,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (!po_args.quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n-\t\tprune_packed_objects(opts);\n+\t\tif (!no_prune_packed)\n+\t\t\tprune_packed_objects(opts);\n \n \t\tif (!keep_unreachable &&\n \t\t (!(pack_everything & LOOSEN_UNREACHABLE) ||\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 25b235c063..728a16ad97 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -127,12 +127,7 @@ test_expect_success 'packed unreachable obs in alternate ODB are not loosened' '\n \tgit reset --hard HEAD^ &&\n \ttest_tick &&\n \tgit reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&\n-\t# The pack-objects call on the next line is equivalent to\n-\t# git repack -A -d without the call to prune-packed\n-\tgit pack-objects --honor-pack-keep --non-empty --all --reflog \\\n-\t --unpack-unreachable packlist &&\n \t! grep \"^$coid \" packlist &&\n \techo >.git/objects/info/alternates &&\n@@ -144,18 +139,22 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar\n \techo \"$coid\" | git pack-objects --non-empty --all --reflog pack &&\n \trm -f .git/objects/pack/* &&\n \tmv pack-* .git/objects/pack/ &&\n-\t# The pack-objects call on the next line is equivalent to\n-\t# git repack -A -d without the call to prune-packed\n-\tgit pack-objects --honor-pack-keep --non-empty --all --reflog \\\n-\t --unpack-unreachable packlist &&\n \t! grep \"^$coid \" &&\n \techo >.git/objects/info/alternates &&\n \ttest_must_fail git show $coid\n '\n \n+test_expect_success '-A -d and --no-prune-packed do not remove loose objects' '\n+\ttest_create_repo repo &&\n+\ttest_when_finished \"rm -rf repo\" &&\n+\ttest_commit -C repo commit &&\n+\tgit -C repo repack -A -d --no-prune-packed &&\n+\tgit -C repo count-objects -v >out &&\n+\tgrep \"^prune-packable: 3\" out\n+'\n+\n test_expect_success 'objects made unreachable by grafts only are kept' '\n \ttest_tick &&\n \tgit commit --allow-empty -m \"commit 4\" &&\n-- \n2.31.0.565.gcc42f43761\n\n"},{"id":"421979","messageId":"20210414191403.4387-3-rafaeloliveira.cs@gmail.com","threadId":"55430","inReplyTo":"20210414191403.4387-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-14T19:14:03Z","receivedAt":"2021-04-14T19:15:14Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"When `-A` and `-d` are used together, besides packing all objects (-A)\nand removing redundant packs (-d), it also unpack all unreachable\nobjects and deletes them by calling `git pruned-packed`. For a partial\nclone, that contains unreferenced objects, this results in unpacking\nall \"promisor\" objects and deleting them right after, which\nunnecessarily increases the `repack` execution time and disk usage\nduring the unpacking of the objects.\n\nFor instance, a partially cloned repository that filters all the blob\nobjects (e.g. \"--filter=blob:none\"), `repack` ends up unpacking all\nblobs into the filesystem that, depending on the repo size, makes\nnearly impossible to repack the operation before running out of disk.\n\nFor a partial clone, `git repack` calls `git pack-objects` twice: (1)\nfor handle the \"promisor\" objects and (2) for performing the repack\nwith --exclude-promisor-objects option, that results in unpacking and\ndeleting of the objects. Given that we actually should keep the\npromisor objects, let's teach `repack` to tell `pack-objects` to\n--keep the old \"promisor\" pack file.\n\nThe --keep-pack option takes only a packfile name, but we concatenate\nboth the path and the name in a single string. Instead, let's split\nthem into separate string in order to easily pass the packfile name\nlater.\n\nAdditionally, add a new perf test to evaluate the performance\nimpact made by this changes (tested on git.git):\n\n Test HEAD^ HEAD\n ------------------------------------------------------------\n 5600.5: gc 137.67(42.48+93.64) 8.08(6.91+1.45) -94.1%\n\nIn this particular script, the improvement is big because every\nobject in the newly-cloned partial repository is a promisor object.\n\nReported-by: SZEDER Gábor \nHelped-by: Jeff King \nSigned-off-by: Rafael Silva \n---\n builtin/repack.c | 9 +++++++--\n t/perf/p5600-partial-clone.sh | 4 ++++\n t/t5616-partial-clone.sh | 9 +++++++++\n 3 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6baaeb979c..0ecd76b79c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -20,7 +20,7 @@ static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n static int write_bitmaps = -1;\n static int use_delta_islands;\n-static char *packdir, *packtmp;\n+static char *packdir, *packtmp_name, *packtmp;\n \n static const char *const git_repack_usage[] = {\n \tN_(\"git repack []\"),\n@@ -533,7 +533,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n-\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n+\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n+\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n@@ -576,6 +577,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\trepack_promisor_objects(&po_args, &names);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n+\t\t\tfor_each_string_list_item(item, &names) {\n+\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n+\t\t\t\t\t packtmp_name, item->string);\n+\t\t\t}\n \t\t\tif (unpack_unreachable) {\n \t\t\t\tstrvec_pushf(&cmd.args,\n \t\t\t\t\t \"--unpack-unreachable=%s\",\ndiff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh\nindex ca785a3341..a965f2c4d6 100755\n--- a/t/perf/p5600-partial-clone.sh\n+++ b/t/perf/p5600-partial-clone.sh\n@@ -35,4 +35,8 @@ test_perf 'count non-promisor commits' '\n \tgit -C bare.git rev-list --all --count --exclude-promisor-objects\n '\n \n+test_perf 'gc' '\n+\tgit -C bare.git gc\n+'\n+\n test_done\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 5cb415386e..de77822735 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -548,6 +548,15 @@ test_expect_success 'fetch from a partial clone, protocol v2' '\n \tgrep \"version 2\" trace\n '\n \n+test_expect_success 'repack does not loose all objects' '\n+\trm -rf client &&\n+\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\tgit -C client repack -A -l -d --no-prune-packed &&\n+\tgit -C client count-objects -v >object-count &&\n+\tgrep \"^prune-packable: 0\" object-count\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.31.0.565.gcc42f43761\n\n"},{"id":"421981","messageId":"gohp6k5z0o6593.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"YHVECXHfZ1bidTJH@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] low-hanging performance fruit with promisor packs","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-14T19:22:18Z","receivedAt":"2021-04-14T19:22:23Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJeff King writes:\n\n>\n> I didn't explore any of those here, and I don't plan to look into them\n> anytime soon. I'm just documenting my findings for later.\n>\n> Anyway, here are the patches.\n>\n> [1/3]: is_promisor_object(): free tree buffer after parsing\n> [2/3]: lookup_unknown_object(): take a repository argument\n> [3/3]: revision: avoid parsing with --exclude-promisor-objects\n>\n> builtin/fsck.c | 2 +-\n> builtin/pack-objects.c | 2 +-\n> http-push.c | 2 +-\n> object.c | 7 +++----\n> object.h | 2 +-\n> packfile.c | 1 +\n> refs.c | 2 +-\n> revision.c | 2 +-\n> t/helper/test-example-decorate.c | 6 +++---\n> t/perf/p5600-partial-clone.sh | 12 ++++++++++++\n> upload-pack.c | 2 +-\n> walker.c | 2 +-\n> 12 files changed, 27 insertions(+), 15 deletions(-)\n>\n> -Peff\n\nI took look on this series and tested as well, together with the\nfix for the \"unpacking and deleting\" promisor objects situation.\n\nIt looks good to me.\n\n-- \nThanks\nRafael\n"},{"id":"421990","messageId":"xmqqr1jcwm96.fsf@gitster.g","threadId":"55430","inReplyTo":"20210414191403.4387-1-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 0/2] prevent `repack` to unpack and delete promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-14T22:10:29Z","receivedAt":"2021-04-14T22:10:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Silva writes:\n\n> This series is built on top of jk/promisor-optim. It conflicts with\n> changes on p5600 otherwise.\n>\n> The following patches fixes the issue where we unnecessarily turn loose\n> all the promisor objects and deletes them right after when running\n> `repack -A -d ..` (via `git gc) for a partial repository. \n>\n> Special thanks to Peff, for proposing a better approach for managing\n> the situation and for Jonathan Tan for earlier interaction on the\n> solution. Previously, I thought we should skip the promisor objects\n> by just adding a check in loosened_object_can_be_discarded(). However,\n> Peff pointed out that we can do better by realizing much sooner that\n> we should not even consider loosening the objects for the _old_ promisor\n> packs.\n>\n> It took me a bit to come up with the test because it seems `repack`\n> doesn't offer an option to skip the \"deletion of unpacked objects\",\n> so this series adds a new option to `repack` for skip the\n> `git prune-packed` execution thus allowing us to easily inspect the\n> unpacked objects before they are removed and simplification of our\n> test suite. Furthermore, The test will now test the `repack` code\n> path instead of performing the operations by calling\n> `pack-objects`.\n\nBeautiful. Thanks for working so well together, all of you.\n\n\n> Rafael Silva (2):\n> repack: teach --no-prune-packed to skip `git prune-packed`\n> repack: avoid loosening promisor pack objects in partial clones\n>\n> Documentation/git-repack.txt | 5 +++++\n> builtin/repack.c | 15 ++++++++++++---\n> t/perf/p5600-partial-clone.sh | 4 ++++\n> t/t5616-partial-clone.sh | 9 +++++++++\n> t/t7700-repack.sh | 23 +++++++++++------------\n> 5 files changed, 41 insertions(+), 15 deletions(-)\n"},{"id":"421994","messageId":"20210414235027.4064035-1-jonathantanmy@google.com","threadId":"55430","inReplyTo":"20210414191403.4387-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 1/2] repack: teach --no-prune-packed to skip `git prune-packed`","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-14T23:50:27Z","receivedAt":"2021-04-14T23:50:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> The `git repack -d` command will remove any packfile that becomes\n> redundant after repacking, and then call `git pruned-packed` for\n> pruning any unpacked objects.\n\ns/pruned-packed/prune-packed/ (note that there is no \"d\" after \"prune\")\nthroughout this commit message.\n\nAlso, if there are any objects pruned, they are packed objects, not\nunpacked objects. Maybe better to say \"...for pruning any objects\nalready in packs\".\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 25b235c063..728a16ad97 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -127,12 +127,7 @@ test_expect_success 'packed unreachable obs in alternate ODB are not loosened' '\n> \tgit reset --hard HEAD^ &&\n> \ttest_tick &&\n> \tgit reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&\n> -\t# The pack-objects call on the next line is equivalent to\n> -\t# git repack -A -d without the call to prune-packed\n> -\tgit pack-objects --honor-pack-keep --non-empty --all --reflog \\\n> -\t --unpack-unreachable -\trm -f .git/objects/pack/* &&\n> -\tmv pack-* .git/objects/pack/ &&\n> +\tgit repack -A -d --no-prune-packed &&\n\nThis is great!\n\n> +test_expect_success '-A -d and --no-prune-packed do not remove loose objects' '\n> +\ttest_create_repo repo &&\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\ttest_commit -C repo commit &&\n> +\tgit -C repo repack -A -d --no-prune-packed &&\n> +\tgit -C repo count-objects -v >out &&\n> +\tgrep \"^prune-packable: 3\" out\n> +'\n\nAs for the test description, I would prefer \"...do not remove loose\nobjects already in packs\".\n"},{"id":"421998","messageId":"20210415010454.4077355-1-jonathantanmy@google.com","threadId":"55430","inReplyTo":"20210414191403.4387-3-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-15T01:04:54Z","receivedAt":"2021-04-15T01:07:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> When `-A` and `-d` are used together, besides packing all objects (-A)\n> and removing redundant packs (-d), it also unpack all unreachable\n> objects and deletes them by calling `git pruned-packed`.\n\nI still think of these objects as not unreachable, even though I know\nthat pack-objects calls them that (the argument is called\n--unpack-unreachable). So I would say \"it also loosens all objects that\nwere previously packed but did not go into the new pack\", but perhaps\nthis is OK too.\n\n> For a partial\n> clone, that contains unreferenced objects, this results in unpacking\n> all \"promisor\" objects and deleting them right after, which\n> unnecessarily increases the `repack` execution time and disk usage\n> during the unpacking of the objects.\n\nI think that the commit message also needs to explain that we're\ndeleting the promisor objects immediately because they happen to be in a\npromisor pack. So perhaps this whole part could be written as follows:\n\n When \"git repack -Ad\" is run in a partial clone, \"pack-objects\" is\n invoked twice: once to repack all promisor objects, and once to repack\n all non-promisor objects. The latter \"pack-objects\" invocation is with\n --exclude-promisor-objects and --unpack-unreachable, which loosens all\n unused objects. Unfortunately, this includes promisor objects.\n\n Because the \"-d\" argument to \"git repack\" subsequently deletes all\n loose objects also in packs, these just-loosened promisor objects will\n be immediately deleted. But this extra disk churn is unnecessary in\n the first place.\n\n> For instance, a partially cloned repository that filters all the blob\n> objects (e.g. \"--filter=blob:none\"), `repack` ends up unpacking all\n> blobs into the filesystem that, depending on the repo size, makes\n> nearly impossible to repack the operation before running out of disk.\n> \n> For a partial clone, `git repack` calls `git pack-objects` twice: (1)\n> for handle the \"promisor\" objects and (2) for performing the repack\n> with --exclude-promisor-objects option, that results in unpacking and\n> deleting of the objects. Given that we actually should keep the\n> promisor objects, let's teach `repack` to tell `pack-objects` to\n> --keep the old \"promisor\" pack file.\n\nIt's not this call (2) that results in any deleting of the objects, but\nthe later call to prune_packed_objects(). Also, promisor objects are\nkept regardless of what we pass to \"pack-objects\" here (the keeping is\ndone separately). Maybe write (continuation from my suggestion above):\n\n In order to avoid this extra disk churn, pass the names of the\n promisor packfiles as \"--keep-pack\" arguments to this\n second invocation of \"pack-objects\". This informs \"pack-objects\" that\n the promisor objects are already in a safe packfile and, therefore, do\n not need to be loosened.\n\n> @@ -533,7 +533,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n> -\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n> +\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n> +\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n> \n> \tsigchain_push_common(remove_pack_on_signal);\n> \n\nNormally I would be concerned that packtmp_name is not freed, but in\nthis case, it's a static variable (same as packtmp).\n\n> @@ -576,6 +577,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\trepack_promisor_objects(&po_args, &names);\n> \n> \t\tif (existing_packs.nr && delete_redundant) {\n> +\t\t\tfor_each_string_list_item(item, &names) {\n> +\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n> +\t\t\t\t\t packtmp_name, item->string);\n> +\t\t\t}\n\nGit style is to not have braces for single-statement loops.\n\n> +test_expect_success 'repack does not loose all objects' '\n> +\trm -rf client &&\n> +\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n> +\ttest_when_finished \"rm -rf client\" &&\n> +\tgit -C client repack -A -l -d --no-prune-packed &&\n> +\tgit -C client count-objects -v >object-count &&\n> +\tgrep \"^prune-packable: 0\" object-count\n> +'\n\ns/loose all objects/loosen promisor objects/\n\nAlso, add a comment describing why we have \"--no-prune-packed\" there\n(probably something about not pruning any loose objects that are already\nin packs, so that we can verify that no redundant loose objects are\nbeing created in the first place).\n"},{"id":"422000","messageId":"xmqqo8egurx5.fsf@gitster.g","threadId":"55430","inReplyTo":"20210415010454.4077355-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-15T03:51:02Z","receivedAt":"2021-04-15T03:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> When `-A` and `-d` are used together, besides packing all objects (-A)\n>> and removing redundant packs (-d), it also unpack all unreachable\n>> objects and deletes them by calling `git pruned-packed`.\n>\n> I still think of these objects as not unreachable, even though I know\n> that pack-objects calls them that (the argument is called\n> --unpack-unreachable). So I would say \"it also loosens all objects that\n> were previously packed but did not go into the new pack\", but perhaps\n> this is OK too.\n\nHmph, that is puzzling. I understand that the operation about\n\n (1) finding all the objects that are still reachable and send them\n into a newly created pack, and\n\n (2) among the objects that were previously in the packs, eject\n those that weren't made into the new pack with the previous\n point.\n\nWhere did I get it wrong? If all the reachable ones are dealt with\nwith the first point, what is leftover is not reachable, no?\n\n\n"},{"id":"422011","messageId":"YHgBTPGTmDUBMGA9@coredump.intra.peff.net","threadId":"55430","inReplyTo":"xmqqo8egurx5.fsf@gitster.g","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-15T09:03:08Z","receivedAt":"2021-04-15T09:03:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 14, 2021 at 08:51:02PM -0700, Junio C Hamano wrote:\n\n> Jonathan Tan writes:\n> \n> >> When `-A` and `-d` are used together, besides packing all objects (-A)\n> >> and removing redundant packs (-d), it also unpack all unreachable\n> >> objects and deletes them by calling `git pruned-packed`.\n> >\n> > I still think of these objects as not unreachable, even though I know\n> > that pack-objects calls them that (the argument is called\n> > --unpack-unreachable). So I would say \"it also loosens all objects that\n> > were previously packed but did not go into the new pack\", but perhaps\n> > this is OK too.\n> \n> Hmph, that is puzzling. I understand that the operation about\n> \n> (1) finding all the objects that are still reachable and send them\n> into a newly created pack, and\n> \n> (2) among the objects that were previously in the packs, eject\n> those that weren't made into the new pack with the previous\n> point.\n> \n> Where did I get it wrong? If all the reachable ones are dealt with\n> with the first point, what is leftover is not reachable, no?\n\nRight. I think your understanding is correct, and the commit message is\na bit confused. Normally after we eject loose objects, they'd stay there\n(a follow-up git-gc may run git-prune and delete them, though if they\nwere recent enough not to just drop completely during the repack, then\ngit-prune would likewise leave them be). Talking about prune-packed here\nis misleading, because it usually has nothing to do with these objects.\n\nWhat makes the partial-clone situation under discussion interesting is\nthat the objects _are_ reachable. They are excluded from the new pack\nbecause we put them in a separate promisor pack. But we erroneously turn\nthem loose, rather than realizing that they were excluded for a\ndifferent reason.\n\nSo the fundamental bug is that we turn them loose at all. What makes the\nbug trickier to see is that when we run prune-packed afterwards, we then\nclean up the evidence of the bug (so it looks more like a performance\nproblem than a correctness one).\n\n-Peff\n"},{"id":"422012","messageId":"YHgBze9V7kq/TkqU@coredump.intra.peff.net","threadId":"55430","inReplyTo":"20210415010454.4077355-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-15T09:05:17Z","receivedAt":"2021-04-15T09:05:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 14, 2021 at 06:04:54PM -0700, Jonathan Tan wrote:\n\n> > @@ -576,6 +577,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \t\trepack_promisor_objects(&po_args, &names);\n> > \n> > \t\tif (existing_packs.nr && delete_redundant) {\n> > +\t\t\tfor_each_string_list_item(item, &names) {\n> > +\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n> > +\t\t\t\t\t packtmp_name, item->string);\n> > +\t\t\t}\n> \n> Git style is to not have braces for single-statement loops.\n\nIt is, though given that for_each_string_list_item() is a weird macro\ninstead of a regular for-loop, IMHO it makes things more obvious to have\nthe braces.\n\n(All the rest of your comments seemed quite good to me).\n\n-Peff\n"},{"id":"422014","messageId":"YHgEGHIgwfobcwDr@coredump.intra.peff.net","threadId":"55430","inReplyTo":"20210414191403.4387-1-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 0/2] prevent `repack` to unpack and delete promisor objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-15T09:15:04Z","receivedAt":"2021-04-15T09:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 14, 2021 at 09:14:01PM +0200, Rafael Silva wrote:\n\n> It took me a bit to come up with the test because it seems `repack`\n> doesn't offer an option to skip the \"deletion of unpacked objects\",\n> so this series adds a new option to `repack` for skip the\n> `git prune-packed` execution thus allowing us to easily inspect the\n> unpacked objects before they are removed and simplification of our\n> test suite. Furthermore, The test will now test the `repack` code\n> path instead of performing the operations by calling\n> `pack-objects`.\n\nThanks for working on this. Overall the patches seem sane, though I\nthink Jonathan's comments (especially about the confusion in the commit\nmessage of 2/2) are worth addressing.\n\nI have mixed feelings on the \"--no-prune-packed\" option, just because\nit's user-visible and I don't think it's something a normal user would\never really want.\n\nIn the new test (and I think in the old ones you modified, though I\ndidn't look carefully) the main thing we care about is whether we write\nout loose objects. So another solution would be to improve the debug\nlogging inside pack-objects to tell us more about what it's doing.\n\nThe fork of Git we use at GitHub has something similar; when we discard\nobjects or force them loose, we write their sha1 values to a log file.\nThis has come in handy for a lot of after-the-fact debugging (\"oops,\nthis repo is corrupted; did we intentionally delete object X?\").\n\nI wonder if we could do something similar with the trace2 facility. I\nknow it can be turned on via config, but I don't know how good the\nsupport is for enabling just one segment of data (and this may generate\na lot of entries, so people using trace2 for telemetry probably wouldn't\nwant it on).\n\nFor the purposes of the tests, though just a normal GIT_TRACE_PACK_DEBUG\nwould be plenty. I dunno. I don't want to open up a can of worms on\nlogging that would hold up getting this quite-substantial fix in place.\nBut once we add --no-prune-packed, it will be hard to take away.\n\n-Peff\n"},{"id":"422043","messageId":"xmqqzgxzqv5x.fsf@gitster.g","threadId":"55430","inReplyTo":"20210414191403.4387-3-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-15T18:06:50Z","receivedAt":"2021-04-15T18:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Silva writes:\n\n> For instance, a partially cloned repository that filters all the blob\n> objects (e.g. \"--filter=blob:none\"), `repack` ends up unpacking all\n> blobs into the filesystem that, depending on the repo size, makes\n> nearly impossible to repack the operation before running out of disk.\n\nCould you clarify this paragraph a bit more? It is unclear why the\nrepository has \"all the blob objects\" that it can loosen with repack\nin the first place, if it was cloned without any blob. Do you mean\nthat repack does not stop at the promisor pack boundary and instead\nlazily download blobs \"on-demand\", which ends up as loose objects?\n\nThanks.\n"},{"id":"422200","messageId":"gohp6ktuo4m6zt.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"20210415010454.4077355-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-18T07:12:55Z","receivedAt":"2021-04-18T07:13:00Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJonathan Tan writes:\n\n>> For a partial\n>> clone, that contains unreferenced objects, this results in unpacking\n>> all \"promisor\" objects and deleting them right after, which\n>> unnecessarily increases the `repack` execution time and disk usage\n>> during the unpacking of the objects.\n>\n> I think that the commit message also needs to explain that we're\n> deleting the promisor objects immediately because they happen to be in a\n> promisor pack. So perhaps this whole part could be written as follows:\n>\n> When \"git repack -Ad\" is run in a partial clone, \"pack-objects\" is\n> invoked twice: once to repack all promisor objects, and once to repack\n> all non-promisor objects. The latter \"pack-objects\" invocation is with\n> --exclude-promisor-objects and --unpack-unreachable, which loosens all\n> unused objects. Unfortunately, this includes promisor objects.\n>\n> Because the \"-d\" argument to \"git repack\" subsequently deletes all\n> loose objects also in packs, these just-loosened promisor objects will\n> be immediately deleted. But this extra disk churn is unnecessary in\n> the first place.\n>\n\nThanks for suggesting this message, obviously it's much better than\nmine specially because removes the confusion that I made when writing\nthe message about the `pack-objects` and `prune-packed`.\n\nI'll update patch's message on the next revision.\n\n>> For instance, a partially cloned repository that filters all the blob\n>> objects (e.g. \"--filter=blob:none\"), `repack` ends up unpacking all\n>> blobs into the filesystem that, depending on the repo size, makes\n>> nearly impossible to repack the operation before running out of disk.\n>> \n>> For a partial clone, `git repack` calls `git pack-objects` twice: (1)\n>> for handle the \"promisor\" objects and (2) for performing the repack\n>> with --exclude-promisor-objects option, that results in unpacking and\n>> deleting of the objects. Given that we actually should keep the\n>> promisor objects, let's teach `repack` to tell `pack-objects` to\n>> --keep the old \"promisor\" pack file.\n>\n> It's not this call (2) that results in any deleting of the objects, but\n> the later call to prune_packed_objects(). Also, promisor objects are\n> kept regardless of what we pass to \"pack-objects\" here (the keeping is\n> done separately). Maybe write (continuation from my suggestion above):\n>\n> In order to avoid this extra disk churn, pass the names of the\n> promisor packfiles as \"--keep-pack\" arguments to this\n> second invocation of \"pack-objects\". This informs \"pack-objects\" that\n> the promisor objects are already in a safe packfile and, therefore, do\n> not need to be loosened.\n>\n\nYou're right, the second call only loosens the object and the deleting\ncomes after it, which makes my message misleading. \n\nThe suggested paragraph explain better the situation, thanks for\nsuggesting it and will replace mine in the next revision.\n\n>> @@ -533,7 +533,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>> \t}\n>> \n>> \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n>> -\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n>> +\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n>> +\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n>> \n>> \tsigchain_push_common(remove_pack_on_signal);\n>> \n>\n> Normally I would be concerned that packtmp_name is not freed, but in\n> this case, it's a static variable (same as packtmp).\n>\n\nIndeed.\n\n>> @@ -576,6 +577,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>> \t\trepack_promisor_objects(&po_args, &names);\n>> \n>> \t\tif (existing_packs.nr && delete_redundant) {\n>> +\t\t\tfor_each_string_list_item(item, &names) {\n>> +\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n>> +\t\t\t\t\t packtmp_name, item->string);\n>> +\t\t\t}\n>\n> Git style is to not have braces for single-statement loops.\n>\n\nI preferred to keep the braces simply because the\nfor_each_string_list_item() is macro instead of a normal for-loop\nstatement. \n\n>> +test_expect_success 'repack does not loose all objects' '\n>> +\trm -rf client &&\n>> +\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n>> +\ttest_when_finished \"rm -rf client\" &&\n>> +\tgit -C client repack -A -l -d --no-prune-packed &&\n>> +\tgit -C client count-objects -v >object-count &&\n>> +\tgrep \"^prune-packable: 0\" object-count\n>> +'\n>\n> s/loose all objects/loosen promisor objects/\n>\n> Also, add a comment describing why we have \"--no-prune-packed\" there\n> (probably something about not pruning any loose objects that are already\n> in packs, so that we can verify that no redundant loose objects are\n> being created in the first place).\n\nThanks for the reviewing and helping clarify the patch intentions. I'll\naddress this comments on the next revision.\n\n-- \nThanks\nRafael\n"},{"id":"422202","messageId":"gohp6ky2dg9f7k.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"YHgEGHIgwfobcwDr@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] prevent `repack` to unpack and delete promisor objects","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-18T08:20:17Z","receivedAt":"2021-04-18T08:20:29Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJeff King writes:\n\n> On Wed, Apr 14, 2021 at 09:14:01PM +0200, Rafael Silva wrote:\n>\n>> It took me a bit to come up with the test because it seems `repack`\n>> doesn't offer an option to skip the \"deletion of unpacked objects\",\n>> so this series adds a new option to `repack` for skip the\n>> `git prune-packed` execution thus allowing us to easily inspect the\n>> unpacked objects before they are removed and simplification of our\n>> test suite. Furthermore, The test will now test the `repack` code\n>> path instead of performing the operations by calling\n>> `pack-objects`.\n>\n> Thanks for working on this. Overall the patches seem sane, though I\n> think Jonathan's comments (especially about the confusion in the commit\n> message of 2/2) are worth addressing.\n>\n\nThanks for the review. Indeed, I will address Jonathan's comments on\nthe next revision to remove the confused and misleading commit message.\n\nSorry for the confusion.\n\n>\n> I have mixed feelings on the \"--no-prune-packed\" option, just because\n> it's user-visible and I don't think it's something a normal user would\n> ever really want.\n>\n> In the new test (and I think in the old ones you modified, though I\n> didn't look carefully) the main thing we care about is whether we write\n> out loose objects. So another solution would be to improve the debug\n> logging inside pack-objects to tell us more about what it's doing.\n>\n\nHonestly, I'm also not happy adding an end user-visible variable just\nfor the sake of testing it, specially because it's unclear whether the\nuser will actually use this option.\n\nInitially, what convinced myself for proposing the changes was the\nadditional cleanup in our test suite, but giving a second thought I'm\nnot sure now whether this is a strong argument either.\n\n> The fork of Git we use at GitHub has something similar; when we discard\n> objects or force them loose, we write their sha1 values to a log file.\n> This has come in handy for a lot of after-the-fact debugging (\"oops,\n> this repo is corrupted; did we intentionally delete object X?\").\n>\n> I wonder if we could do something similar with the trace2 facility. I\n> know it can be turned on via config, but I don't know how good the\n> support is for enabling just one segment of data (and this may generate\n> a lot of entries, so people using trace2 for telemetry probably wouldn't\n> want it on).\n>\n> For the purposes of the tests, though just a normal GIT_TRACE_PACK_DEBUG\n> would be plenty. I dunno. I don't want to open up a can of worms on\n> logging that would hold up getting this quite-substantial fix in place.\n> But once we add --no-prune-packed, it will be hard to take away.\n>\n> -Peff\n\nAfter reading this comment and investigating a bit more, I believe\nincreasing the debug logging of `pack-objects` will help drop the first\npatch, at least for now, and allowing the fix to progress without the\n\"controversial\" user option. Later, we can revisit adding the\n`--no-prune-packed` (or a better named) option in case we think this\nwill be useful for the users.\n\nI'll be addressing this in v2.\n\n-- \nThanks\nRafael\n"},{"id":"422205","messageId":"gohp6ktuo456kq.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"xmqqzgxzqv5x.fsf@gitster.g","subject":"Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-18T08:40:22Z","receivedAt":"2021-04-18T08:40:30Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJunio C Hamano writes:\n\n> Rafael Silva writes:\n>\n>> For instance, a partially cloned repository that filters all the blob\n>> objects (e.g. \"--filter=blob:none\"), `repack` ends up unpacking all\n>> blobs into the filesystem that, depending on the repo size, makes\n>> nearly impossible to repack the operation before running out of disk.\n>\n> Could you clarify this paragraph a bit more? It is unclear why the\n> repository has \"all the blob objects\" that it can loosen with repack\n> in the first place, if it was cloned without any blob. Do you mean\n> that repack does not stop at the promisor pack boundary and instead\n> lazily download blobs \"on-demand\", which ends up as loose objects?\n>\n> Thanks.\n\nI should have written that we \"unpack (or better turn loose) the \npromisor objects\" not the `blob` objects. Instead, I should have\nwritten something like:\n\n ... ends up loosening all promisor objects, on this case all\n the `trees` and `commits` objects, into the filesystem ...\n\nApologise for the confusion and this misleading message. I'll clarify\nthis in the v2. \n\n-- \nThanks\nRafael\n"},{"id":"422211","messageId":"20210418135749.27152-1-rafaeloliveira.cs@gmail.com","threadId":"55430","inReplyTo":"20210414191403.4387-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v2 0/1] prevent `repack` to unpack and delete promisor objects","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-18T13:57:48Z","receivedAt":"2021-04-18T14:00:01Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"Here's the v2, sorry for the delay.\n\nThis series is built on top of jk/promisor-optim (now graduated to next). It\nconflicts with changes on p5600 otherwise.\n\nThe following patches fixes the issue where we unnecessarily turn loose\nall the promisor objects and deletes them right after when running\n`git repack -A -d ..` (via `git gc) for a partial repository. \n\nSpecial thanks to Peff, for proposing a better approach for managing\nthe situation and for Jonathan Tan for earlier interaction on the\nsolution. Previously, I thought we should skip the promisor objects\nby just adding a check in loosened_object_can_be_discarded(). However,\nPeff pointed out that we can do better by realizing much sooner that\nwe should not even consider loosening the objects for the _old_ promisor\npacks.\n\n===============\nChanges from v1:\n\n * v2 contains only patch instead of two from the previous round.\n\n * I include Jonathan's suggestion throughout the entire patch, most\n notably the commit message that was confused and a misleading. Sorry\n for that. Special thanks to him for helping suggesting a _much better_\n commit message among other suggested improvements and corrections.\n\n * The [Patch 1/2] from v1 is dropped. It was adding a user-visible\n option to `repack` in order to skip the call to `prune-packed` and\n prevent destroying the evidence of the bug so it can be tested.\n However, Peff raised some concerns about adding a user-visible option\n (that is unclear whether the user will ever want) just for the sake\n of testing it - honestly, I wasn't too happy with this either. In v2, we\n now teach `pack-objects` to count the objects of interest\n (loosened objects) and emit this information via trace2, which allows\n checking the debugging logging for the evidence.\n\n * Test is modified to rely on the the added trace2 event information.\n\n * Updates on the performance numbers, including adding one execution\n for a bigger repository (linux.git).\n\nEven though the v2 is 1-patch long, I thought it make thing clear to read if\na re-send the cover-letter as the [Patch v2 1/1] is already a bit lengthy,\nand explaining the whole story here.\n\nRafael Silva (1):\n repack: avoid loosening promisor objects in partial clones\n\n builtin/pack-objects.c | 8 +++++++-\n builtin/repack.c | 9 +++++++--\n t/perf/p5600-partial-clone.sh | 4 ++++\n t/t5616-partial-clone.sh | 8 ++++++++\n 4 files changed, 26 insertions(+), 3 deletions(-)\n\nRange-diff against v1:\n1: 2431f8b75d < -: ---------- repack: teach --no-prune-packed to skip `git prune-packed`\n2: 1331049a86 ! 1: 9d996393c9 repack: avoid loosening promisor pack objects in partial clones\n @@ Metadata\n Author: Rafael Silva \n \n ## Commit message ##\n - repack: avoid loosening promisor pack objects in partial clones\n -\n - When `-A` and `-d` are used together, besides packing all objects (-A)\n - and removing redundant packs (-d), it also unpack all unreachable\n - objects and deletes them by calling `git pruned-packed`. For a partial\n - clone, that contains unreferenced objects, this results in unpacking\n - all \"promisor\" objects and deleting them right after, which\n - unnecessarily increases the `repack` execution time and disk usage\n - during the unpacking of the objects.\n -\n - For instance, a partially cloned repository that filters all the blob\n - objects (e.g. \"--filter=blob:none\"), `repack` ends up unpacking all\n - blobs into the filesystem that, depending on the repo size, makes\n - nearly impossible to repack the operation before running out of disk.\n -\n - For a partial clone, `git repack` calls `git pack-objects` twice: (1)\n - for handle the \"promisor\" objects and (2) for performing the repack\n - with --exclude-promisor-objects option, that results in unpacking and\n - deleting of the objects. Given that we actually should keep the\n - promisor objects, let's teach `repack` to tell `pack-objects` to\n - --keep the old \"promisor\" pack file.\n -\n - The --keep-pack option takes only a packfile name, but we concatenate\n - both the path and the name in a single string. Instead, let's split\n - them into separate string in order to easily pass the packfile name\n - later.\n -\n - Additionally, add a new perf test to evaluate the performance\n - impact made by this changes (tested on git.git):\n -\n - Test HEAD^ HEAD\n - ------------------------------------------------------------\n - 5600.5: gc 137.67(42.48+93.64) 8.08(6.91+1.45) -94.1%\n -\n - In this particular script, the improvement is big because every\n - object in the newly-cloned partial repository is a promisor object.\n + repack: avoid loosening promisor objects in partial clones\n +\n + When `git repack -A -d` is run in a partial clone, `pack-objects`\n + is invoked twice: once to repack all promisor objects, and once to\n + repack all non-promisor objects. The latter `pack-objects` invocation\n + is with --exclude-promisor-objects and --unpack-unreachable, which\n + loosens all unused objects. Unfortunately, this includes promisor\n + objects.\n +\n + Because the -d argument to `git repack` subsequently deletes all loose\n + objects also in packs, these just-loosened promisor objects will be\n + immediately deleted. However, this extra disk churn is unnecessary in\n + the first place. For example, a newly-clone partial repo that filters\n + all blob objects (e.g. `--filter=blob:none`), `repack` ends up\n + unpacking all trees and commits into the filesystem because every\n + object, in this particular case, is a promisor object. Depending on\n + the repo size, this increases the disk usage considerably: In my copy\n + of the linux.git, the object directory peaked 26GB of more disk usage.\n +\n + In order to avoid this extra disk churn, pass the names of the promisor\n + packfiles as --keep-pack arguments to the second invocation of\n + `pack-objects`. This informs `pack-objects` that the promisor objects\n + are already in a safe packfile and, therefore, do not need to be\n + loosened. The --keep-pack option takes only a packfile name, but we\n + concatenate both the path and the name in a single string. Instead,\n + let's split them into separate string in order to easily pass the\n + packfile name later.\n +\n + For testing, we need to validate whether any object was loosened.\n + However, the \"evidence\" (loosened objects) is deleted during the\n + process which prevents us from inspecting the object directory.\n + Instead, let's teach `pack-objects` to count loosened objects and\n + emit via trace2 thus allowing inspecting the debug events after the\n + process is finished. This new event is used on the added regression\n + test.\n +\n + Lastly, add a new perf test to evaluate the performance impact\n + made by this changes (tested on git.git):\n +\n + Test HEAD^ HEAD\n + ----------------------------------------------------------\n + 5600.3: gc 134.38(41.93+90.95) 7.80(6.72+1.35) -94.2%\n +\n + For a bigger repository, such as linux.git, the improvement is\n + even bigger:\n +\n + Test HEAD^ HEAD\n + -------------------------------------------------------------------\n + 5600.3: gc 6833.00(918.07+3162.74) 268.79(227.02+39.18) -96.1%\n +\n + These improvements are particular big because every object in the\n + newly-cloned partial repository is a promisor object.\n \n Reported-by: SZEDER Gábor \n Helped-by: Jeff King \n + Helped-by: Jonathan Tan \n Signed-off-by: Rafael Silva \n - Signed-off-by: Junio C Hamano \n +\n + ## builtin/pack-objects.c ##\n +@@ builtin/pack-objects.c: static void loosen_unused_packed_objects(void)\n + {\n + \tstruct packed_git *p;\n + \tuint32_t i;\n ++\tuint32_t loosened_objects_nr = 0;\n + \tstruct object_id oid;\n + \n + \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n +@@ builtin/pack-objects.c: static void loosen_unused_packed_objects(void)\n + \t\t\tnth_packed_object_id(&oid, p, i);\n + \t\t\tif (!packlist_find(&to_pack, &oid) &&\n + \t\t\t !has_sha1_pack_kept_or_nonlocal(&oid) &&\n +-\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime))\n ++\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime)) {\n + \t\t\t\tif (force_object_loose(&oid, p->mtime))\n + \t\t\t\t\tdie(_(\"unable to force loose object\"));\n ++\t\t\t\tloosened_objects_nr++;\n ++\t\t\t}\n + \t\t}\n + \t}\n ++\n ++\ttrace2_data_intmax(\"pack-objects\", the_repository,\n ++\t\t\t \"loosen_unused_packed_objects/loosened\", loosened_objects_nr);\n + }\n + \n + /*\n \n ## builtin/repack.c ##\n @@ builtin/repack.c: static int delta_base_offset = 1;\n @@ t/t5616-partial-clone.sh: test_expect_success 'fetch from a partial clone, proto\n \tgrep \"version 2\" trace\n '\n \n -+test_expect_success 'repack does not loose all objects' '\n -+\trm -rf client &&\n ++test_expect_success 'repack does not loosen promisor objects' '\n ++\trm -rf client trace &&\n +\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n -+\ttest_when_finished \"rm -rf client\" &&\n -+\tgit -C client repack -A -l -d --no-prune-packed &&\n -+\tgit -C client count-objects -v >object-count &&\n -+\tgrep \"^prune-packable: 0\" object-count\n ++\ttest_when_finished \"rm -rf client trace\" &&\n ++\tGIT_TRACE2_PERF=\"$(pwd)/trace\" git -C client repack -A -d &&\n ++\tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n +'\n +\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.31.0.699.g8849f49b87\n\n"},{"id":"422212","messageId":"20210418135749.27152-2-rafaeloliveira.cs@gmail.com","threadId":"55430","inReplyTo":"20210418135749.27152-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v2 1/1] repack: avoid loosening promisor objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-18T13:57:49Z","receivedAt":"2021-04-18T14:00:18Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"When `git repack -A -d` is run in a partial clone, `pack-objects`\nis invoked twice: once to repack all promisor objects, and once to\nrepack all non-promisor objects. The latter `pack-objects` invocation\nis with --exclude-promisor-objects and --unpack-unreachable, which\nloosens all unused objects. Unfortunately, this includes promisor\nobjects.\n\nBecause the -d argument to `git repack` subsequently deletes all loose\nobjects also in packs, these just-loosened promisor objects will be\nimmediately deleted. However, this extra disk churn is unnecessary in\nthe first place. For example, a newly-clone partial repo that filters\nall blob objects (e.g. `--filter=blob:none`), `repack` ends up\nunpacking all trees and commits into the filesystem because every\nobject, in this particular case, is a promisor object. Depending on\nthe repo size, this increases the disk usage considerably: In my copy\nof the linux.git, the object directory peaked 26GB of more disk usage.\n\nIn order to avoid this extra disk churn, pass the names of the promisor\npackfiles as --keep-pack arguments to the second invocation of\n`pack-objects`. This informs `pack-objects` that the promisor objects\nare already in a safe packfile and, therefore, do not need to be\nloosened. The --keep-pack option takes only a packfile name, but we\nconcatenate both the path and the name in a single string. Instead,\nlet's split them into separate string in order to easily pass the\npackfile name later.\n\nFor testing, we need to validate whether any object was loosened.\nHowever, the \"evidence\" (loosened objects) is deleted during the\nprocess which prevents us from inspecting the object directory.\nInstead, let's teach `pack-objects` to count loosened objects and\nemit via trace2 thus allowing inspecting the debug events after the\nprocess is finished. This new event is used on the added regression\ntest.\n\nLastly, add a new perf test to evaluate the performance impact\nmade by this changes (tested on git.git):\n\n Test HEAD^ HEAD\n ----------------------------------------------------------\n 5600.3: gc 134.38(41.93+90.95) 7.80(6.72+1.35) -94.2%\n\nFor a bigger repository, such as linux.git, the improvement is\neven bigger:\n\n Test HEAD^ HEAD\n -------------------------------------------------------------------\n 5600.3: gc 6833.00(918.07+3162.74) 268.79(227.02+39.18) -96.1%\n\nThese improvements are particular big because every object in the\nnewly-cloned partial repository is a promisor object.\n\nReported-by: SZEDER Gábor \nHelped-by: Jeff King \nHelped-by: Jonathan Tan \nSigned-off-by: Rafael Silva \n---\n builtin/pack-objects.c | 8 +++++++-\n builtin/repack.c | 9 +++++++--\n t/perf/p5600-partial-clone.sh | 4 ++++\n t/t5616-partial-clone.sh | 8 ++++++++\n 4 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 40ee6fa19f..73889cec95 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3479,6 +3479,7 @@ static void loosen_unused_packed_objects(void)\n {\n \tstruct packed_git *p;\n \tuint32_t i;\n+\tuint32_t loosened_objects_nr = 0;\n \tstruct object_id oid;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n@@ -3492,11 +3493,16 @@ static void loosen_unused_packed_objects(void)\n \t\t\tnth_packed_object_id(&oid, p, i);\n \t\t\tif (!packlist_find(&to_pack, &oid) &&\n \t\t\t !has_sha1_pack_kept_or_nonlocal(&oid) &&\n-\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime))\n+\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime)) {\n \t\t\t\tif (force_object_loose(&oid, p->mtime))\n \t\t\t\t\tdie(_(\"unable to force loose object\"));\n+\t\t\t\tloosened_objects_nr++;\n+\t\t\t}\n \t\t}\n \t}\n+\n+\ttrace2_data_intmax(\"pack-objects\", the_repository,\n+\t\t\t \"loosen_unused_packed_objects/loosened\", loosened_objects_nr);\n }\n \n /*\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2847fdfbab..5f9bc74adc 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -20,7 +20,7 @@ static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n static int write_bitmaps = -1;\n static int use_delta_islands;\n-static char *packdir, *packtmp;\n+static char *packdir, *packtmp_name, *packtmp;\n \n static const char *const git_repack_usage[] = {\n \tN_(\"git repack []\"),\n@@ -530,7 +530,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n-\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n+\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n+\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n@@ -573,6 +574,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\trepack_promisor_objects(&po_args, &names);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n+\t\t\tfor_each_string_list_item(item, &names) {\n+\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n+\t\t\t\t\t packtmp_name, item->string);\n+\t\t\t}\n \t\t\tif (unpack_unreachable) {\n \t\t\t\tstrvec_pushf(&cmd.args,\n \t\t\t\t\t \"--unpack-unreachable=%s\",\ndiff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh\nindex ca785a3341..a965f2c4d6 100755\n--- a/t/perf/p5600-partial-clone.sh\n+++ b/t/perf/p5600-partial-clone.sh\n@@ -35,4 +35,8 @@ test_perf 'count non-promisor commits' '\n \tgit -C bare.git rev-list --all --count --exclude-promisor-objects\n '\n \n+test_perf 'gc' '\n+\tgit -C bare.git gc\n+'\n+\n test_done\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 5cb415386e..6e3e7565d0 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -548,6 +548,14 @@ test_expect_success 'fetch from a partial clone, protocol v2' '\n \tgrep \"version 2\" trace\n '\n \n+test_expect_success 'repack does not loosen promisor objects' '\n+\trm -rf client trace &&\n+\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n+\ttest_when_finished \"rm -rf client trace\" &&\n+\tGIT_TRACE2_PERF=\"$(pwd)/trace\" git -C client repack -A -d &&\n+\tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.31.0.699.g8849f49b87\n\n"},{"id":"422213","messageId":"gohp6kr1j8nmdv.fsf@cpm12071.fritz.box","threadId":"55430","inReplyTo":"20210414235027.4064035-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/2] repack: teach --no-prune-packed to skip `git prune-packed`","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-18T14:15:22Z","receivedAt":"2021-04-18T14:15:36Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nThanks for reviewing this patch. really appreciate it.\n\nHowever, in the v2, I decided to drop this in favor of just adding a\ntrace2 event that emits all the loosened objects. This is to avoid\nadding user-visible option just for the sake of testing it.\n\nJonathan Tan writes:\n\n>> The `git repack -d` command will remove any packfile that becomes\n>> redundant after repacking, and then call `git pruned-packed` for\n>> pruning any unpacked objects.\n>\n> s/pruned-packed/prune-packed/ (note that there is no \"d\" after \"prune\")\n> throughout this commit message.\n>\n> Also, if there are any objects pruned, they are packed objects, not\n> unpacked objects. Maybe better to say \"...for pruning any objects\n> already in packs\".\n>\n\nThanks for catching the typo and the clarification.\n\n>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>> index 25b235c063..728a16ad97 100755\n>> --- a/t/t7700-repack.sh\n>> +++ b/t/t7700-repack.sh\n>> @@ -127,12 +127,7 @@ test_expect_success 'packed unreachable obs in alternate ODB are not loosened' '\n>> \tgit reset --hard HEAD^ &&\n>> \ttest_tick &&\n>> \tgit reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&\n>> -\t# The pack-objects call on the next line is equivalent to\n>> -\t# git repack -A -d without the call to prune-packed\n>> -\tgit pack-objects --honor-pack-keep --non-empty --all --reflog \\\n>> -\t --unpack-unreachable > -\trm -f .git/objects/pack/* &&\n>> -\tmv pack-* .git/objects/pack/ &&\n>> +\tgit repack -A -d --no-prune-packed &&\n>\n> This is great!\n>\n\nUnfortunately, with the drop of this patch in v2, we no longer refactor\nthese tests. However, this still might be possible with the trace2 event\ninformation that is now-added in the v2. Of course, if this doesn't\nimpact or weakens the test in anyway.\n\n>> +test_expect_success '-A -d and --no-prune-packed do not remove loose objects' '\n>> +\ttest_create_repo repo &&\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\ttest_commit -C repo commit &&\n>> +\tgit -C repo repack -A -d --no-prune-packed &&\n>> +\tgit -C repo count-objects -v >out &&\n>> +\tgrep \"^prune-packable: 3\" out\n>> +'\n>\n> As for the test description, I would prefer \"...do not remove loose\n> objects already in packs\".\n\nYes, this is an improvement of the test description. \n\n-- \nThanks\nRafael\n"},{"id":"422323","messageId":"20210419191553.581877-1-jonathantanmy@google.com","threadId":"55430","inReplyTo":"20210418135749.27152-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH v2 1/1] repack: avoid loosening promisor objects in partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-19T19:15:53Z","receivedAt":"2021-04-19T19:15:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> When `git repack -A -d` is run in a partial clone, `pack-objects`\n> is invoked twice: once to repack all promisor objects, and once to\n> repack all non-promisor objects. The latter `pack-objects` invocation\n> is with --exclude-promisor-objects and --unpack-unreachable, which\n> loosens all unused objects. Unfortunately, this includes promisor\n> objects.\n\ns/loosens all unused objects/loosens all objects unused during this invocation/\n\n> [snip] The --keep-pack option takes only a packfile name, but we\n> concatenate both the path and the name in a single string. Instead,\n> let's split them into separate string in order to easily pass the\n> packfile name later.\n\nI think mentioning this part is unnecessary in the commit message.\n\nWith or without these changes, this patch looks good to me.\n"},{"id":"422352","messageId":"xmqqa6pt98j4.fsf@gitster.g","threadId":"55430","inReplyTo":"20210418135749.27152-2-rafaeloliveira.cs@gmail.com","subject":"Re: [PATCH v2 1/1] repack: avoid loosening promisor objects in partial clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-19T23:09:03Z","receivedAt":"2021-04-19T23:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Silva writes:\n\n> When `git repack -A -d` is run in a partial clone, `pack-objects`\n> is invoked twice: once to repack all promisor objects, and once to\n> repack all non-promisor objects. The latter `pack-objects` invocation\n> is with --exclude-promisor-objects and --unpack-unreachable, which\n> loosens all unused objects. Unfortunately, this includes promisor\n> objects.\n>\n> Because the -d argument to `git repack` subsequently deletes all loose\n> objects also in packs, these just-loosened promisor objects will be\n> immediately deleted. However, this extra disk churn is unnecessary in\n> the first place. For example, a newly-clone partial repo that filters\n\n\"in a newly-cloned partial repo\", I'd think.\n\n> For testing, we need to validate whether any object was loosened.\n> However, the \"evidence\" (loosened objects) is deleted during the\n> process which prevents us from inspecting the object directory.\n> Instead, let's teach `pack-objects` to count loosened objects and\n> emit via trace2 thus allowing inspecting the debug events after the\n> process is finished. This new event is used on the added regression\n> test.\n\nNicely designed.\n\n> +\tuint32_t loosened_objects_nr = 0;\n> \tstruct object_id oid;\n> \n> \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> @@ -3492,11 +3493,16 @@ static void loosen_unused_packed_objects(void)\n> \t\t\tnth_packed_object_id(&oid, p, i);\n> \t\t\tif (!packlist_find(&to_pack, &oid) &&\n> \t\t\t !has_sha1_pack_kept_or_nonlocal(&oid) &&\n> -\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime))\n> +\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime)) {\n> \t\t\t\tif (force_object_loose(&oid, p->mtime))\n> \t\t\t\t\tdie(_(\"unable to force loose object\"));\n> +\t\t\t\tloosened_objects_nr++;\n> +\t\t\t}\n> \t\t}\n> \t}\n> +\n> +\ttrace2_data_intmax(\"pack-objects\", the_repository,\n> +\t\t\t \"loosen_unused_packed_objects/loosened\", loosened_objects_nr);\n> }\n\nOK, so this is just the \"stats\".\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 2847fdfbab..5f9bc74adc 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -20,7 +20,7 @@ static int delta_base_offset = 1;\n> static int pack_kept_objects = -1;\n> static int write_bitmaps = -1;\n> static int use_delta_islands;\n> -static char *packdir, *packtmp;\n> +static char *packdir, *packtmp_name, *packtmp;\n> \n> static const char *const git_repack_usage[] = {\n> \tN_(\"git repack []\"),\n> @@ -530,7 +530,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n> -\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n> +\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n> +\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n\nJust a mental note, but we should move away from \".tmp-$$\" that is a\nremnant from the days back when this was a shell script, and use the\ntempfile.h API (#leftoverbits). Such a change must not be part of\nthis topic, of course.\n\nThanks. Will queue and see what others say.\n\n"},{"id":"422618","messageId":"gohp6kfszjmptg.fsf@gmail.com","threadId":"55430","inReplyTo":"20210419191553.581877-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 1/1] repack: avoid loosening promisor objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-21T18:54:06Z","receivedAt":"2021-04-21T18:54:12Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJonathan Tan writes:\n\n>> When `git repack -A -d` is run in a partial clone, `pack-objects`\n>> is invoked twice: once to repack all promisor objects, and once to\n>> repack all non-promisor objects. The latter `pack-objects` invocation\n>> is with --exclude-promisor-objects and --unpack-unreachable, which\n>> loosens all unused objects. Unfortunately, this includes promisor\n>> objects.\n>\n> s/loosens all unused objects/loosens all objects unused during this invocation/\n>\n\nThanks, will include this change in v3.\n\n>> [snip] The --keep-pack option takes only a packfile name, but we\n>> concatenate both the path and the name in a single string. Instead,\n>> let's split them into separate string in order to easily pass the\n>> packfile name later.\n>\n> I think mentioning this part is unnecessary in the commit message.\n>\n\nmake sense. I'll remove then to reduce the commit message.\n\n> With or without these changes, this patch looks good to me.\n\nThanks for the review.\n\n-- \nThanks\nRafael\n"},{"id":"422626","messageId":"gohp6kim4fl9tc.fsf@gmail.com","threadId":"55430","inReplyTo":"xmqqa6pt98j4.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] repack: avoid loosening promisor objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-21T19:25:04Z","receivedAt":"2021-04-21T19:25:10Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"\nJunio C Hamano writes:\n\n> Rafael Silva writes:\n>\n>> When `git repack -A -d` is run in a partial clone, `pack-objects`\n>> is invoked twice: once to repack all promisor objects, and once to\n>> repack all non-promisor objects. The latter `pack-objects` invocation\n>> is with --exclude-promisor-objects and --unpack-unreachable, which\n>> loosens all unused objects. Unfortunately, this includes promisor\n>> objects.\n>>\n>> Because the -d argument to `git repack` subsequently deletes all loose\n>> objects also in packs, these just-loosened promisor objects will be\n>> immediately deleted. However, this extra disk churn is unnecessary in\n>> the first place. For example, a newly-clone partial repo that filters\n>\n> \"in a newly-cloned partial repo\", I'd think.\n>\n\nThanks, will fix on the next revision.\n\n>> For testing, we need to validate whether any object was loosened.\n>> However, the \"evidence\" (loosened objects) is deleted during the\n>> process which prevents us from inspecting the object directory.\n>> Instead, let's teach `pack-objects` to count loosened objects and\n>> emit via trace2 thus allowing inspecting the debug events after the\n>> process is finished. This new event is used on the added regression\n>> test.\n>\n> Nicely designed.\n>\n\nThanks :)\n\n>> +\tuint32_t loosened_objects_nr = 0;\n>> \tstruct object_id oid;\n>> \n>> \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n>> @@ -3492,11 +3493,16 @@ static void loosen_unused_packed_objects(void)\n>> \t\t\tnth_packed_object_id(&oid, p, i);\n>> \t\t\tif (!packlist_find(&to_pack, &oid) &&\n>> \t\t\t !has_sha1_pack_kept_or_nonlocal(&oid) &&\n>> -\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime))\n>> +\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime)) {\n>> \t\t\t\tif (force_object_loose(&oid, p->mtime))\n>> \t\t\t\t\tdie(_(\"unable to force loose object\"));\n>> +\t\t\t\tloosened_objects_nr++;\n>> +\t\t\t}\n>> \t\t}\n>> \t}\n>> +\n>> +\ttrace2_data_intmax(\"pack-objects\", the_repository,\n>> +\t\t\t \"loosen_unused_packed_objects/loosened\", loosened_objects_nr);\n>> }\n>\n> OK, so this is just the \"stats\".\n>\n>> diff --git a/builtin/repack.c b/builtin/repack.c\n>> index 2847fdfbab..5f9bc74adc 100644\n>> --- a/builtin/repack.c\n>> +++ b/builtin/repack.c\n>> @@ -20,7 +20,7 @@ static int delta_base_offset = 1;\n>> static int pack_kept_objects = -1;\n>> static int write_bitmaps = -1;\n>> static int use_delta_islands;\n>> -static char *packdir, *packtmp;\n>> +static char *packdir, *packtmp_name, *packtmp;\n>> \n>> static const char *const git_repack_usage[] = {\n>> \tN_(\"git repack []\"),\n>> @@ -530,7 +530,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>> \t}\n>> \n>> \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n>> -\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n>> +\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n>> +\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n>\n> Just a mental note, but we should move away from \".tmp-$$\" that is a\n> remnant from the days back when this was a shell script, and use the\n> tempfile.h API (#leftoverbits). Such a change must not be part of\n> this topic, of course.\n>\n\nIndeed. This should be move tempfile.h API.\n\n>\n> Thanks. Will queue and see what others say.\n\nThanks for reviewing it.\n\n-- \nThanks\nRafael\n"},{"id":"422629","messageId":"20210421193212.79784-1-rafaeloliveira.cs@gmail.com","threadId":"55430","inReplyTo":"20210418135749.27152-1-rafaeloliveira.cs@gmail.com","subject":"[PATCH v3] repack: avoid loosening promisor objects in partial clones","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2021-04-21T19:32:12Z","receivedAt":"2021-04-21T19:35:00Z","isPatch":true,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"When `git repack -A -d` is run in a partial clone, `pack-objects`\nis invoked twice: once to repack all promisor objects, and once to\nrepack all non-promisor objects. The latter `pack-objects` invocation\nis with --exclude-promisor-objects and --unpack-unreachable, which\nloosens all objects unused during this invocation. Unfortunately,\nthis includes promisor objects.\n\nBecause the -d argument to `git repack` subsequently deletes all loose\nobjects also in packs, these just-loosened promisor objects will be\nimmediately deleted. However, this extra disk churn is unnecessary in\nthe first place. For example, in a newly-cloned partial repo that\nfilters all blob objects (e.g. `--filter=blob:none`), `repack` ends up\nunpacking all trees and commits into the filesystem because every\nobject, in this particular case, is a promisor object. Depending on\nthe repo size, this increases the disk usage considerably: In my copy\nof the linux.git, the object directory peaked 26GB of more disk usage.\n\nIn order to avoid this extra disk churn, pass the names of the promisor\npackfiles as --keep-pack arguments to the second invocation of\n`pack-objects`. This informs `pack-objects` that the promisor objects\nare already in a safe packfile and, therefore, do not need to be\nloosened.\n\nFor testing, we need to validate whether any object was loosened.\nHowever, the \"evidence\" (loosened objects) is deleted during the\nprocess which prevents us from inspecting the object directory.\nInstead, let's teach `pack-objects` to count loosened objects and\nemit via trace2 thus allowing inspecting the debug events after the\nprocess is finished. This new event is used on the added regression\ntest.\n\nLastly, add a new perf test to evaluate the performance impact\nmade by this changes (tested on git.git):\n\n Test HEAD^ HEAD\n ----------------------------------------------------------\n 5600.3: gc 134.38(41.93+90.95) 7.80(6.72+1.35) -94.2%\n\nFor a bigger repository, such as linux.git, the improvement is\neven bigger:\n\n Test HEAD^ HEAD\n -------------------------------------------------------------------\n 5600.3: gc 6833.00(918.07+3162.74) 268.79(227.02+39.18) -96.1%\n\nThese improvements are particular big because every object in the\nnewly-cloned partial repository is a promisor object.\n\nReported-by: SZEDER Gábor \nHelped-by: Jeff King \nHelped-by: Jonathan Tan \nSigned-off-by: Rafael Silva \n---\n\nThis series is built on top of jk/promisor-optim (now graduated to next). It\nconflicts with changes on p5600 otherwise.\n\nI believe the changes is pretty clear from the `range-diff` (only changes\non the commit message).\n\nRange-diff against v2:\n1: 7380e3e724 ! 1: 6d94907ca3 repack: avoid loosening promisor objects in partial clones\n @@ Commit message\n is invoked twice: once to repack all promisor objects, and once to\n repack all non-promisor objects. The latter `pack-objects` invocation\n is with --exclude-promisor-objects and --unpack-unreachable, which\n - loosens all unused objects. Unfortunately, this includes promisor\n - objects.\n + loosens all objects unused during this invocation. Unfortunately,\n + this includes promisor objects.\n \n Because the -d argument to `git repack` subsequently deletes all loose\n objects also in packs, these just-loosened promisor objects will be\n immediately deleted. However, this extra disk churn is unnecessary in\n - the first place. For example, a newly-clone partial repo that filters\n - all blob objects (e.g. `--filter=blob:none`), `repack` ends up\n + the first place. For example, in a newly-cloned partial repo that\n + filters all blob objects (e.g. `--filter=blob:none`), `repack` ends up\n unpacking all trees and commits into the filesystem because every\n object, in this particular case, is a promisor object. Depending on\n the repo size, this increases the disk usage considerably: In my copy\n @@ Commit message\n packfiles as --keep-pack arguments to the second invocation of\n `pack-objects`. This informs `pack-objects` that the promisor objects\n are already in a safe packfile and, therefore, do not need to be\n - loosened. The --keep-pack option takes only a packfile name, but we\n - concatenate both the path and the name in a single string. Instead,\n - let's split them into separate string in order to easily pass the\n - packfile name later.\n + loosened.\n \n For testing, we need to validate whether any object was loosened.\n However, the \"evidence\" (loosened objects) is deleted during the\n\n builtin/pack-objects.c | 8 +++++++-\n builtin/repack.c | 9 +++++++--\n t/perf/p5600-partial-clone.sh | 4 ++++\n t/t5616-partial-clone.sh | 8 ++++++++\n 4 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c1186f50a3..208cce228d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3479,6 +3479,7 @@ static void loosen_unused_packed_objects(void)\n {\n \tstruct packed_git *p;\n \tuint32_t i;\n+\tuint32_t loosened_objects_nr = 0;\n \tstruct object_id oid;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n@@ -3492,11 +3493,16 @@ static void loosen_unused_packed_objects(void)\n \t\t\tnth_packed_object_id(&oid, p, i);\n \t\t\tif (!packlist_find(&to_pack, &oid) &&\n \t\t\t !has_sha1_pack_kept_or_nonlocal(&oid) &&\n-\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime))\n+\t\t\t !loosened_object_can_be_discarded(&oid, p->mtime)) {\n \t\t\t\tif (force_object_loose(&oid, p->mtime))\n \t\t\t\t\tdie(_(\"unable to force loose object\"));\n+\t\t\t\tloosened_objects_nr++;\n+\t\t\t}\n \t\t}\n \t}\n+\n+\ttrace2_data_intmax(\"pack-objects\", the_repository,\n+\t\t\t \"loosen_unused_packed_objects/loosened\", loosened_objects_nr);\n }\n \n /*\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2847fdfbab..5f9bc74adc 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -20,7 +20,7 @@ static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n static int write_bitmaps = -1;\n static int use_delta_islands;\n-static char *packdir, *packtmp;\n+static char *packdir, *packtmp_name, *packtmp;\n \n static const char *const git_repack_usage[] = {\n \tN_(\"git repack []\"),\n@@ -530,7 +530,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n-\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n+\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n+\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n@@ -573,6 +574,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\trepack_promisor_objects(&po_args, &names);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n+\t\t\tfor_each_string_list_item(item, &names) {\n+\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n+\t\t\t\t\t packtmp_name, item->string);\n+\t\t\t}\n \t\t\tif (unpack_unreachable) {\n \t\t\t\tstrvec_pushf(&cmd.args,\n \t\t\t\t\t \"--unpack-unreachable=%s\",\ndiff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh\nindex ca785a3341..a965f2c4d6 100755\n--- a/t/perf/p5600-partial-clone.sh\n+++ b/t/perf/p5600-partial-clone.sh\n@@ -35,4 +35,8 @@ test_perf 'count non-promisor commits' '\n \tgit -C bare.git rev-list --all --count --exclude-promisor-objects\n '\n \n+test_perf 'gc' '\n+\tgit -C bare.git gc\n+'\n+\n test_done\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 5cb415386e..cf3e82bdf5 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -548,6 +548,14 @@ test_expect_success 'fetch from a partial clone, protocol v2' '\n \tgrep \"version 2\" trace\n '\n \n+test_expect_success 'repack does not loosen promisor objects' '\n+\trm -rf client trace &&\n+\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n+\ttest_when_finished \"rm -rf client trace\" &&\n+\tGIT_TRACE2_PERF=\"$(pwd)/trace\" git -C client repack -A -d &&\n+\tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.31.0.715.g22a2752fc5\n\n"}]}