From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 0/3] Clean up leaks in commit-graph.c
Date: Wed, 03 Oct 2018 10:12:14 -0700 (PDT) [thread overview]
Message-ID: <pull.42.v2.git.gitgitgadget@gmail.com> (raw)
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
next prev parent reply other threads:[~2018-10-03 17:12 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-02 14:58 [PATCH 0/2] Clean up leaks in commit-graph.c Derrick Stolee via GitGitGadget
2018-10-02 14:58 ` [PATCH 1/2] commit-graph: clean up leaked memory during write Derrick Stolee via GitGitGadget
2018-10-02 15:40 ` Martin Ågren
2018-10-02 17:59 ` Stefan Beller
2018-10-02 19:08 ` Martin Ågren
2018-10-02 19:44 ` Stefan Beller
2018-10-02 22:34 ` Jeff King
2018-10-02 22:44 ` Stefan Beller
2018-10-03 12:04 ` Derrick Stolee
2018-10-03 15:36 ` [PATCH 0/2] commit-graph: more leak fixes Martin Ågren
2018-10-03 15:36 ` [PATCH 1/2] commit-graph: free `struct packed_git` after closing it Martin Ågren
2018-10-03 15:36 ` [PATCH 2/2] builtin/commit-graph.c: UNLEAK variables Martin Ågren
2018-10-03 16:19 ` [PATCH 0/2] commit-graph: more leak fixes Derrick Stolee
2018-10-03 16:24 ` Martin Ågren
2018-10-02 22:37 ` [PATCH 1/2] commit-graph: clean up leaked memory during write Jeff King
2018-10-02 14:58 ` [PATCH 2/2] commit-graph: reduce initial oid allocation Derrick Stolee via GitGitGadget
2018-10-03 17:12 ` Derrick Stolee via GitGitGadget [this message]
2018-10-03 17:12 ` [PATCH v2 1/3] commit-graph: clean up leaked memory during write Derrick Stolee via GitGitGadget
2018-10-03 17:12 ` [PATCH v2 2/3] builtin/commit-graph.c: UNLEAK variables Martin Ågren via GitGitGadget
2018-10-03 17:12 ` [PATCH v2 3/3] commit-graph: reduce initial oid allocation Derrick Stolee via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.42.v2.git.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).