{"thread":{"id":"66044","subject":"Performance regression in connectivity check during receive-pack (git 2.54)","startedAt":"2026-07-21T03:17:37Z","lastAt":"2026-08-04T06:19:53Z","messageCount":13,"participants":["Wolfgang Kritzinger","Jeff King","Taylor Blau","Junio C Hamano","Patrick Steinhardt","Justin Tobler"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"548712","messageId":"CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com","threadId":"66044","inReplyTo":null,"subject":"Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Wolfgang Kritzinger","fromEmail":"wkritzinger@atlassian.com","sentAt":"2026-07-21T03:17:23Z","receivedAt":"2026-07-21T03:17:37Z","isPatch":false,"body":"Hi!\n\nI'm a developer working on the on-premise version of Bitbucket at\nAtlassian.\n\nWe noticed pushes to Bitbucket got much slower after upgrading Git\non the server side from 2.50 to 2.54. I traced the slow part to\nthe connectivity check that receive-pack runs:\n\ngit rev-list --objects --stdin --not --exclude-hidden=receive --all \\\n--quiet --alternate-refs --progress=Checking connectivity\n\n`strace` shows that after a push, 2.54 does a failing open() of\nof numerous loose objects -- once in the quarantine (incoming)\ndirectory and once in the main object store -- before finding it\nin a pack:\n\nopenat(\".../objects/tmp_objdir-incoming-XXXX/ed/58..\", O_RDONLY) = ENOENT\nopenat(\".../objects/ed/58..\", O_RDONLY) = ENOENT\n\n2.50 does not do this. In most customer deployments of Bitbucket,\nthe Git data lives on an NFS share. The extra latency on NFS makes\nthis process of checking for non-existent loose objects take too\nlong, the push essentially hangs at the \"Checking connectivity\" step.\n\nI believe this new behavior was introduced in the recent object\ndatabase rework. After using bisect, I belive the problem can be\ntraced back to commit 8384cbcb4c.\n\nI don't know the codebase well, but from what I can see is that\nthe order in which objects are looked up in object databases\nchanged.\n\nAssuming there are two object databases configured (Main repo,\nand the quarantine directory, for example), the lookup order used\nto be:\n\n1. _quarantine dir_ packs\n2. _main dir_ packs\n3. _quarantine dir_ loose objects\n4. _main dir_ loose objects\n\nWith Git 2.54, the order appears to have changed to:\n\n1. _quarantine dir_ packs\n2. _quarantine dir_ loose objects\n3. _main dir_ packs\n4. _main dir_ loose objects\n\nIn my testing, within a well-packed repo, Git 2.50 actually never\nperformed a loose object lookup.\n\nThe current design seems to iterate over the configured object\ndatabases and perform a pack and loose object lookup for each.\n\nIs there a way to avoid these costly loose object lookups?\n"},{"id":"548713","messageId":"20260721035733.GA581473@coredump.intra.peff.net","threadId":"66044","inReplyTo":"CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-21T03:57:33Z","receivedAt":"2026-07-21T03:57:34Z","isPatch":false,"body":"On Tue, Jul 21, 2026 at 03:17:23PM +1200, Wolfgang Kritzinger wrote:\n\n> `strace` shows that after a push, 2.54 does a failing open() of\n> of numerous loose objects -- once in the quarantine (incoming)\n> directory and once in the main object store -- before finding it\n> in a pack:\n> \n> openat(\".../objects/tmp_objdir-incoming-XXXX/ed/58..\", O_RDONLY) = ENOENT\n> openat(\".../objects/ed/58..\", O_RDONLY) = ENOENT\n\nInteresting. Here's a smaller reproduction recipe that shows the issue:\n\n  # clone of git.git, or any other non-trivial repo; it should be mostly\n  # packed\n  src=/path/to/git\n\n  git init empty\n  export GIT_ALTERNATE_OBJECT_DIRECTORIES=$src/.git/objects\n  strace -fe openat \\\n    git -C empty rev-list --objects $(git -C $src rev-parse HEAD) >/dev/null\n\nIn v2.50, we see almost no loose object open calls, because we check the\npack first. But in v2.54, we see tons of them.\n\n> 2.50 does not do this. In most customer deployments of Bitbucket,\n> the Git data lives on an NFS share. The extra latency on NFS makes\n> this process of checking for non-existent loose objects take too\n> long, the push essentially hangs at the \"Checking connectivity\" step.\n\nYeah, I can imagine. But even on a fast filesystem, we definitely want\nto avoid all of those syscalls. Replacing \"strace\" above with a timing\nharness, even on a system with fast syscalls and a warm cache, the v2.54\nversion is ~12% slower.\n\n> I believe this new behavior was introduced in the recent object\n> database rework. After using bisect, I belive the problem can be\n> traced back to commit 8384cbcb4c.\n\nHmm, my bisect ended up at a593373b09 (packfile: refactor\n`find_pack_entry()` to work on the packfile store, 2026-01-09), which is\nnearby. I'm not sure if it might depend on other factors (e.g., presence\nof commit graphs, midx, etc) or my reproduction is not exactly like\nyours, or if one of us messed up bisection.\n\n+cc Patrick as the author of both commits.\n\n> I don't know the codebase well, but from what I can see is that\n> the order in which objects are looked up in object databases\n> changed.\n> \n> Assuming there are two object databases configured (Main repo,\n> and the quarantine directory, for example), the lookup order used\n> to be:\n> \n> 1. _quarantine dir_ packs\n> 2. _main dir_ packs\n> 3. _quarantine dir_ loose objects\n> 4. _main dir_ loose objects\n> \n> With Git 2.54, the order appears to have changed to:\n> \n> 1. _quarantine dir_ packs\n> 2. _quarantine dir_ loose objects\n> 3. _main dir_ packs\n> 4. _main dir_ loose objects\n> \n> In my testing, within a well-packed repo, Git 2.50 actually never\n> performed a loose object lookup.\n\nYeah, and that type of regression makes sense for what a593373b09 was\ntrying to do. But I think the v2.54 behavior is wrong. We should check\nall packs before any loose objects.\n\nI'm not sure of the correct fix. This is working against the whole \"odb\nsources are independent and abstract\" refactoring that a593373b09 was\ngoing for. But I think it's an important optimization. I guess the\nabstract version would be that each source has \"fast\" and \"slow\" lookups\nor something like that, and we check all fast ones before slow ones. But\nthat is pretty gross.\n\nI'll leave it to Patrick to ponder further. I haven't really been paying\na lot of attention to the odb refactoring.\n\n-Peff\n"},{"id":"548714","messageId":"al7-EaWaW3BYq4Nj@com-79390","threadId":"66044","inReplyTo":"20260721035733.GA581473@coredump.intra.peff.net","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-21T05:05:21Z","receivedAt":"2026-07-21T05:05:25Z","isPatch":false,"body":"On Mon, Jul 20, 2026 at 11:57:33PM -0400, Jeff King wrote:\n> On Tue, Jul 21, 2026 at 03:17:23PM +1200, Wolfgang Kritzinger wrote:\n>\n> > `strace` shows that after a push, 2.54 does a failing open() of\n> > of numerous loose objects -- once in the quarantine (incoming)\n> > directory and once in the main object store -- before finding it\n> > in a pack:\n> >\n> > openat(\".../objects/tmp_objdir-incoming-XXXX/ed/58..\", O_RDONLY) = ENOENT\n> > openat(\".../objects/ed/58..\", O_RDONLY) = ENOENT\n>\n> Interesting. Here's a smaller reproduction recipe that shows the issue:\n>\n>   # clone of git.git, or any other non-trivial repo; it should be mostly\n>   # packed\n>   src=/path/to/git\n>\n>   git init empty\n>   export GIT_ALTERNATE_OBJECT_DIRECTORIES=$src/.git/objects\n>   strace -fe openat \\\n>     git -C empty rev-list --objects $(git -C $src rev-parse HEAD) >/dev/null\n>\n> In v2.50, we see almost no loose object open calls, because we check the\n> pack first. But in v2.54, we see tons of them.\n>\n> > 2.50 does not do this. In most customer deployments of Bitbucket,\n> > the Git data lives on an NFS share. The extra latency on NFS makes\n> > this process of checking for non-existent loose objects take too\n> > long, the push essentially hangs at the \"Checking connectivity\" step.\n>\n> Yeah, I can imagine. But even on a fast filesystem, we definitely want\n> to avoid all of those syscalls. Replacing \"strace\" above with a timing\n> harness, even on a system with fast syscalls and a warm cache, the v2.54\n> version is ~12% slower.\n>\n> > I believe this new behavior was introduced in the recent object\n> > database rework. After using bisect, I belive the problem can be\n> > traced back to commit 8384cbcb4c.\n\n\n> Hmm, my bisect ended up at a593373b09 (packfile: refactor\n> `find_pack_entry()` to work on the packfile store, 2026-01-09), which is\n> nearby. I'm not sure if it might depend on other factors (e.g., presence\n> of commit graphs, midx, etc) or my reproduction is not exactly like\n> yours, or if one of us messed up bisection.\n>\n> +cc Patrick as the author of both commits.\n\nI think that both bisection results are equally valid for different\nreasons.\n\nBefore 8384cbcb4c, 'find_pack_entry()' did\n\n    packfile_store_prepare(r->objects->sources->packfiles);\n\n, then tried each of the stores in order to first see if (1) a MIDX was\navailable to locate the object in some pack, or (2) failing that, if\nthere exists some non-MIDX'd pack which could do the same.\n\nWorth noting is that 'packfile_store_prepare()' effectively did:\n\n    for (s = store->source->odb->sources; s; s = s->next) {\n        prepare_multi_pack_index_one(s);\n        prepare_packed_git_one(s);\n    }\n\nThus preparing the first store also prepared every alternate store,\nenabling 'find_pack_entry()' to search through all store's MIDX and\npack lists/sources.\n\n8384cbcb4c changes this such that 'packfile_store_prepare()' now only\nprepares its owning source:\n\n    prepare_multi_pack_index_one(store->source);\n    prepare_packed_git_one(store->source);\n\n, which is reasonable, but 'find_pack_entry()' still loops over all\nsources starting from 'r->objects->sources' and calls the function\n'packfile_store_prepare()'. But! It calls that function over the same\nargument each time, like so:\n\n    for (source = r->objects->sources; source; source = source->next) {\n        packfile_store_prepare(r->objects->sources->packfiles);\n        if (source->midx && fill_midx_entry(source->midx, oid, e))\n            return 1;\n    }\n\nSo we never prepare the packfile store from other sources!\n\nIn Wolfgang's case, if we have an quarantine store followed by the main\nobject store, our lookup order will be:\n\n 1. prepare the quarantine object store\n 2. search packs in the quarantine object store\n 3. search packs in the main object store (which will fail, since this\n    list is guaranteed to be empty since we never called\n    'packfile_store_prepare()')\n 4. search loose objects in the quarantine object store\n 5. search loose objects in the main object store\n 6. haven't found anything, so we must reprepare\n 7. search packs in the main object store, which will now succeed, as\n    the previous reprepare called 'packfile_store_prepare()' on the main\n    object store's packfile source.\n\nCommit a593373b09 changes things, since it makes 'find_pack_entry()' no\nlonger operate over the entire repository, but over a single store.\nBefore searching that store, it prepares it, like so:\n\n    static int find_pack_entry(struct packfile_store *store,\n                               const struct object_id *oid,\n                               sturct pack_entry *e)\n    {\n        struct packfile_list_entry *l;\n\n        packfile_store_prepare(store);\n        if (store->source->midx && fill_midx_entry(...))\n            return 1;\n\n        for (l = store->packs.head; l; l = l->next) {\n            struct packed_git *p = l->pack;\n            if (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {\n                /* ... */\n                return 1;\n            }\n        }\n\n        return 0;\n    }\n\nSo commit a593373b09 indeed squashes the bug introduced by 8384cbcb4c,\nand when lookup reaches the main store, it prepares the main store\ncorrectly.\n\nBut a593373b09 also changes the lookup order, because the caller in\n'do_oid_object_info_extended()` already loops over sources!\n\n    static int do_oid_object_info_extended(struct object_database *odb,\n                                           const struct object_id *oid,\n                                           struct object_info *oi, unsigned flags)\n    {\n        /* replace objects, cached lookups, etc., ... */\n\n        odb_prepare_alterantes(odb);\n\n        while (1) {\n            struct odb_source *source;\n\n            for (source = odb->sources; source; source = source->next) {\n                if (!packfile_store_read_object_info(source->packfiles,\n                                                     real, oi, flags) ||\n                    !odb_source_loose_read_object_info(source, real, oi,\n                                                       flags))\n                    return 0;\n            }\n        }\n    }\n\nBefore a593373b09, that call to 'packfile_store_read_object_info()'\nlooped over all sources, since it still called 'find_pack_entry()'.\n\nIn other words, prior to a593373b09, the lookup proceeded like so:\n\n 1. search packfiles in quarantine\n 2. search packfiles in the main object store\n 3. search loose objects in quarantine\n 4. search loose objects in the main object store\n\nBut a593373b09 changes that to instead proceed store-by-store, as\nfollows:\n\n 1. search packfiles in quarantine\n 2. search loose objects in quarantine\n 3. search packfiles in the main object store\n 4. search loose objects in the main object store\n\nSo even with all object sources prepared, every object found in a later\nsource pays a failed loose object lookup in an earlier one, which I\nbelieve matches what Peff strace'd above.\n\nSo both bisections make sense. If the later store has not been prepared\nyet, commit 8384cbcb4c is where Git first fails to see its packs and\nfalls through to loose object checks. If the stores are already\nprepared, that problem does not show up, and a593373b09 is where Git\nfirst starts checking loose objects in an earlier source before looking\nin a later source's packs.\n\nI think that something like the following (untested) would fix the\nimmediate issue:\n\n--- 8< ---\ndiff --git a/odb.c b/odb.c\nindex cf6e7938c0..aeb2915f0f 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -568,9 +568,28 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \twhile (1) {\n \t\tstruct odb_source *source;\n\n-\t\tfor (source = odb->sources; source; source = source->next)\n-\t\t\tif (!odb_source_read_object_info(source, real, oi, flags))\n+\t\t/*\n+\t\t * Check all packed sources before trying loose ones. A loose\n+\t\t * miss requires a filesystem lookup, and receive-pack's\n+\t\t * quarantine source makes the main object directory an\n+\t\t * alternate.\n+\t\t */\n+\t\tfor (source = odb->sources; source; source = source->next) {\n+\t\t\tstruct odb_source_files *files =\n+\t\t\t\todb_source_files_downcast(source);\n+\n+\t\t\tif (!odb_source_read_object_info(&files->packed->base,\n+\t\t\t\t\t\t\t real, oi, flags))\n \t\t\t\treturn 0;\n+\t\t}\n+\t\tfor (source = odb->sources; source; source = source->next) {\n+\t\t\tstruct odb_source_files *files =\n+\t\t\t\todb_source_files_downcast(source);\n+\n+\t\t\tif (!odb_source_read_object_info(&files->loose->base,\n+\t\t\t\t\t\t\t real, oi, flags))\n+\t\t\t\treturn 0;\n+\t\t}\n\n \t\t/*\n \t\t * When the object hasn't been found we try a second read and\n--- >8 ---\n\nBut...\n\n> I'm not sure of the correct fix. This is working against the whole \"odb\n> sources are independent and abstract\" refactoring that a593373b09 was\n> going for. But I think it's an important optimization. I guess the\n> abstract version would be that each source has \"fast\" and \"slow\" lookups\n> or something like that, and we check all fast ones before slow ones. But\n> that is pretty gross.\n\n...that fix is breaking the very abstraction that the pluggable-ODB\neffort is trying to create in the first place, at least in my\nunderstanding of the project's goals.\n\n> I'll leave it to Patrick to ponder further. I haven't really been paying\n> a lot of attention to the odb refactoring.\n\nI am genuinely not sure what the right path forward here is, given that\nI do not have a super firm understanding of all of the refactoring that\nhas taken place here. I would be likewise eager to hear from Patrick or\nothers with thoughts on how to resolve this.\n\nThanks,\nTaylor\n"},{"id":"548725","messageId":"xmqqtsps76f1.fsf@gitster.g","threadId":"66044","inReplyTo":"20260721035733.GA581473@coredump.intra.peff.net","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-21T14:40:02Z","receivedAt":"2026-07-21T14:40:07Z","isPatch":false,"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, and that type of regression makes sense for what a593373b09 was\n> trying to do. But I think the v2.54 behavior is wrong. We should check\n> all packs before any loose objects.\n>\n> I'm not sure of the correct fix. This is working against the whole \"odb\n> sources are independent and abstract\" refactoring that a593373b09 was\n> going for. But I think it's an important optimization. I guess the\n> abstract version would be that each source has \"fast\" and \"slow\" lookups\n> or something like that, and we check all fast ones before slow ones. But\n> that is pretty gross.\n>\n> I'll leave it to Patrick to ponder further. I haven't really been paying\n> a lot of attention to the odb refactoring.\n\nI think checking the fast sources before the slow ones is probably\nthe best we can do if we want to retain the 'each odb source is an\nopaque object' abstraction.\n\nStepping back a bit, the 'rev-list' command used for the\nconnectivity check is curious in multiple aspects.\n\n * On the surface, it looks as if the caller wants an enumeration of\n   all objects that appear in the range.  However, the caller is not\n   interested in the actual list of objects.  Instead, they are\n   interested only in a single bit: whether the traversal succeeds\n   or dies due to a missing object.  This is because the traversal\n   determines whether we need to fetch, or whether we are already up\n   to date, to decide whether the proposed 'fetch' is a no-op.  The\n   positive ends of the traversal represent what we are about to\n   fetch; if we already have all the objects needed to reach those\n   tips in our repository, we can do without actually downloading\n   anything [*].\n\n * A false positive answer to the question \"does the traversal die\n   due to a missing object?\" does not affect correctness, as this is\n   merely an optimization to save downloads (though a false negative\n   is unacceptable).\n\nGiven this non-standard use of the command, we can pass\napplication-specific cues (such as \"we are doing this traversal for\na connectivity check\") down to the machinery as a hint to help it\noptimize its operation, and I suspect that such a hint might have\nvalue.\n\nFor example, we could enumerate all loose objects in the loose\nobject store using 256 opendir() and readdir() calls for about\n10,000 files (since once you have more than 6,700 loose objects,\nauto-gc would pack them) and store them in an in-core table [**].\nThis would enable us to say \"the object with that name does not\nexist here\" without running lstat() at all.  I wonder how many\nlstat() calls we would need to save for such a scheme to pay off.\n\nThere may be other highly application-specific optimization\nopportunities, as utilizing revision traversal for\nconnectivity checking has peculiar correctness requirements\nthat differ from the normal use of the API.\n\n[Footnote]\n\n * It follows that in a lazily cloned repository with promisor\n   remotes, the traversal could download everything needed as it\n   goes, only to conclude: \"No need for the main fetch; we have\n   everything we need.\"  I would expect this to be a fairly slow\n   process that defeats the entire reason we have this connectivity\n   check up front as an optimization.  While I have not checked, I\n   believe the actual code prevents this either by skipping the\n   connectivity check altogether, or by instructing the connectivity\n   checker to treat promised (but not immediately available) objects\n   as missing and abort.  But my point is that theoretically one\n   does not even need 'git fetch' in a lazily cloned repository.  It\n   is sufficient to use 'git ls-remote' to determine the tips of\n   remote refs, and run 'rev-list' to fill the range.\n\n** If in-core memory pressure is a concern, we could use a Bloom\n   filter, as we only need to know \"the object is definitely not\n   here\" and can tolerate \"that object might be here, but we are not\n   certain.\"\n"},{"id":"548773","messageId":"amCuLpT6vYzo1GF8@pks.im","threadId":"66044","inReplyTo":"xmqqtsps76f1.fsf@gitster.g","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-22T11:49:02Z","receivedAt":"2026-07-22T11:49:15Z","isPatch":false,"body":"On Tue, Jul 21, 2026 at 07:40:02AM -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > Yeah, and that type of regression makes sense for what a593373b09 was\n> > trying to do. But I think the v2.54 behavior is wrong. We should check\n> > all packs before any loose objects.\n> >\n> > I'm not sure of the correct fix. This is working against the whole \"odb\n> > sources are independent and abstract\" refactoring that a593373b09 was\n> > going for. But I think it's an important optimization. I guess the\n> > abstract version would be that each source has \"fast\" and \"slow\" lookups\n> > or something like that, and we check all fast ones before slow ones. But\n> > that is pretty gross.\n> >\n> > I'll leave it to Patrick to ponder further. I haven't really been paying\n> > a lot of attention to the odb refactoring.\n> \n> I think checking the fast sources before the slow ones is probably\n> the best we can do if we want to retain the 'each odb source is an\n> opaque object' abstraction.\n\nSeeing that this is about the `tmp_objdir` case: one of the things that\nJustin and I wanted to work on anyway is that we want to stop modifying\nthe list of sources during transactions in the first place. It always\nfelt kind of gross that we're modifying the sources when creating a\ntransaction, as the only reason that we do this for is so that the\nwrites actually go to the temporary object directory instead of to the\nprimary object source. And that doesn't make a lot of sense to begin\nwith.\n\nThe alternative to this would be to instead have logic in functions like\n`odb_write()` that checks whether we have an active transaction or not.\nIf so, the write would go into the transaction directly instead of going\ninto the primary source, and consequently we wouldn't even have to\nmodify the list of sources at all.\n\nThis shouldn't create too much of a problem, as we typically don't\nintend to even read objects that we've written into the transaction\nimmediately. It would avoid that we try to read objects from the\ntemporary object directory. And it would also allow us to eventually\nmove all the logic to write objects into the transactions exclusively.\n\nI'm currently out of office though, and will be on vacation next week.\nI'll explore this area a bit more though once I'm back in office in two\nweeks.\n\nPatrick\n"},{"id":"548786","messageId":"xmqqh5lrrplt.fsf@gitster.g","threadId":"66044","inReplyTo":"amCuLpT6vYzo1GF8@pks.im","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-22T15:49:50Z","receivedAt":"2026-07-22T15:49:53Z","isPatch":false,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The alternative to this would be to instead have logic in functions like\n> `odb_write()` that checks whether we have an active transaction or not.\n> If so, the write would go into the transaction directly instead of going\n> into the primary source, and consequently we wouldn't even have to\n> modify the list of sources at all.\n>\n> This shouldn't create too much of a problem, as we typically don't\n> intend to even read objects that we've written into the transaction\n> immediately. It would avoid that we try to read objects from the\n> temporary object directory. And it would also allow us to eventually\n> move all the logic to write objects into the transactions exclusively.\n\nI suspect several of those 'transactions' are actually misspelt\n'temporary directories', but I catch your drift.  That said, a\nredesign like that feels more or less independent of the fix for our\nimmediate performance regression.\n\nAfter all, didn't Peff show us a case where no odb sources were\nbeing flipped in the middle?  Simply setting up one object store to\nborrow from another via the alternates mechanism demonstrated that\nchecking packs across all object stores before hunting for loose\nobjects in any of them makes a world of difference.\n\n> I'm currently out of office though, and will be on vacation next week.\n> I'll explore this area a bit more though once I'm back in office in two\n> weeks.\n\nUnderstood.  Bon voyage and have fun!\n\n"},{"id":"548806","messageId":"20260723103911.GA604358@coredump.intra.peff.net","threadId":"66044","inReplyTo":"xmqqtsps76f1.fsf@gitster.g","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-23T10:39:11Z","receivedAt":"2026-07-23T10:39:19Z","isPatch":false,"body":"On Tue, Jul 21, 2026 at 07:40:02AM -0700, Junio C Hamano wrote:\n\n> Stepping back a bit, the 'rev-list' command used for the\n> connectivity check is curious in multiple aspects.\n> \n>  * On the surface, it looks as if the caller wants an enumeration of\n>    all objects that appear in the range.  However, the caller is not\n>    interested in the actual list of objects.  Instead, they are\n>    interested only in a single bit: whether the traversal succeeds\n>    or dies due to a missing object.  This is because the traversal\n>    determines whether we need to fetch, or whether we are already up\n>    to date, to decide whether the proposed 'fetch' is a no-op.  The\n>    positive ends of the traversal represent what we are about to\n>    fetch; if we already have all the objects needed to reach those\n>    tips in our repository, we can do without actually downloading\n>    anything [*].\n> \n>  * A false positive answer to the question \"does the traversal die\n>    due to a missing object?\" does not affect correctness, as this is\n>    merely an optimization to save downloads (though a false negative\n>    is unacceptable).\n> \n> Given this non-standard use of the command, we can pass\n> application-specific cues (such as \"we are doing this traversal for\n> a connectivity check\") down to the machinery as a hint to help it\n> optimize its operation, and I suspect that such a hint might have\n> value.\n\nYeah, I think there may be some interesting opportunities for\noptimization in check_connected(). But as you noted, it is sometimes\nused for fetch asking \"do we probably have all of these objects\" but\nalso for strict connectivity checks for incoming objects. The quarantine\narea triggers only for the latter (in this case receive-pack), so it\nwould not really help here.\n\nIOW, I'd consider it a mostly orthogonal possible direction for\noptimization (but still a potentially interesting one).\n\n-Peff\n"},{"id":"548807","messageId":"20260723104625.GB604358@coredump.intra.peff.net","threadId":"66044","inReplyTo":"amCuLpT6vYzo1GF8@pks.im","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-23T10:46:25Z","receivedAt":"2026-07-23T10:46:26Z","isPatch":false,"body":"On Wed, Jul 22, 2026 at 01:49:02PM +0200, Patrick Steinhardt wrote:\n\n> Seeing that this is about the `tmp_objdir` case: one of the things that\n> Justin and I wanted to work on anyway is that we want to stop modifying\n> the list of sources during transactions in the first place. It always\n> felt kind of gross that we're modifying the sources when creating a\n> transaction, as the only reason that we do this for is so that the\n> writes actually go to the temporary object directory instead of to the\n> primary object source. And that doesn't make a lot of sense to begin\n> with.\n> \n> The alternative to this would be to instead have logic in functions like\n> `odb_write()` that checks whether we have an active transaction or not.\n> If so, the write would go into the transaction directly instead of going\n> into the primary source, and consequently we wouldn't even have to\n> modify the list of sources at all.\n\nYes, the swapping of the \"regular\" and \"alternate\" odbs for the\nquarantine transaction is kind of hacky. But I don't think this is\nsomething you can solve just via the odb API. The notion of which\nsources to read/write from crosses process boundaries. In particular:\n\n  1. We write using a separate index-pack process. It has to be told to\n     write into the transaction area, not the regular odb.\n\n  2. After receiving objects, we _do_ read them in order to do quality\n     checks before admitting them to the repository. The connectivity\n     check discussed here is one example. That happens in a separate\n     rev-list process, though it in theory could be moved in-process.\n\n     But we also run user-specified hooks, which may run arbitrary Git\n     commands. Those hooks need to be given an environment where they\n     can transparently read from both the quarantine area and the\n     original odb.\n\nSo you'll have to communicate between processes both \"write here, not\nthere\" and \"look at both here and there to read objects\". And I suspect\nthe result is going to look a lot like the alternates juggling we are\ndoing today.\n\n-Peff\n"},{"id":"548808","messageId":"20260723104943.GC604358@coredump.intra.peff.net","threadId":"66044","inReplyTo":"xmqqh5lrrplt.fsf@gitster.g","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-23T10:49:43Z","receivedAt":"2026-07-23T10:49:44Z","isPatch":false,"body":"On Wed, Jul 22, 2026 at 08:49:50AM -0700, Junio C Hamano wrote:\n\n> I suspect several of those 'transactions' are actually misspelt\n> 'temporary directories', but I catch your drift.  That said, a\n> redesign like that feels more or less independent of the fix for our\n> immediate performance regression.\n> \n> After all, didn't Peff show us a case where no odb sources were\n> being flipped in the middle?  Simply setting up one object store to\n> borrow from another via the alternates mechanism demonstrated that\n> checking packs across all object stores before hunting for loose\n> objects in any of them makes a world of difference.\n\nYeah, exactly. This is really a regression in alternates performance,\nbut it just so happens that the quarantine system is built on top of\nalternates so we noticed it there.\n\nI'd expect \"clone -s / --reference\" to have similar problems, and also\nfor sites like GitHub and GitLab that make heavy use of alternates for\nobject sharing between forks. And those would pay the penalty on just\nabout every operation (because we'd expect the alternate to be holding\nmost of the objects in those cases).\n\n-Peff\n"},{"id":"548852","messageId":"amLgMqkqxR8mKIbT@pks.im","threadId":"66044","inReplyTo":"20260723104943.GC604358@coredump.intra.peff.net","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-24T03:46:58Z","receivedAt":"2026-07-24T03:47:08Z","isPatch":false,"body":"On Thu, Jul 23, 2026 at 06:49:43AM -0400, Jeff King wrote:\n> On Wed, Jul 22, 2026 at 08:49:50AM -0700, Junio C Hamano wrote:\n> \n> > I suspect several of those 'transactions' are actually misspelt\n> > 'temporary directories', but I catch your drift.  That said, a\n> > redesign like that feels more or less independent of the fix for our\n> > immediate performance regression.\n> > \n> > After all, didn't Peff show us a case where no odb sources were\n> > being flipped in the middle?  Simply setting up one object store to\n> > borrow from another via the alternates mechanism demonstrated that\n> > checking packs across all object stores before hunting for loose\n> > objects in any of them makes a world of difference.\n> \n> Yeah, exactly. This is really a regression in alternates performance,\n> but it just so happens that the quarantine system is built on top of\n> alternates so we noticed it there.\n> \n> I'd expect \"clone -s / --reference\" to have similar problems, and also\n> for sites like GitHub and GitLab that make heavy use of alternates for\n> object sharing between forks. And those would pay the penalty on just\n> about every operation (because we'd expect the alternate to be holding\n> most of the objects in those cases).\n\nYou're right, I also realized that after sending my mail.\n\nOne other angle that Justin and I have been discussing (we were at an\noffsite together over the last couple days) was that we can do a small\ncourse correction: instead of handling alternates on the ODB level, we\nmay be able to start treating alternates as an implementation detail of\nit. So both the handling of alternates, but also the handling of the\nGIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES environment\nvariables would be moved into the \"files\" backend itself.\n\nThis would solve a bunch of smaller issues that we're currently\ngrappling with where some of the concepts in Git really want to operate\nacross all of the alternates:\n\n  - The OBJECT_INFO_SECOND_READ flag can be dropped as it becomes an\n    implementation detail.\n\n  - We can fix the performance regression because we can now easily\n    reorder access to read via packfiles first across all sub-sources.\n\n  - Commit graphs and bitmap really are a singleton, so loading them via\n    multiple sources is awkward.\n\n  - The object storage extension that I've written got quite a bit ugly\n    as it wasn't quite clear where exactly to draw the line. Especially\n    hadnling the environment variables mentioned above into the \"files\"\n    backend removes one point of friction I encountered.\n\n  - Object database maintenance needs to be aware of the other non-local\n    sources.\n\nAlso, doing that change isn't as bad as it may sound at first. We'd\nstill retain the whole `struct odb_source` list because we want to have\nthem for submodule sources. Furthermore, alternates aren't required for\nisolation either as we currently use them via the temporary object\ndirectory. An alternative implementation may use a completely separate\nmechanism to achieve write isolation, which is also why we have made the\nenvironment variables pluggable that the `struct odb_transaction` ends\nup passing to the child process.\n\nI think overall this could simplify some of the design, and it makes a\nbunch of issues that I have been struggling with go away. The devil may\nbe in the details of course, but I think transitioning towards this\nshould be doable.\n\nSo I'll work towards that goal. I've got a patch series already that\nremoves all of our calls to `odb_prepare_alternates()` as a first step,\nbut I'll only send that in two weeks once I'm back in office. I'll then\nhave a look at how bad the subsequent steps would be.\n\nThanks!\n\nPatrick\n"},{"id":"548995","messageId":"20260726085117.GA3529599@coredump.intra.peff.net","threadId":"66044","inReplyTo":"amLgMqkqxR8mKIbT@pks.im","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-26T08:51:17Z","receivedAt":"2026-07-26T08:51:18Z","isPatch":false,"body":"On Fri, Jul 24, 2026 at 05:46:58AM +0200, Patrick Steinhardt wrote:\n\n> One other angle that Justin and I have been discussing (we were at an\n> offsite together over the last couple days) was that we can do a small\n> course correction: instead of handling alternates on the ODB level, we\n> may be able to start treating alternates as an implementation detail of\n> it. So both the handling of alternates, but also the handling of the\n> GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES environment\n> variables would be moved into the \"files\" backend itself.\n\nThat seems reasonable to me. In theory it could lose some flexibility if\nsome code really wanted to represent alternates as abstract sources, but\nI can't think of why you'd want to do so. Traditionally we did not even\nreally have separate sources at all, and the point of adding them was\nnot so much to have arbitrary combinations of sources as to abstract the\ndetails.\n\nI guess somebody could want to use a local alternates object-dir along\nwith their sql database of objects or whatever. But even then, it seems\nlike the alternate could be added as its own files-backend odb source.\n\n> This would solve a bunch of smaller issues that we're currently\n> grappling with where some of the concepts in Git really want to operate\n> across all of the alternates:\n> [...]\n\nYeah, that all sounds like a positive direction. Thanks!\n\n-Peff\n"},{"id":"549090","messageId":"amd4yR3EEn_fVZcm@denethor","threadId":"66044","inReplyTo":"amLgMqkqxR8mKIbT@pks.im","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-07-27T15:45:46Z","receivedAt":"2026-07-27T15:45:51Z","isPatch":false,"body":"On 26/07/24 05:46AM, Patrick Steinhardt wrote:\n> This would solve a bunch of smaller issues that we're currently\n> grappling with where some of the concepts in Git really want to operate\n> across all of the alternates:\n> \n>   - The OBJECT_INFO_SECOND_READ flag can be dropped as it becomes an\n>     implementation detail.\n> \n>   - We can fix the performance regression because we can now easily\n>     reorder access to read via packfiles first across all sub-sources.\n\nLetting the backend control the ordering would be a nice property.\n\n>   - Commit graphs and bitmap really are a singleton, so loading them via\n>     multiple sources is awkward.\n\nI completely agree. Having the ODB source be more self-contained with\nthe alternates better fits the shape of commits graphs and alternates\nIMO.\n\n>   - The object storage extension that I've written got quite a bit ugly\n>     as it wasn't quite clear where exactly to draw the line. Especially\n>     hadnling the environment variables mentioned above into the \"files\"\n>     backend removes one point of friction I encountered.\n> \n>   - Object database maintenance needs to be aware of the other non-local\n>     sources.\n> \n> Also, doing that change isn't as bad as it may sound at first. We'd\n> still retain the whole `struct odb_source` list because we want to have\n> them for submodule sources. Furthermore, alternates aren't required for\n> isolation either as we currently use them via the temporary object\n> directory. An alternative implementation may use a completely separate\n> mechanism to achieve write isolation, which is also why we have made the\n> environment variables pluggable that the `struct odb_transaction` ends\n> up passing to the child process.\n\nOnce all temporary object directory users are updated to use ODB\ntransactions, we could stop reording source list when starting/ending a\ntransaction. Instead the transaction could be tracked separately\ninternally and during ODB read/writes the transaction could be directly\nused as needed. This is something I plan to tackle in a future series\nsoon.\n\n> I think overall this could simplify some of the design, and it makes a\n> bunch of issues that I have been struggling with go away. The devil may\n> be in the details of course, but I think transitioning towards this\n> should be doable.\n\nI am certainly a fan of this direction. We do lose some flexibility in\nterms of supporting alternates more generically, but I'm not sure\nsupporting alternates of different source types in the same repo would\nbe something we want anyways in practice due to the additional\ncomplexity.\n\n-Justin\n"},{"id":"549519","messageId":"anGEgUrzzQYzEK_K@pks.im","threadId":"66044","inReplyTo":"amd4yR3EEn_fVZcm@denethor","subject":"Re: Performance regression in connectivity check during receive-pack (git 2.54)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-04T06:19:45Z","receivedAt":"2026-08-04T06:19:53Z","isPatch":false,"body":"On Mon, Jul 27, 2026 at 10:45:46AM -0500, Justin Tobler wrote:\n> On 26/07/24 05:46AM, Patrick Steinhardt wrote:\n[snip]\n> > I think overall this could simplify some of the design, and it makes a\n> > bunch of issues that I have been struggling with go away. The devil may\n> > be in the details of course, but I think transitioning towards this\n> > should be doable.\n> \n> I am certainly a fan of this direction. We do lose some flexibility in\n> terms of supporting alternates more generically, but I'm not sure\n> supporting alternates of different source types in the same repo would\n> be something we want anyways in practice due to the additional\n> complexity.\n\nTrue. But if we ever find that we actually want that flexibility we\ndon't paint ourselves into a corner, either, as it is rather trivial to\nintroduce another source type that allows us to mix and match different\nsources.\n\nPatrick\n"}]}