[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