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

[PATCH v2 0/3] Clean up leaks in commit-graph.c

From
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Date
Oct 3, 2018, 17:12 UTC
Message-ID
<pull.42.v2.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.42.git.gitgitgadget@gmail.com>

While looking at the commit-graph code, I noticed some memory leaks. These can be found by running

valgrind --leak-check=full ./git commit-graph write --reachable

The impact of these leaks are small, as we never call write_commit_graph _reachable in a loop, but it is best to be diligent here.

While looking at memory consumption within write_commit_graph(), I noticed that we initialize our oid list with "object count / 4", which seems to be a huge over-count. I reduce this by a factor of eight.

I built off of ab/commit-graph-progress, because my patch involves lines close to those changes.

V2 includes feedback from V1 along with Martin's additional patches.
Thanks, -Stolee
Derrick Stolee (2):
  commit-graph: clean up leaked memory during write
  commit-graph: reduce initial oid allocation
Martin Ågren (1):
  builtin/commit-graph.c: UNLEAK variables
 builtin/commit-graph.c | 11 ++++++-----
 commit-graph.c         | 16 ++++++++++------
 2 files changed, 16 insertions(+), 11 deletions(-)
base-commit: 6b89a34c89fc763292f06012318b852b74825619
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-42%2Fderrickstolee%2Fcommit-graph-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-42/derrickstolee/commit-graph-leak-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/42
Range-diff vs v1:
 1:  6906c25415 ! 1:  ba65680b3d commit-graph: clean up leaked memory during write
     @@ -7,17 +7,29 @@
          leaked in write_commit_graph_reachable(). Clean these up so our
          memory checking is cleaner.
      
     -    Running 'valgrind --leak-check=full git commit-graph write
     -    --reachable' demonstrates these leaks and how they are fixed after
     -    this change.
     +    Further, if we use a list of pack-files to find the commits, we
     +    can leak the packed_git structs after scanning them for commits.
      
     +    Running the following commands demonstrates the leak before and
     +    the fix after:
     +
     +    * valgrind --leak-check=full ./git commit-graph write --reachable
     +    * valgrind --leak-check=full ./git commit-graph write --stdin-packs
     +
     +    Signed-off-by: Martin Ågren <martin.agren@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
      diff --git a/commit-graph.c b/commit-graph.c
      --- a/commit-graph.c
      +++ b/commit-graph.c
      @@
     - 	string_list_init(&list, 1);
     + void write_commit_graph_reachable(const char *obj_dir, int append,
     + 				  int report_progress)
     + {
     +-	struct string_list list;
     ++	struct string_list list = STRING_LIST_INIT_DUP;
     + 
     +-	string_list_init(&list, 1);
       	for_each_ref(add_ref_to_list, &list);
       	write_commit_graph(obj_dir, NULL, &list, append, report_progress);
      +
     @@ -25,6 +37,14 @@
       }
       
       void write_commit_graph(const char *obj_dir,
     +@@
     + 				die(_("error opening index for %s"), packname.buf);
     + 			for_each_object_in_pack(p, add_packed_commits, &oids, 0);
     + 			close_pack(p);
     ++			free(p);
     + 		}
     + 		stop_progress(&oids.progress);
     + 		strbuf_release(&packname);
      @@
       	compute_generation_numbers(&commits, report_progress);
       
     @@ -45,5 +65,8 @@
      +	free(graph_name);
      +	free(commits.list);
       	free(oids.list);
     - 	oids.alloc = 0;
     - 	oids.nr = 0;
     +-	oids.alloc = 0;
     +-	oids.nr = 0;
     + }
     + 
     + #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
 -:  ---------- > 2:  13032d8475 builtin/commit-graph.c: UNLEAK variables
 2:  e29a0eaf03 = 3:  1002fd34fc commit-graph: reduce initial oid allocation
-- 
gitgitgadget
Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee via GitGitGadget
Message 17 of 20 in “Clean up leaks in commit-graph.c”
  1. 0/2 Clean up leaks in commit-graph.cDerrick Stolee via GitGitGadget, Oct 2, 2018
  2. 1/2 commit-graph: clean up leaked memory during writeDerrick Stolee via GitGitGadget, Oct 2, 2018
  3. Martin ÅgrenOct 2, 2018
  4. Stefan BellerOct 2, 2018
  5. Martin ÅgrenOct 2, 2018
  6. Stefan BellerOct 2, 2018
  7. Jeff KingOct 2, 2018
  8. Stefan BellerOct 2, 2018
  9. Derrick StoleeOct 3, 2018
  10. 0/2 commit-graph: more leak fixesMartin Ågren, Oct 3, 2018
  11. 1/2 commit-graph: free `struct packed_git` after closing itMartin Ågren, Oct 3, 2018
  12. 2/2 builtin/commit-graph.c: UNLEAK variablesMartin Ågren, Oct 3, 2018
  13. Derrick StoleeOct 3, 2018
  14. Martin ÅgrenOct 3, 2018
  15. Jeff KingOct 2, 2018
  16. 2/2 commit-graph: reduce initial oid allocationDerrick Stolee via GitGitGadget, Oct 2, 2018
  17. 0/3 Clean up leaks in commit-graph.cDerrick Stolee via GitGitGadget, Oct 3, 2018
  18. 1/3 commit-graph: clean up leaked memory during writeDerrick Stolee via GitGitGadget, Oct 3, 2018
  19. 2/3 builtin/commit-graph.c: UNLEAK variablesMartin Ågren via GitGitGadget, Oct 3, 2018
  20. 3/3 commit-graph: reduce initial oid allocationDerrick Stolee via GitGitGadget, Oct 3, 2018

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.