| Message ID | YHVFnNvGim8Iduwq@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1fa951d7ea9e943f001ac7c7502995273db5776 |
| Headers | show |
| Series | low-hanging performance fruit with promisor packs | expand |
Jeff King <peff@peff.net> writes: > ... 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 That's clever. I like it. > 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. ;-).
diff --git a/revision.c b/revision.c index 553c0faa9b..7e73dafd96 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 754aaec3dc..ca785a3341 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
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 <peff@peff.net> --- revision.c | 2 +- t/perf/p5600-partial-clone.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)