packfile: fix perf regression with many packs - #2202
Open
dscho wants to merge 1 commit into
Open
Conversation
Member
Author
|
/submit |
|
Submitted as pull.2202.git.1786561870638.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> In one reported use case (https://github.com/microsoft/git/issues/970),
> N equals 37,815 and caused a slow-down of a simple `git rev-parse
> --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> increased from under 2 minutes to over half an hour.
Face with Rolling Eyes (1f644) 🙄
As we grow older, more and more extreme use cases that we initially
thought were simply crazy become reality.
> Let's fix this by establishing a fast path for known-new packfiles.
As long as the caller reliably knows that the pack it has is new and
cannot be on the list, there is no reason to cycle through all the
packs in the ring to attempt removing it in vain.
Clever and clean.
> diff --git a/packfile.c b/packfile.c
> index 0eee45055f..f80f05a1fe 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -781,7 +781,7 @@ void packfile_store_add_pack(struct odb_source_packed *store,
> if (pack->pack_fd != -1)
> pack_open_fds++;
>
> - packfile_list_append(&store->packs, pack);
> + packfile_list_append(&store->packs, pack, 1);
> strmap_put(&store->packs_by_path, pack->pack_name, pack);
> }
>
> diff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh
> index af173a7b73..4221f9dd70 100755
> --- a/t/perf/p5303-many-packs.sh
> +++ b/t/perf/p5303-many-packs.sh
> @@ -141,4 +141,8 @@ test_perf "load 10,000 packs" '
> git rev-parse --verify "HEAD^{commit}"
> '
>
> +test_perf "abbreviate with 10,000 packs" '
> + git rev-parse --short HEAD
> +'
> +
> test_done
>
> base-commit: 11c6700f10234578d10523faf35656ca491425c9 |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Wed, Aug 12, 2026 at 12:51:30PM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > In one reported use case (https://github.com/microsoft/git/issues/970),
> > N equals 37,815 and caused a slow-down of a simple `git rev-parse
> > --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> > 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> > increased from under 2 minutes to over half an hour.
>
> Face with Rolling Eyes (1f644) 🙄
>
> As we grow older, more and more extreme use cases that we initially
> thought were simply crazy become reality.
Sort of. The quadratic adding became a problem long ago, hence
ec48540fe8 (packfile.c: speed up loading lots of packfiles, 2019-11-27).
So this was something we already dealt with that regressed. We can even
see the regression in our perf suite:
$ GIT_SKIP_TESTS='p5303.[1-9] p5303.1[0-9]' ./run 589127caa730^ 589127caa730 p5303-many-packs.sh
Test 589127caa730^ 589127caa730
----------------------------------------------------------------------
5303.21: load 10,000 packs 0.13(0.11+0.02) 0.45(0.42+0.02) +246.2%
Unfortunately I don't think anybody pays close attention to the perf
suite (partially because it's clunky and expensive to run, but also
because it often requires human judgement to decide when something is a
real change and not just a blip).
None of that has any bearing on the fix, which seems reasonable to me,
but...
> > --- a/t/perf/p5303-many-packs.sh
> > +++ b/t/perf/p5303-many-packs.sh
> > @@ -141,4 +141,8 @@ test_perf "load 10,000 packs" '
> > git rev-parse --verify "HEAD^{commit}"
> > '
> >
> > +test_perf "abbreviate with 10,000 packs" '
> > + git rev-parse --short HEAD
> > +'
...I wonder what value this is adding. It shows the same slowdown as the
existing test you can see in the context (and whose results I showed
above).
-Peff |
|
User |
|
Ben Knoble wrote on the Git mailing list (how to reply to this email): > Le 12 août 2026 à 15:15, Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> a écrit :
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Since 589127caa730 (packfile: move list of packs into the packfile
> store, 2025-10-30), there is a performance regression when many
> packfiles need to be loaded: `packfile_store_add_pack()` now calls
> `packfile_list_remove_internal()` to detect whether the packfile was
> _already_ in the list, if if so, move it to the end of the list. This
> function linearly scans the existing list before every insertion. Newly
> loading N packs therefore has complexity O(N²).
>
> In one reported use case (https://github.com/microsoft/git/issues/970),
> N equals 37,815 and caused a slow-down of a simple `git rev-parse
> --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> increased from under 2 minutes to over half an hour.
>
> Let's fix this by establishing a fast path for known-new packfiles.
>
> The keen reader will note that there is currently only a single,
> "known-new" caller of the `packfile_list_append()` function, and wonder
> why not simply remove this check whether the packfile already exists in
> the list? Originally, when above-mentioned commit introduced that logic,
> there was a second caller in `prepare_midx()`, which would have required
> that check, but that caller was removed in 6aff1f25a046 (packfile:
> always add packfiles to MRU when adding a pack, 2025-10-30). Still, the
> function is declared in a header file, and to avoid any problems with
> in-flight or downstream callers, it is safer to extend the signature to
> be explicit whether or not to skip that check.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> packfile: fix perf regression with many packs
>
> This issue was spotted by a Microsoft Git user with the massive amount
> of packfiles typical of an average, long-running monorepo checkout.
As a different kind of intermediate solution, would turning on maintenance for that user’s checkout help? (Not sure that would help CI clone times unless the server repacks, of course.) |
|
User |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Aug 12, 2026 at 07:11:09PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Since 589127caa730 (packfile: move list of packs into the packfile
> store, 2025-10-30), there is a performance regression when many
> packfiles need to be loaded: `packfile_store_add_pack()` now calls
> `packfile_list_remove_internal()` to detect whether the packfile was
> _already_ in the list, if if so, move it to the end of the list. This
Nit: s/if if/and if/
> function linearly scans the existing list before every insertion. Newly
> loading N packs therefore has complexity O(N²).
>
> In one reported use case (https://github.com/microsoft/git/issues/970),
> N equals 37,815 and caused a slow-down of a simple `git rev-parse
> --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> increased from under 2 minutes to over half an hour.
Wow, 38k packfiles is a lot.
> Let's fix this by establishing a fast path for known-new packfiles.
>
> The keen reader will note that there is currently only a single,
> "known-new" caller of the `packfile_list_append()` function, and wonder
> why not simply remove this check whether the packfile already exists in
> the list? Originally, when above-mentioned commit introduced that logic,
> there was a second caller in `prepare_midx()`, which would have required
> that check, but that caller was removed in 6aff1f25a046 (packfile:
> always add packfiles to MRU when adding a pack, 2025-10-30). Still, the
> function is declared in a header file, and to avoid any problems with
> in-flight or downstream callers, it is safer to extend the signature to
> be explicit whether or not to skip that check.
Quite conservative, but fair enough.
> diff --git a/packfile-list.c b/packfile-list.c
> index 01fb913abf..1379ab3a4f 100644
> --- a/packfile-list.c
> +++ b/packfile-list.c
> @@ -57,11 +57,12 @@ void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack)
> list->tail = entry;
> }
>
> -void packfile_list_append(struct packfile_list *list, struct packed_git *pack)
> +void packfile_list_append(struct packfile_list *list, struct packed_git *pack,
> + int is_new)
> {
> struct packfile_list_entry *entry;
>
> - entry = packfile_list_remove_internal(list, pack);
> + entry = is_new ? NULL : packfile_list_remove_internal(list, pack);
> if (!entry) {
> entry = xmalloc(sizeof(*entry));
> entry->pack = pack;
I wonder whether we should slightly reformulate this and rename `is_new`
to `accept_duplicates`. Because ultimately, that is what we're doing
now: instead of ensuring that the packfile is unique in the list, we
just don't care and just append the entry to the list.
An alternative would be to use a hashmap here that tracks the packs that
have already been added. It has the advantage that it also covers the
`prepend()` operation and that callers don't have to be aware of this
mechanism at all. Furthermore, moving preexisting entries to the back or
front could become O(logn) if the list was doubly-linked. We do this
operation quite often to re-sort entries in the list when looking up
objects.
Overall though I'm not quite sure whether the added complexity would be
worth it, see below patch.
Thanks!
Patrick
diff --git a/http-push.c b/http-push.c
index 94a1fac9ab..52b00e7c95 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1729,6 +1729,7 @@ int cmd_main(int argc, const char **argv)
const char *gitdir;
CALLOC_ARRAY(repo, 1);
+ packfile_list_init(&repo->packs);
argv++;
for (i = 1; i < argc; i++, argv++) {
@@ -1992,6 +1993,7 @@ int cmd_main(int argc, const char **argv)
cleanup:
if (info_ref_lock)
unlock_remote(info_ref_lock);
+ packfile_list_clear(&repo->packs);
free(repo->url);
free(repo);
diff --git a/http-walker.c b/http-walker.c
index b58a3b2a92..541437e52d 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -325,6 +325,7 @@ static void process_alternates_response(void *callback_data)
warning("adding alternate object store: %s",
target.buf);
CALLOC_ARRAY(newalt, 1);
+ packfile_list_init(&newalt->packs);
newalt->base = strbuf_detach(&target, NULL);
while (tail->next != NULL)
@@ -609,6 +610,7 @@ struct walker *get_http_walker(const char *url)
struct walker *walker = xmalloc(sizeof(struct walker));
CALLOC_ARRAY(data->alt, 1);
+ packfile_list_init(&data->alt->packs);
data->alt->base = xstrdup(url);
for (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s)
*s = 0;
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0890704e76..082c2494cb 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -835,6 +835,7 @@ struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
CALLOC_ARRAY(packed, 1);
odb_source_init(&packed->base, odb, ODB_SOURCE_PACKED, path, local);
+ packfile_list_init(&packed->packs);
strmap_init(&packed->packs_by_path);
packed->base.free = odb_source_packed_free;
diff --git a/packfile-list.c b/packfile-list.c
index 01fb913abf..d3c4843d8d 100644
--- a/packfile-list.c
+++ b/packfile-list.c
@@ -2,6 +2,28 @@
#include "packfile.h"
#include "packfile-list.h"
+static unsigned int packfile_list_entry_hash(struct packfile_list_entry *e)
+{
+ return memhash(&e->pack, sizeof(e->pack));
+}
+
+static int packfile_list_entry_cmp(const void *data UNUSED,
+ const struct hashmap_entry *h1,
+ const struct hashmap_entry *h2,
+ const void *keydata UNUSED)
+{
+ const struct packfile_list_entry *e1, *e2;
+ e1 = container_of(h1, const struct packfile_list_entry, ent);
+ e2 = container_of(h2, const struct packfile_list_entry, ent);
+ return e1->pack != e2->pack;
+}
+
+void packfile_list_init(struct packfile_list *list)
+{
+ memset(list, 0, sizeof(*list));
+ hashmap_init(&list->seen, packfile_list_entry_cmp, NULL, 0);
+}
+
void packfile_list_clear(struct packfile_list *list)
{
struct packfile_list_entry *e, *next;
@@ -12,6 +34,20 @@ void packfile_list_clear(struct packfile_list *list)
}
list->head = list->tail = NULL;
+
+ hashmap_clear(&list->seen);
+}
+
+static struct packfile_list_entry *packfile_list_lookup(struct packfile_list *list,
+ struct packed_git *pack)
+{
+ struct packfile_list_entry key = { .pack = pack };
+ struct hashmap_entry *ent;
+
+ hashmap_entry_init(&key.ent, packfile_list_entry_hash(&key));
+ ent = hashmap_get(&list->seen, &key.ent, NULL);
+
+ return ent ? container_of(ent, struct packfile_list_entry, ent) : NULL;
}
static struct packfile_list_entry *packfile_list_remove_internal(struct packfile_list *list,
@@ -38,20 +74,33 @@ static struct packfile_list_entry *packfile_list_remove_internal(struct packfile
void packfile_list_remove(struct packfile_list *list, struct packed_git *pack)
{
- free(packfile_list_remove_internal(list, pack));
+ struct packfile_list_entry key = { .pack = pack };
+
+ hashmap_entry_init(&key.ent, packfile_list_entry_hash(&key));
+ if (hashmap_remove(&list->seen, &key.ent, NULL)) {
+ struct packfile_list_entry *e = packfile_list_remove_internal(list, pack);
+ if (!e)
+ BUG("corrupt packfile list");
+ free(e);
+ }
}
void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack)
{
struct packfile_list_entry *entry;
- entry = packfile_list_remove_internal(list, pack);
- if (!entry) {
+ if (packfile_list_lookup(list, pack)) {
+ entry = packfile_list_remove_internal(list, pack);
+ if (!entry)
+ BUG("corrupt packfile list");
+ } else {
entry = xmalloc(sizeof(*entry));
entry->pack = pack;
+ hashmap_entry_init(&entry->ent, packfile_list_entry_hash(entry));
+ hashmap_add(&list->seen, &entry->ent);
}
- entry->next = list->head;
+ entry->next = list->head;
list->head = entry;
if (!list->tail)
list->tail = entry;
@@ -61,13 +110,18 @@ void packfile_list_append(struct packfile_list *list, struct packed_git *pack)
{
struct packfile_list_entry *entry;
- entry = packfile_list_remove_internal(list, pack);
- if (!entry) {
+ if (packfile_list_lookup(list, pack)) {
+ entry = packfile_list_remove_internal(list, pack);
+ if (!entry)
+ BUG("corrupt packfile list");
+ } else {
entry = xmalloc(sizeof(*entry));
entry->pack = pack;
+ hashmap_entry_init(&entry->ent, packfile_list_entry_hash(entry));
+ hashmap_add(&list->seen, &entry->ent);
}
- entry->next = NULL;
+ entry->next = NULL;
if (list->tail) {
list->tail->next = entry;
list->tail = entry;
diff --git a/packfile-list.h b/packfile-list.h
index 1b05e2aa36..bfb7017852 100644
--- a/packfile-list.h
+++ b/packfile-list.h
@@ -1,17 +1,22 @@
#ifndef PACKFILE_LIST_H
#define PACKFILE_LIST_H
+#include "hashmap.h"
+
struct object_id;
struct packfile_list {
struct packfile_list_entry *head, *tail;
+ struct hashmap seen;
};
struct packfile_list_entry {
+ struct hashmap_entry ent;
struct packfile_list_entry *next;
struct packed_git *pack;
};
+void packfile_list_init(struct packfile_list *list);
void packfile_list_clear(struct packfile_list *list);
void packfile_list_remove(struct packfile_list *list, struct packed_git *pack);
void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack); |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Aug 12, 2026 at 05:29:55PM -0400, Jeff King wrote:
> On Wed, Aug 12, 2026 at 12:51:30PM -0700, Junio C Hamano wrote:
>
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> > > In one reported use case (https://github.com/microsoft/git/issues/970),
> > > N equals 37,815 and caused a slow-down of a simple `git rev-parse
> > > --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> > > 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> > > increased from under 2 minutes to over half an hour.
> >
> > Face with Rolling Eyes (1f644) 🙄
> >
> > As we grow older, more and more extreme use cases that we initially
> > thought were simply crazy become reality.
>
> Sort of. The quadratic adding became a problem long ago, hence
> ec48540fe8 (packfile.c: speed up loading lots of packfiles, 2019-11-27).
>
> So this was something we already dealt with that regressed. We can even
> see the regression in our perf suite:
>
> $ GIT_SKIP_TESTS='p5303.[1-9] p5303.1[0-9]' ./run 589127caa730^ 589127caa730 p5303-many-packs.sh
> Test 589127caa730^ 589127caa730
> ----------------------------------------------------------------------
> 5303.21: load 10,000 packs 0.13(0.11+0.02) 0.45(0.42+0.02) +246.2%
>
> Unfortunately I don't think anybody pays close attention to the perf
> suite (partially because it's clunky and expensive to run, but also
> because it often requires human judgement to decide when something is a
> real change and not just a blip).
Yeah, that's a problem indeed. At GitLab we do have Bencher set up for
continuous benchmarking [1], but due to recent changes to our CI setup
those are now very flaky because seemingly, we flip-flop between two
different runners that have different specs. But we're obviously missing
a test there with lots of packfiles, so we didn't catch this regression.
Thanks!
Patrick
[1]: https://bencher.dev/perf/git/plots |
Since 589127c (packfile: move list of packs into the packfile store, 2025-10-30), there is a performance regression when many packfiles need to be loaded: `packfile_store_add_pack()` now calls `packfile_list_remove_internal()` to detect whether the packfile was _already_ in the list, and if so, move it to the end of the list. This function linearly scans the existing list before every insertion. Newly loading N packs therefore has complexity O(N²). In one reported use case (microsoft#970), N equals 37,815 and caused a slow-down of a simple `git rev-parse --short HEAD` (which is regularly executed as part of `GIT_PS1`) from 0.4s to 4.5s. Let's fix this by establishing a fast path for known-new packfiles. The keen reader will note that there is currently only a single, "known-new" caller of the `packfile_list_append()` function, and wonder why not simply remove this check whether the packfile already exists in the list? Originally, when above-mentioned commit introduced that logic, there was a second caller in `prepare_midx()`, which would have required that check, but that caller was removed in 6aff1f2 (packfile: always add packfiles to MRU when adding a pack, 2025-10-30). Still, the function is declared in a header file, and to avoid any problems with in-flight or downstream callers, it is safer to extend the signature to be explicit whether or not to skip that check. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
fix-perf-regression-in-v2.53-with-many-packfiles
branch
from
August 13, 2026 09:10
3dfb305 to
de11b6b
Compare
|
Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Jeff,
On Wed, 12 Aug 2026, Jeff King wrote:
> On Wed, Aug 12, 2026 at 12:51:30PM -0700, Junio C Hamano wrote:
>
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> [...]
> but...
>
> > > --- a/t/perf/p5303-many-packs.sh
> > > +++ b/t/perf/p5303-many-packs.sh
> > > @@ -141,4 +141,8 @@ test_perf "load 10,000 packs" '
> > > git rev-parse --verify "HEAD^{commit}"
> > > '
> > >
> > > +test_perf "abbreviate with 10,000 packs" '
> > > + git rev-parse --short HEAD
> > > +'
>
> ...I wonder what value this is adding. It shows the same slowdown as the
> existing test you can see in the context (and whose results I showed
> above).
I do think that there is value in adding this. It not only directly
reflects what GIT_PS1 runs, but it also exercises a subtly different path:
`--short` has to look for the unique abbreviation, whereas `--verify` can
stop as soon as it found the OID already.
Ciao,
Johannes |
|
Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Junio,
On Wed, 12 Aug 2026, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > In one reported use case (https://github.com/microsoft/git/issues/970),
> > N equals 37,815 and caused a slow-down of a simple `git rev-parse
> > --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> > 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> > increased from under 2 minutes to over half an hour.
>
> Face with Rolling Eyes (1f644) 🙄
>
> As we grow older, more and more extreme use cases that we initially
> thought were simply crazy become reality.
I have to take back the claim about the clone time, the hunt for that CI
regression is still ongoing, and this patch does _not_ fix it.
Ciao,
Johannes
>
> > Let's fix this by establishing a fast path for known-new packfiles.
>
> As long as the caller reliably knows that the pack it has is new and
> cannot be on the list, there is no reason to cycle through all the
> packs in the ring to attempt removing it in vain.
>
> Clever and clean.
>
> > diff --git a/packfile.c b/packfile.c
> > index 0eee45055f..f80f05a1fe 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -781,7 +781,7 @@ void packfile_store_add_pack(struct odb_source_packed *store,
> > if (pack->pack_fd != -1)
> > pack_open_fds++;
> >
> > - packfile_list_append(&store->packs, pack);
> > + packfile_list_append(&store->packs, pack, 1);
> > strmap_put(&store->packs_by_path, pack->pack_name, pack);
> > }
> >
> > diff --git a/t/perf/p5303-many-packs.sh b/t/perf/p5303-many-packs.sh
> > index af173a7b73..4221f9dd70 100755
> > --- a/t/perf/p5303-many-packs.sh
> > +++ b/t/perf/p5303-many-packs.sh
> > @@ -141,4 +141,8 @@ test_perf "load 10,000 packs" '
> > git rev-parse --verify "HEAD^{commit}"
> > '
> >
> > +test_perf "abbreviate with 10,000 packs" '
> > + git rev-parse --short HEAD
> > +'
> > +
> > test_done
> >
> > base-commit: 11c6700f10234578d10523faf35656ca491425c9
> |
|
Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Ben,
On Wed, 12 Aug 2026, Ben Knoble wrote:
> > Le 12 août 2026 à 15:15, Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> a écrit :
> >
> > [...]
> > packfile: fix perf regression with many packs
> >
> > This issue was spotted by a Microsoft Git user with the massive
> > amount of packfiles typical of an average, long-running monorepo
> > checkout.
>
> As a different kind of intermediate solution, would turning on
> maintenance for that user’s checkout help? (Not sure that would help CI
> clone times unless the server repacks, of course.)
I should have clarified that the issue is a _Scalar_ clone. And
specifically a _Microsoft Git Scalar_ clone.
This matters because, for various reasons that I don't want to elaborate
on because today I'm in need of lifting up my mood, a substantial part of
Microsoft Git failed to get upstreamed to core Git.
One of these is the "shared cache repository", i.e. a bare repository that
is established as an alternate of the actual clone, and into which the
actual scheduled fetches go. For full details, see
https://github.com/microsoft/git/commit/55226d12ed36 (scalar: do
initialize `gvfs.sharedCache`, 2021-05-03).
Now, maintenance _does_ run, usually, on that shared cache repository
(being careful not to inadvertently drop objects merely because they're
unreachable within the shared cache repository). So theoretically, you're
right that maintenance should help this issue.
For reasons (which I don't have the time to find out, but I suspect that
maintenance simply takes too long and does not finish by the time the
machine is shut down for the day), it is still not exactly rare to find
setups with five-digit packfile counts. And since we _can_ handle this
more gracefully, we should ;-)
Ciao,
Johannes |
|
Johannes Schindelin wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On Thu, 13 Aug 2026, Patrick Steinhardt wrote:
> On Wed, Aug 12, 2026 at 07:11:09PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Since 589127caa730 (packfile: move list of packs into the packfile
> > store, 2025-10-30), there is a performance regression when many
> > packfiles need to be loaded: `packfile_store_add_pack()` now calls
> > `packfile_list_remove_internal()` to detect whether the packfile was
> > _already_ in the list, if if so, move it to the end of the list. This
>
> Nit: s/if if/and if/
Thanks, will fix, along with dropping the claim that the CI clone was
fixed by this patch.
>
> > function linearly scans the existing list before every insertion. Newly
> > loading N packs therefore has complexity O(N²).
> >
> > In one reported use case (https://github.com/microsoft/git/issues/970),
> > N equals 37,815 and caused a slow-down of a simple `git rev-parse
> > --short HEAD` (which is regularly executed as part of `GIT_PS1`) from
> > 0.4s to 4.5s. In another, heavily exercised CI scenario, clone times
> > increased from under 2 minutes to over half an hour.
>
> Wow, 38k packfiles is a lot.
Yes.
> > Let's fix this by establishing a fast path for known-new packfiles.
> >
> > The keen reader will note that there is currently only a single,
> > "known-new" caller of the `packfile_list_append()` function, and wonder
> > why not simply remove this check whether the packfile already exists in
> > the list? Originally, when above-mentioned commit introduced that logic,
> > there was a second caller in `prepare_midx()`, which would have required
> > that check, but that caller was removed in 6aff1f25a046 (packfile:
> > always add packfiles to MRU when adding a pack, 2025-10-30). Still, the
> > function is declared in a header file, and to avoid any problems with
> > in-flight or downstream callers, it is safer to extend the signature to
> > be explicit whether or not to skip that check.
>
> Quite conservative, but fair enough.
>
> > diff --git a/packfile-list.c b/packfile-list.c
> > index 01fb913abf..1379ab3a4f 100644
> > --- a/packfile-list.c
> > +++ b/packfile-list.c
> > @@ -57,11 +57,12 @@ void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack)
> > list->tail = entry;
> > }
> >
> > -void packfile_list_append(struct packfile_list *list, struct packed_git *pack)
> > +void packfile_list_append(struct packfile_list *list, struct packed_git *pack,
> > + int is_new)
> > {
> > struct packfile_list_entry *entry;
> >
> > - entry = packfile_list_remove_internal(list, pack);
> > + entry = is_new ? NULL : packfile_list_remove_internal(list, pack);
> > if (!entry) {
> > entry = xmalloc(sizeof(*entry));
> > entry->pack = pack;
>
> I wonder whether we should slightly reformulate this and rename `is_new`
> to `accept_duplicates`. Because ultimately, that is what we're doing
> now: instead of ensuring that the packfile is unique in the list, we
> just don't care and just append the entry to the list.
Hmm. I don't quite agree, we're _not_ accepting duplicates. We know that
those packfiles _cannot_ be duplicates.
> An alternative would be to use a hashmap here that tracks the packs that
> have already been added. It has the advantage that it also covers the
> `prepend()` operation and that callers don't have to be aware of this
> mechanism at all. Furthermore, moving preexisting entries to the back or
> front could become O(logn) if the list was doubly-linked. We do this
> operation quite often to re-sort entries in the list when looking up
> objects.
Indeed, that was my initial reaction, too. I was well on my way to start
writing a hashmap-based fix when the AI assistant pointed out that no
duplicates could possibly exist yet.
> Overall though I'm not quite sure whether the added complexity would be
> worth it, see below patch.
Wow, you got a lot further than I did! And yes, I agree that we do not
(yet?) need to deal with the added complexity.
Ciao,
Johannes
>
> Thanks!
>
> Patrick
>
> diff --git a/http-push.c b/http-push.c
> index 94a1fac9ab..52b00e7c95 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1729,6 +1729,7 @@ int cmd_main(int argc, const char **argv)
> const char *gitdir;
>
> CALLOC_ARRAY(repo, 1);
> + packfile_list_init(&repo->packs);
>
> argv++;
> for (i = 1; i < argc; i++, argv++) {
> @@ -1992,6 +1993,7 @@ int cmd_main(int argc, const char **argv)
> cleanup:
> if (info_ref_lock)
> unlock_remote(info_ref_lock);
> + packfile_list_clear(&repo->packs);
> free(repo->url);
> free(repo);
>
> diff --git a/http-walker.c b/http-walker.c
> index b58a3b2a92..541437e52d 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -325,6 +325,7 @@ static void process_alternates_response(void *callback_data)
> warning("adding alternate object store: %s",
> target.buf);
> CALLOC_ARRAY(newalt, 1);
> + packfile_list_init(&newalt->packs);
> newalt->base = strbuf_detach(&target, NULL);
>
> while (tail->next != NULL)
> @@ -609,6 +610,7 @@ struct walker *get_http_walker(const char *url)
> struct walker *walker = xmalloc(sizeof(struct walker));
>
> CALLOC_ARRAY(data->alt, 1);
> + packfile_list_init(&data->alt->packs);
> data->alt->base = xstrdup(url);
> for (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s)
> *s = 0;
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 0890704e76..082c2494cb 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -835,6 +835,7 @@ struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
>
> CALLOC_ARRAY(packed, 1);
> odb_source_init(&packed->base, odb, ODB_SOURCE_PACKED, path, local);
> + packfile_list_init(&packed->packs);
> strmap_init(&packed->packs_by_path);
>
> packed->base.free = odb_source_packed_free;
> diff --git a/packfile-list.c b/packfile-list.c
> index 01fb913abf..d3c4843d8d 100644
> --- a/packfile-list.c
> +++ b/packfile-list.c
> @@ -2,6 +2,28 @@
> #include "packfile.h"
> #include "packfile-list.h"
>
> +static unsigned int packfile_list_entry_hash(struct packfile_list_entry *e)
> +{
> + return memhash(&e->pack, sizeof(e->pack));
> +}
> +
> +static int packfile_list_entry_cmp(const void *data UNUSED,
> + const struct hashmap_entry *h1,
> + const struct hashmap_entry *h2,
> + const void *keydata UNUSED)
> +{
> + const struct packfile_list_entry *e1, *e2;
> + e1 = container_of(h1, const struct packfile_list_entry, ent);
> + e2 = container_of(h2, const struct packfile_list_entry, ent);
> + return e1->pack != e2->pack;
> +}
> +
> +void packfile_list_init(struct packfile_list *list)
> +{
> + memset(list, 0, sizeof(*list));
> + hashmap_init(&list->seen, packfile_list_entry_cmp, NULL, 0);
> +}
> +
> void packfile_list_clear(struct packfile_list *list)
> {
> struct packfile_list_entry *e, *next;
> @@ -12,6 +34,20 @@ void packfile_list_clear(struct packfile_list *list)
> }
>
> list->head = list->tail = NULL;
> +
> + hashmap_clear(&list->seen);
> +}
> +
> +static struct packfile_list_entry *packfile_list_lookup(struct packfile_list *list,
> + struct packed_git *pack)
> +{
> + struct packfile_list_entry key = { .pack = pack };
> + struct hashmap_entry *ent;
> +
> + hashmap_entry_init(&key.ent, packfile_list_entry_hash(&key));
> + ent = hashmap_get(&list->seen, &key.ent, NULL);
> +
> + return ent ? container_of(ent, struct packfile_list_entry, ent) : NULL;
> }
>
> static struct packfile_list_entry *packfile_list_remove_internal(struct packfile_list *list,
> @@ -38,20 +74,33 @@ static struct packfile_list_entry *packfile_list_remove_internal(struct packfile
>
> void packfile_list_remove(struct packfile_list *list, struct packed_git *pack)
> {
> - free(packfile_list_remove_internal(list, pack));
> + struct packfile_list_entry key = { .pack = pack };
> +
> + hashmap_entry_init(&key.ent, packfile_list_entry_hash(&key));
> + if (hashmap_remove(&list->seen, &key.ent, NULL)) {
> + struct packfile_list_entry *e = packfile_list_remove_internal(list, pack);
> + if (!e)
> + BUG("corrupt packfile list");
> + free(e);
> + }
> }
>
> void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack)
> {
> struct packfile_list_entry *entry;
>
> - entry = packfile_list_remove_internal(list, pack);
> - if (!entry) {
> + if (packfile_list_lookup(list, pack)) {
> + entry = packfile_list_remove_internal(list, pack);
> + if (!entry)
> + BUG("corrupt packfile list");
> + } else {
> entry = xmalloc(sizeof(*entry));
> entry->pack = pack;
> + hashmap_entry_init(&entry->ent, packfile_list_entry_hash(entry));
> + hashmap_add(&list->seen, &entry->ent);
> }
> - entry->next = list->head;
>
> + entry->next = list->head;
> list->head = entry;
> if (!list->tail)
> list->tail = entry;
> @@ -61,13 +110,18 @@ void packfile_list_append(struct packfile_list *list, struct packed_git *pack)
> {
> struct packfile_list_entry *entry;
>
> - entry = packfile_list_remove_internal(list, pack);
> - if (!entry) {
> + if (packfile_list_lookup(list, pack)) {
> + entry = packfile_list_remove_internal(list, pack);
> + if (!entry)
> + BUG("corrupt packfile list");
> + } else {
> entry = xmalloc(sizeof(*entry));
> entry->pack = pack;
> + hashmap_entry_init(&entry->ent, packfile_list_entry_hash(entry));
> + hashmap_add(&list->seen, &entry->ent);
> }
> - entry->next = NULL;
>
> + entry->next = NULL;
> if (list->tail) {
> list->tail->next = entry;
> list->tail = entry;
> diff --git a/packfile-list.h b/packfile-list.h
> index 1b05e2aa36..bfb7017852 100644
> --- a/packfile-list.h
> +++ b/packfile-list.h
> @@ -1,17 +1,22 @@
> #ifndef PACKFILE_LIST_H
> #define PACKFILE_LIST_H
>
> +#include "hashmap.h"
> +
> struct object_id;
>
> struct packfile_list {
> struct packfile_list_entry *head, *tail;
> + struct hashmap seen;
> };
>
> struct packfile_list_entry {
> + struct hashmap_entry ent;
> struct packfile_list_entry *next;
> struct packed_git *pack;
> };
>
> +void packfile_list_init(struct packfile_list *list);
> void packfile_list_clear(struct packfile_list *list);
> void packfile_list_remove(struct packfile_list *list, struct packed_git *pack);
> void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack);
> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This issue was spotted by a Microsoft Git user with the massive amount of packfiles typical of an average, long-running monorepo checkout.
Changes since v1:
Cc: Patrick Steinhardt ps@pks.im
cc: Jeff King peff@peff.net
cc: Ben Knoble ben.knoble@gmail.com