From fcc07e980b344a813b47358d0aa823d07c7ac744 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 13 Apr 2021 03:15:54 -0400 Subject: is_promisor_object(): free tree buffer after parsing To get the list of all promisor objects, we not only include all objects in promisor packs, but also parse each of those objects to see which objects they reference. After parsing a tree object, the tree->buffer field will remain populated until we explicitly free it. So in a partial clone of blob:none, for example, we are essentially reading every tree in the repository (since they're all in the initial promisor pack), and keeping all of their uncompressed contents in memory at once. This patch frees the tree buffers after we've finished marking all of their reachable objects. We shouldn't need to do this for any other object type. While we are using some extra memory to store the structs, no other object type stores the whole contents in its parsed form (we do sometimes hold on to commit buffers, but less so these days due to commit graphs, plus most commands which care about promisor objects turn off the save_commit_buffer global). Even for a moderate-sized repository like git.git, this patch drops the peak heap (as measured by massif) for git-fsck from ~1.7GB to ~138MB. Fsck is a good candidate for measuring here because it doesn't interact with the promisor code except to call is_promisor_object(), so we can isolate just this problem. The added perf test shows only a tiny improvement on my machine for git.git, since 1.7GB isn't enough to cause any real memory pressure: Test HEAD^ HEAD -------------------------------------------------------------------------------- 5600.4: fsck 21.26(20.90+0.35) 20.84(20.79+0.04) -2.0% With linux.git the absolute change is a bit bigger, though still a small percentage: Test HEAD^ HEAD ----------------------------------------------------------------------------- 5600.4: fsck 262.26(259.13+3.12) 254.92(254.62+0.29) -2.8% I didn't have the patience to run it under massif with linux.git, but it's probably on the order of about 14GB improvement, since that's the sum of the sizes of all of the uncompressed trees (but still isn't enough to create memory pressure on this particular machine, which has 64GB of RAM). Smaller machines would probably see a bigger effect on runtime (and sadly our perf suite does not measure peak heap). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/packfile.c b/packfile.c index 8668345..b79cbc8 100644 --- a/packfile.c +++ b/packfile.c @@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid, return 0; while (tree_entry_gently(&desc, &entry)) oidset_insert(set, &entry.oid); + free_tree_buffer(tree); } else if (obj->type == OBJ_COMMIT) { struct commit *commit = (struct commit *) obj; struct commit_list *parents = commit->parents; diff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh index 3e04bd2..754aaec 100755 --- a/t/perf/p5600-partial-clone.sh +++ b/t/perf/p5600-partial-clone.sh @@ -23,4 +23,8 @@ test_perf 'checkout of result' ' git -C worktree checkout -f ' +test_perf 'fsck' ' + git -C bare.git fsck +' + test_done -- cgit v0.10.2-6-g49f6 From 45a187cc34b5016842b91ef14b59e40505afff46 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 13 Apr 2021 03:16:36 -0400 Subject: lookup_unknown_object(): take a repository argument All of the other lookup_foo() functions take a repository argument, but lookup_unknown_object() was never converted, and it uses the_repository internally. Let's fix that. We could leave a wrapper that uses the_repository, but there aren't that many calls, so we'll just convert them all. I looked briefly at each site to see if we had a repository struct (besides the_repository) we could pass, but none of them do (so this conversion to pass the_repository is a pure noop in each case, though it does take us one step closer to eventually getting rid of the_repository). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/fsck.c b/builtin/fsck.c index 70ff958..e6a80e5 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -725,7 +725,7 @@ static int fsck_cache_tree(struct cache_tree *it) static void mark_object_for_connectivity(const struct object_id *oid) { - struct object *obj = lookup_unknown_object(oid); + struct object *obj = lookup_unknown_object(the_repository, oid); obj->flags |= HAS_OBJ; } diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 525c2d8..c1186f5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3386,7 +3386,7 @@ static void add_objects_in_unpacked_packs(void) for (i = 0; i < p->num_objects; i++) { nth_packed_object_id(&oid, p, i); - o = lookup_unknown_object(&oid); + o = lookup_unknown_object(the_repository, &oid); if (!(o->flags & OBJECT_ADDED)) mark_in_pack_object(o, p, &in_pack); o->flags |= OBJECT_ADDED; diff --git a/http-push.c b/http-push.c index b60d5fc..8131232 100644 --- a/http-push.c +++ b/http-push.c @@ -1436,7 +1436,7 @@ static void one_remote_ref(const char *refname) * may be required for updating server info later. */ if (repo->can_update_info_refs && !has_object_file(&ref->old_oid)) { - obj = lookup_unknown_object(&ref->old_oid); + obj = lookup_unknown_object(the_repository, &ref->old_oid); fprintf(stderr, " fetch %s for %s\n", oid_to_hex(&ref->old_oid), refname); add_fetch_request(obj); diff --git a/object.c b/object.c index 7834378..1418845 100644 --- a/object.c +++ b/object.c @@ -177,12 +177,11 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet) } } -struct object *lookup_unknown_object(const struct object_id *oid) +struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid) { - struct object *obj = lookup_object(the_repository, oid); + struct object *obj = lookup_object(r, oid); if (!obj) - obj = create_object(the_repository, oid, - alloc_object_node(the_repository)); + obj = create_object(r, oid, alloc_object_node(r)); return obj; } diff --git a/object.h b/object.h index 59daadc..87a6da4 100644 --- a/object.h +++ b/object.h @@ -145,7 +145,7 @@ struct object *parse_object_or_die(const struct object_id *oid, const char *name 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); /** Returns the object, with potentially excess memory allocated. **/ -struct object *lookup_unknown_object(const struct object_id *oid); +struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid); struct object_list *object_list_insert(struct object *item, struct object_list **list_p); diff --git a/refs.c b/refs.c index 261fd82..1616c75 100644 --- a/refs.c +++ b/refs.c @@ -337,7 +337,7 @@ static int filter_refs(const char *refname, const struct object_id *oid, enum peel_status peel_object(const struct object_id *name, struct object_id *oid) { - struct object *o = lookup_unknown_object(name); + struct object *o = lookup_unknown_object(the_repository, name); if (o->type == OBJ_NONE) { int type = oid_object_info(the_repository, name, NULL); diff --git a/t/helper/test-example-decorate.c b/t/helper/test-example-decorate.c index c8a1cde..b9d1200 100644 --- a/t/helper/test-example-decorate.c +++ b/t/helper/test-example-decorate.c @@ -26,8 +26,8 @@ int cmd__example_decorate(int argc, const char **argv) * Add 2 objects, one with a non-NULL decoration and one with a NULL * decoration. */ - one = lookup_unknown_object(&one_oid); - two = lookup_unknown_object(&two_oid); + one = lookup_unknown_object(the_repository, &one_oid); + two = lookup_unknown_object(the_repository, &two_oid); ret = add_decoration(&n, one, &decoration_a); if (ret) BUG("when adding a brand-new object, NULL should be returned"); @@ -56,7 +56,7 @@ int cmd__example_decorate(int argc, const char **argv) ret = lookup_decoration(&n, two); if (ret != &decoration_b) BUG("lookup should return added declaration"); - three = lookup_unknown_object(&three_oid); + three = lookup_unknown_object(the_repository, &three_oid); ret = lookup_decoration(&n, three); if (ret) BUG("lookup for unknown object should return NULL"); diff --git a/upload-pack.c b/upload-pack.c index e19583a..5c1cd19 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1153,7 +1153,7 @@ static void receive_needs(struct upload_pack_data *data, static int mark_our_ref(const char *refname, const char *refname_full, const struct object_id *oid) { - struct object *o = lookup_unknown_object(oid); + struct object *o = lookup_unknown_object(the_repository, oid); if (ref_is_hidden(refname, refname_full)) { o->flags |= HIDDEN_REF; diff --git a/walker.c b/walker.c index 4984bf8..c5e2921 100644 --- a/walker.c +++ b/walker.c @@ -298,7 +298,7 @@ int walker_fetch(struct walker *walker, int targets, char **target, error("Could not interpret response from server '%s' as something to pull", target[i]); goto done; } - if (process(walker, lookup_unknown_object(&oids[i]))) + if (process(walker, lookup_unknown_object(the_repository, &oids[i]))) goto done; } -- cgit v0.10.2-6-g49f6 From c1fa951d7ea9e943f001ac7c7502995273db5776 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 13 Apr 2021 03:17:48 -0400 Subject: revision: avoid parsing with --exclude-promisor-objects When --exclude-promisor-objects is given, before traversing any objects we iterate over all of the objects in any promisor packs, marking them as UNINTERESTING and SEEN. We turn the oid we get from iterating the pack into an object with parse_object(), but this has two problems: - it's slow; we are zlib inflating (and reconstructing from deltas) every byte of every object in the packfile - it leaves the tree buffers attached to their structs, which means our heap usage will grow to store every uncompressed tree simultaneously. This can be gigabytes. We can obviously fix the second by freeing the tree buffers after we've parsed them. But we can observe that the function doesn't look at the object contents at all! The only reason we call parse_object() is that we need a "struct object" on which to set the flags. There are two options here: - we can look up just the object type via oid_object_info(), and then call the appropriate lookup_foo() function - we can call lookup_unknown_object(), which gives us an OBJ_NONE struct (which will get auto-converted later by object_as_type() via calls to lookup_commit(), etc). The first one is closer to the current code, but we do pay the price to look up the type for each object. The latter should be more efficient in CPU, though it wastes a little bit of memory (the "unknown" object structs are a union of all object types, so some of the structs are bigger than they need to be). It also runs the risk of triggering a latent bug in code that calls lookup_object() directly but isn't ready to handle OBJ_NONE (such code would already be buggy, but we use lookup_unknown_object() infrequently enough that it might be hiding). I went with the second option here. I don't think the risk is high (and we'd want to find and fix any such bugs anyway), and it should be more efficient overall. The new tests in p5600 show off the improvement (this is on git.git): Test HEAD^ HEAD ------------------------------------------------------------------------------- 5600.5: count commits 0.37(0.37+0.00) 0.38(0.38+0.00) +2.7% 5600.6: count non-promisor commits 11.74(11.37+0.37) 0.04(0.03+0.00) -99.7% The improvement is particularly big in this script because _every_ object in the newly-cloned partial repo is a promisor object. So after marking them all, there's nothing left to traverse. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/revision.c b/revision.c index 553c0fa..7e73daf 100644 --- a/revision.c +++ b/revision.c @@ -3271,7 +3271,7 @@ static int mark_uninteresting(const struct object_id *oid, void *cb) { struct rev_info *revs = cb; - struct object *o = parse_object(revs->repo, oid); + struct object *o = lookup_unknown_object(revs->repo, oid); o->flags |= UNINTERESTING | SEEN; return 0; } diff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh index 754aaec..ca785a3 100755 --- a/t/perf/p5600-partial-clone.sh +++ b/t/perf/p5600-partial-clone.sh @@ -27,4 +27,12 @@ test_perf 'fsck' ' git -C bare.git fsck ' +test_perf 'count commits' ' + git -C bare.git rev-list --all --count +' + +test_perf 'count non-promisor commits' ' + git -C bare.git rev-list --all --count --exclude-promisor-objects +' + test_done -- cgit v0.10.2-6-g49f6