git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 3/3] revision: avoid parsing with --exclude-promisor-objects

From
Jeff King <peff@peff.net>
Date
Apr 13, 2021, 07:17 UTC
Message-ID
<YHVFnNvGim8Iduwq@coredump.intra.peff.net>
In-Reply-To
<YHVECXHfZ1bidTJH@coredump.intra.peff.net>

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(-)
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
-- 
2.31.1.659.g9b9913af63
Previous: Jeff KingNext: Junio C Hamano
Message 16 of 46 in “rather slow 'git repack' in 'blob:none' partial clones”
  1. SZEDER GáborApr 3, 2021
  2. Rafael SilvaApr 5, 2021
  3. Jeff KingApr 7, 2021
  4. Jonathan TanApr 8, 2021
  5. Jeff KingApr 8, 2021
  6. Rafael SilvaApr 12, 2021
  7. SZEDER GáborApr 12, 2021
  8. Bryan TurnerApr 12, 2021
  9. Jeff KingApr 12, 2021
  10. Jeff KingApr 12, 2021
  11. 0/3 low-hanging performance fruit with promisor packsJeff King, Apr 13, 2021
  12. 1/3 is_promisor_object(): free tree buffer after parsingJeff King, Apr 13, 2021
  13. Junio C HamanoApr 13, 2021
  14. Jeff KingApr 14, 2021
  15. 2/3 lookup_unknown_object(): take a repository argumentJeff King, Apr 13, 2021
  16. 3/3 revision: avoid parsing with --exclude-promisor-objectsJeff King, Apr 13, 2021
  17. Junio C HamanoApr 13, 2021
  18. SZEDER GáborApr 13, 2021
  19. Jonathan TanApr 14, 2021
  20. Rafael SilvaApr 14, 2021
  21. SZEDER GáborApr 13, 2021
  22. Jeff KingApr 14, 2021
  23. SZEDER GáborApr 11, 2021
  24. Rafael SilvaApr 12, 2021
  25. 0/2 prevent `repack` to unpack and delete promisor objectsRafael Silva, Apr 14, 2021
  26. 1/2 repack: teach --no-prune-packed to skip `git prune-packed`Rafael Silva, Apr 14, 2021
  27. Jonathan TanApr 14, 2021
  28. Rafael SilvaApr 18, 2021
  29. 2/2 repack: avoid loosening promisor pack objects in partial clonesRafael Silva, Apr 14, 2021
  30. Jonathan TanApr 15, 2021
  31. Junio C HamanoApr 15, 2021
  32. Jeff KingApr 15, 2021
  33. Jeff KingApr 15, 2021
  34. Rafael SilvaApr 18, 2021
  35. Junio C HamanoApr 15, 2021
  36. Rafael SilvaApr 18, 2021
  37. Junio C HamanoApr 14, 2021
  38. Jeff KingApr 15, 2021
  39. Rafael SilvaApr 18, 2021
  40. 0/1 prevent `repack` to unpack and delete promisor objectsRafael Silva, Apr 18, 2021
  41. 1/1 repack: avoid loosening promisor objects in partial clonesRafael Silva, Apr 18, 2021
  42. Jonathan TanApr 19, 2021
  43. Rafael SilvaApr 21, 2021
  44. Junio C HamanoApr 19, 2021
  45. Rafael SilvaApr 21, 2021
  46. repack: avoid loosening promisor objects in partial clonesRafael Silva, Apr 21, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.