On Thu, Jun 16, 2022 at 02:07:41AM -0400, Jeff King wrote:
> Those rev-lists run in 1.7s and 224s respectively. Ouch!
Even though I expected the second one to be slow, I was shocked at just
how slow it was. The patch below speeds it up by a factor of 2, and I
think is worth applying separately, regardless of anything else being
discussed in this thread.
-- >8 --
Subject: is_promisor_object(): walk promisor packs in pack-order
When we generate the list of promisor objects, we walk every pack with a
.promisor file and examine its objects for any links to other objects.
By default, for_each_packed_object() will go in pack .idx order.
This is the worst case with respect to our delta base cache. If we have
a delta chain of A->B->C->D, then visiting A may require reconstructing
both B and C, unless we also visited B recently, in which case we may
have cached its value. Because .idx order is based on sha1, it's random
with respect to the actual object contents and deltas, and thus we're
unlikely to get many cache hits.
If we instead traverse in pack order, then we get the optimal case:
packs are written to keep delta families together, and to place bases
before their children.
Even on a modest repository like git.git, this has a noticeable speedup
on p5600.4, which runs "fsck" on a partial clone with blob:none (so lots
of trees which need to be walked, and which delta well):
Test HEAD^ HEAD
-------------------------------------------------------
5600.4: 17.87(17.83+0.04) 15.42(15.35+0.06) -13.7%
On a larger repository like linux.git, the speedup is even more
pronounced:
Test HEAD^ HEAD
-----------------------------------------------------------
5600.4: 322.47(322.01+0.42) 186.41(185.76+0.63) -42.2%
Any other operations that call is_promisor_object(), like "rev-list
--exclude-promisor-objects", would similarly benefit, but the
invocations in p5600 don't actually trigger any such cases.
Note that we may pay a small price to build a rev-index in-memory to do
the pack-order traversal. But it's still a big net win, and even that
small cost goes away if you are using pack.writeReverseIndex.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
As an aside, feels like pack.writeReverseIndex ought to become the
default (which AFAIK hasn't happened yet).
packfile.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/packfile.c b/packfile.c
index 8e812a84a3..ed69fe457b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2275,7 +2275,8 @@ int is_promisor_object(const struct object_id *oid)
if (has_promisor_remote()) {
for_each_packed_object(add_promisor_object,
&promisor_objects,
- FOR_EACH_OBJECT_PROMISOR_ONLY);
+ FOR_EACH_OBJECT_PROMISOR_ONLY |
+ FOR_EACH_OBJECT_PACK_ORDER);
}
promisor_objects_prepared = 1;
}
--
2.37.0.rc0.352.g10876ef154
