Re: [PATCH v3 3/4] revision: avoid loading object headers multiple times
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-08-02 12:58:20
On Mon, Aug 02 2021, Patrick Steinhardt wrote:
quoted hunk ↗ jump to hunk
[[PGP Signed Part:Undecided]] When loading references, we try to optimize loading of commits by using the commit graph. To do so, we first need to determine whether the object actually is a commit or not, which is why we always execute `oid_object_info()` first. Like this, we'll unpack the object header of each object first. This pattern can be quite inefficient in case many references point to the same commit: if the object didn't end up in the cached objects, then we'll repeatedly unpack the same object header, even if we've already seen the object before. Optimize this pattern by using `lookup_unknown_object()` first in order to determine whether we've seen the object before. If so, then we don't need to re-parse the header but can directly use its object information and thus gain a modest performance improvement. Executed in a real-world repository with around 2.2 million references: Benchmark #1: HEAD~: rev-list --unsorted-input --objects --quiet --not --all --not $newrev Time (mean ± σ): 4.771 s ± 0.238 s [User: 4.440 s, System: 0.330 s] Range (min … max): 4.539 s … 5.219 s 10 runs Benchmark #2: HEAD: rev-list --unsorted-input --objects --quiet --not --all --not $newrev Time (mean ± σ): 4.454 s ± 0.037 s [User: 4.122 s, System: 0.332 s] Range (min … max): 4.375 s … 4.496 s 10 runs Summary 'HEAD: rev-list --unsorted-input --objects --quiet --not --all --not $newrev' ran 1.07 ± 0.05 times faster than 'HEAD~: rev-list --unsorted-input --objects --quiet --not --all --not $newrev' The downside is that `lookup_unknown_object()` is forced to always allocate an object such that it's big enough to host all object types' structs and thus we may waste memory here. This tradeoff is probably worth it though considering the following struct sizes: - commit: 72 bytes - tree: 56 bytes - blob: 40 bytes - tag: 64 bytes Assuming that in almost all repositories, most references will point to either a tag or a commit, we'd have a modest increase in memory consumption of about 12.5% here. Signed-off-by: Patrick Steinhardt <redacted> --- revision.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)diff --git a/revision.c b/revision.c index f06a5d63a3..671b6d6513 100644 --- a/revision.c +++ b/revision.c@@ -359,14 +359,22 @@ static struct object *get_reference(struct rev_info *revs, const char *name, const struct object_id *oid, unsigned int flags) { - struct object *object; + struct object *object = lookup_unknown_object(revs->repo, oid); + + if (object->type == OBJ_NONE) { + int type = oid_object_info(revs->repo, oid, NULL); + if (type < 0 || !object_as_type(object, type, 1)) {
Let's s/int type/enum object_type, personally I think we should never do "type < 0" either, and check OBJ_BAD explicitly, but I've seemingly lost that discussion on-list before. But I think the consensus is that we should not do !type, but rather type == OBJ_NONE.