Hi,
Pete Wyckoff wrote:
> ==18789== HEAP SUMMARY:
> ==18789== in use at exit: 511,786,999 bytes in 3,954,180 blocks
> ==18789== total heap usage: 6,352,564 allocs, 2,398,384 frees, 2,610,529,433 bytes allocated
This is disturbing, to say the least. Let me try to chomp through the
Valgrind output to see what I understand.
[Re-ordering for convenience]
> [...]
Ignoring small losses from strdup(), parse_args() and other unrelated
things for the moment.
> ==18789== 16 bytes in 1 blocks are definitely lost in loss record 14 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x49578D: commit_list_insert (commit.c:356)
> ==18789== by 0x495886: commit_list_insert_by_date (commit.c:390)
> ==18789== by 0x4F51BC: commit_list_insert_by_date_cached (revision.c:508)
> ==18789== by 0x4F5344: add_parents_to_list (revision.c:550)
> ==18789== by 0x4F5C62: limit_list (revision.c:836)
> ==18789== by 0x4FA49B: prepare_revision_walk (revision.c:2068)
> ==18789== by 0x47522F: prepare_revs (revert.c:650)
> ==18789== by 0x475B6D: walk_revs_populate_todo (revert.c:855)
> ==18789== by 0x4766BF: pick_revisions (revert.c:1123)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789==
> ==18789== 144 (16 direct, 128 indirect) bytes in 1 blocks are definitely lost in loss record 41 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x47536C: commit_list_append (revert.c:692)
> ==18789== by 0x475B8A: walk_revs_populate_todo (revert.c:859)
> ==18789== by 0x4766BF: pick_revisions (revert.c:1123)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789== by 0x40558E: run_argv (git.c:513)
> ==18789== by 0x40571B: main (git.c:588)
Ugh, I never free the commit_list I build up in commit_list_append().
Rough sketch of fix (caution: untested):
Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
diff --git a/builtin/revert.c b/builtin/revert.c
index 0d8020c..76be0e3 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -770,8 +770,11 @@ static int parse_insn_buffer(char *buf, struct
commit_list **todo_list,
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
commit = parse_insn_line(p, eol, opts);
- if (!commit)
+ if (!commit) {
+ if (*todo_list)
+ free_commit_list(*todo_list);
return error(_("Could not parse line %d."), i);
+ }
next = commit_list_append(commit, next);
p = *eol ? eol + 1 : eol;
}
@@ -1020,14 +1023,17 @@ static int pick_commits(struct commit_list
*todo_list, struct replay_opts *opts)
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
res = do_pick_commit(cur->item, opts);
- if (res)
+ if (res) {
+ free_commit_list(todo_list);
return res;
+ }
}
/*
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
*/
+ free_commit_list(todo_list);
remove_sequencer_state();
return 0;
}
--
> ==18789== 1,728 bytes in 9 blocks are definitely lost in loss record 67 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x4C27927: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FFCD: xrealloc (wrapper.c:82)
> ==18789== by 0x488D5B: handle_attr_line (attr.c:325)
> ==18789== by 0x488DDD: read_attr_from_array (attr.c:340)
> ==18789== by 0x48924E: bootstrap_attr_stack (attr.c:501)
> ==18789== by 0x48940F: prepare_attr_stack (attr.c:568)
> ==18789== by 0x489A27: collect_all_attrs (attr.c:725)
> ==18789== by 0x489AED: git_check_attr (attr.c:739)
> ==18789== by 0x49E314: convert_attrs (convert.c:730)
> ==18789== by 0x49F230: get_stream_filter (convert.c:1247)
> ==18789== by 0x4B69B1: write_entry (entry.c:191)
> ==18789==
> ==18789== 7,726 (4,320 direct, 3,406 indirect) bytes in 9 blocks are definitely lost in loss record 77 of 97
> ==18789== at 0x4C27882: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FFCD: xrealloc (wrapper.c:82)
> ==18789== by 0x488D5B: handle_attr_line (attr.c:325)
> ==18789== by 0x488E86: read_attr_from_file (attr.c:358)
> ==18789== by 0x489130: read_attr (attr.c:428)
> ==18789== by 0x489335: bootstrap_attr_stack (attr.c:525)
> ==18789== by 0x48940F: prepare_attr_stack (attr.c:568)
> ==18789== by 0x489A27: collect_all_attrs (attr.c:725)
> ==18789== by 0x489AED: git_check_attr (attr.c:739)
> ==18789== by 0x49E314: convert_attrs (convert.c:730)
> ==18789== by 0x49F230: get_stream_filter (convert.c:1247)
> ==18789== by 0x4B69B1: write_entry (entry.c:191)
Interesting- I wonder where .gitattributes parsing code fits into all
this. The purpose of bootstrap _attr_stack() is to populate
attr_stack for its callers. Lots of memory allocation happening in
handle_attr_line() -- that information is returned to
bootstrap_attr_stack(). We have to keep backtracking until that
information is provably useless and free it. Hm, convert_attrs() (in
convert.c) is a common caller in both cases, but the populated
attr_stack is local to attr.c; I wonder if this is the problem. If my
hunch is right, it should be a trivial fix (caution: untested):
Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
diff --git a/attr.c b/attr.c
index 76b079f..12e3824 100644
--- a/attr.c
+++ b/attr.c
@@ -745,6 +745,7 @@ int git_check_attr(const char *path, int num,
struct git_attr_check *check)
check[i].value = value;
}
+ drop_attr_stack();
return 0;
}
--
> ==18789== 1,107 bytes in 9 blocks are definitely lost in loss record 62 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x519DCD: setup_unpack_trees_porcelain (unpack-trees.c:79)
> ==18789== by 0x4C8160: git_merge_trees (merge-recursive.c:235)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 1,143 bytes in 9 blocks are definitely lost in loss record 63 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x519E8F: setup_unpack_trees_porcelain (unpack-trees.c:82)
> ==18789== by 0x4C8160: git_merge_trees (merge-recursive.c:235)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 1,269 bytes in 9 blocks are definitely lost in loss record 64 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x519CE0: setup_unpack_trees_porcelain (unpack-trees.c:66)
> ==18789== by 0x4C8160: git_merge_trees (merge-recursive.c:235)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 704 bytes in 8 blocks are definitely lost in loss record 58 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4A46BD: diff_cache (diff-lib.c:474)
> ==18789== by 0x4A46FC: run_diff_index (diff-lib.c:482)
> ==18789== by 0x4A48DA: index_differs_from (diff-lib.c:517)
> ==18789== by 0x474AC2: do_pick_commit (revert.c:503)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789==
> ==18789== 1,316 bytes in 10 blocks are possibly lost in loss record 65 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789==
> ==18789== 2,376 bytes in 27 blocks are definitely lost in loss record 72 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4C81C2: git_merge_trees (merge-recursive.c:241)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789==
> ==18789== 744,353 bytes in 7,493 blocks are definitely lost in loss record 88 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4A46BD: diff_cache (diff-lib.c:474)
> ==18789== by 0x4A46FC: run_diff_index (diff-lib.c:482)
> ==18789==
> ==18789== 1,865,981 bytes in 18,806 blocks are definitely lost in loss record 90 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4C81C2: git_merge_trees (merge-recursive.c:241)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789==
> ==18789== 313,333,787 bytes in 2,509,382 blocks are definitely lost in loss record 97 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
So many issues with unpack-trees! Let's try to look for some common
patterns, and see where the problems are occurring. I can see two
distinct patterns: backtraces that include
setup_unpack_trees_porcelain() and those that include
create_ce_entry(). Let's go with create_ce_entry() first: it
allocates quite a bit of memory according to cache.h:247. And it
returns the memory back to unpack_nondirectories(). What's worse?
unpack_nondirectories() calls create_ce_entry() in a loop and stuffs
all this new memory into a cache_entry provided by unpack_callback().
In the end, all the calls boil down to git_merge_trees(): I don't see
the ton of memory allocated higher in the callstack being passed down
in any way. I don't understand- why doesn't unpack_callback() clean
up the memory in "struct cache_entry *src" after it's done?
> ==18789== 281 bytes in 2 blocks are possibly lost in loss record 51 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x4E483E: create_from_disk (read-cache.c:1255)
> ==18789== by 0x4E4B7F: read_index_from (read-cache.c:1328)
> ==18789== by 0x4E4758: read_index (read-cache.c:1224)
> ==18789== by 0x47469F: do_recursive_merge (revert.c:411)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 8,386,453 (1,031,576 direct, 7,354,877 indirect) bytes in 1 blocks are definitely lost in loss record 91 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x4E4B38: read_index_from (read-cache.c:1319)
> ==18789== by 0x4E4758: read_index (read-cache.c:1224)
> ==18789== by 0x4DB50F: read_index_preload (preload-index.c:105)
> ==18789== by 0x4752A1: read_and_refresh_cache (revert.c:661)
> ==18789== by 0x47657B: pick_revisions (revert.c:1081)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789== by 0x40558E: run_argv (git.c:513)
> ==18789== by 0x40571B: main (git.c:588)
> ==18789==
> ==18789== 69,782,106 (6,793,984 direct, 62,988,122 indirect) bytes in 8 blocks are definitely lost in loss record 94 of 97
> ==18789== at 0x4C27882: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FFCD: xrealloc (wrapper.c:82)
> ==18789== by 0x4E3D7C: add_index_entry (read-cache.c:976)
> ==18789== by 0x519FFE: add_entry (unpack-trees.c:119)
> ==18789== by 0x51D0A6: merged_entry (unpack-trees.c:1501)
> ==18789== by 0x51D444: threeway_merge (unpack-trees.c:1615)
> ==18789== by 0x51A645: call_unpack_fn (unpack-trees.c:289)
> ==18789== by 0x51B1E2: unpack_nondirectories (unpack-trees.c:586)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789==
> ==18789== 105,619,300 (9,284,184 direct, 96,335,116 indirect) bytes in 9 blocks are definitely lost in loss record 96 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x4E4B38: read_index_from (read-cache.c:1319)
> ==18789== by 0x4E4758: read_index (read-cache.c:1224)
> ==18789== by 0x47469F: do_recursive_merge (revert.c:411)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789== by 0x40558E: run_argv (git.c:513)
Will investigate read-cache the context of Duy's comment- it looks
like I have to read through some more history to understand what's
going on.
Thanks.
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html