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

[PATCH 2/3] fsck: turn off save_commit_buffer

From
Jeff King <peff@peff.net>
Date
Sep 22, 2022, 10:13 UTC
Message-ID
<Yyw1UIhhj5zVtl1T@coredump.intra.peff.net>
In-Reply-To
<Yyw0PSVe3YTQGgRS@coredump.intra.peff.net>

When parsing a commit, the default behavior is to stuff the original buffer into a commit_slab (which takes ownership of it). But for a tool like fsck, this isn't useful. While we may look at the buffer further as part of fsck_commit(), we'll always do so through a separate pointer; attaching the buffer to the slab doesn't help.

Worse, it means we have to remember to free the commit buffer in all call paths. We do so in fsck_obj(), which covers a regular "git fsck". But with "--connectivity-only", we forget to do so in both traverse_one_object(), which covers reachable objects, and mark_unreachable_referents(), which covers unreachable ones. As a result, that mode ends up storing an uncompressed copy of every commit on the heap at once.

We could teach the code paths for --connectivity-only to also free commit buffers. But there's an even easier fix: we can just turn off the save_commit_buffer flag, and then we won't attach them to the commits in the first place.

This reduces the peak heap of running "git fsck --connectivity-only" in a clone of linux.git from ~2GB to ~1GB. According to massif, the remaining memory goes where you'd expect: the object structs themselves, the obj_hash containing them, and the delta base cache.

Note that we'll leave the call to free commit buffers in fsck_obj() for now; it's not quite redundant because of a related bug that we'll fix in a subsequent commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fsck.c | 1 +
 1 file changed, 1 insertion(+)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 34e575a170..b45de003d4 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -855,6 +855,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 
 	errors_found = 0;
 	read_replace_refs = 0;
+	save_commit_buffer = 0;
 
 	argc = parse_options(argc, argv, prefix, fsck_opts, fsck_usage, 0);
 
-- 
2.38.0.rc1.583.ga560cd8328
Previous: Jeff KingNext: Jeff King
Message 9 of 10 in “[INVESTIGATION] why is fsck --connectivity-only so much more expensive than rev-list --objects --all?”
  1. John CaiSep 20, 2022
  2. Jeff KingSep 20, 2022
  3. 0/3 reducing fsck memory usageJeff King, Sep 22, 2022
  4. 1/3 fsck: free tree buffers after walking unreachable objectsJeff King, Sep 22, 2022
  5. Junio C HamanoSep 22, 2022
  6. Jeff KingSep 22, 2022
  7. Junio C HamanoSep 22, 2022
  8. Jeff KingSep 22, 2022
  9. 2/3 fsck: turn off save_commit_bufferJeff King, Sep 22, 2022
  10. 3/3 parse_object_buffer(): respect save_commit_bufferJeff King, Sep 22, 2022

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.