From: Elijah Newren <newren@xxxxxxxxx>
Simplify code maintenance by removing the ability to toggle between
usage of memory pools and direct allocations. This allows us to also
remove paths_to_free since it was solely about bookkeeping to make sure
we freed the necessary paths, and allows us to remove some auxiliary
functions.
Suggested-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
---
merge-ort.c | 209 ++++++++++++----------------------------------------
1 file changed, 47 insertions(+), 162 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 86ab8f60121..88ade50f4ed 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -37,8 +37,6 @@
#include "unpack-trees.h"
#include "xdiff-interface.h"
-#define USE_MEMORY_POOL 1 /* faster, but obscures memory leak hunting */
-
/*
* We have many arrays of size 3. Whenever we have such an array, the
* indices refer to one of the sides of the three-way merge. This is so
@@ -305,8 +303,6 @@ struct merge_options_internal {
* * these keys serve to intern all the path strings, which allows
* us to do pointer comparison on directory names instead of
* strcmp; we just have to be careful to use the interned strings.
- * (Technically paths_to_free may track some strings that were
- * removed from froms paths.)
*
* The values of paths:
* * either a pointer to a merged_info, or a conflict_info struct
@@ -349,18 +345,7 @@ struct merge_options_internal {
* freed together too. Using a memory pool for these provides a
* nice speedup.
*/
- struct mem_pool internal_pool;
- struct mem_pool *pool; /* NULL, or pointer to internal_pool */
-
- /*
- * paths_to_free: additional list of strings to free
- *
- * If keys are removed from "paths", they are added to paths_to_free
- * to ensure they are later freed. We avoid free'ing immediately since
- * other places (e.g. conflict_info.pathnames[]) may still be
- * referencing these paths.
- */
- struct string_list paths_to_free;
+ struct mem_pool pool;
/*
* output: special messages and conflict notices for various paths
@@ -539,19 +524,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
void (*strset_clear_func)(struct strset *) =
reinitialize ? strset_partial_clear : strset_clear;
- if (opti->pool)
- strmap_clear_func(&opti->paths, 0);
- else {
- /*
- * We marked opti->paths with strdup_strings = 0, so that
- * we wouldn't have to make another copy of the fullpath
- * created by make_traverse_path from setup_path_info().
- * But, now that we've used it and have no other references
- * to these strings, it is time to deallocate them.
- */
- free_strmap_strings(&opti->paths);
- strmap_clear_func(&opti->paths, 1);
- }
+ strmap_clear_func(&opti->paths, 0);
/*
* All keys and values in opti->conflicted are a subset of those in
@@ -560,20 +533,6 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
*/
strmap_clear_func(&opti->conflicted, 0);
- if (!opti->pool) {
- /*
- * opti->paths_to_free is similar to opti->paths; we
- * created it with strdup_strings = 0 to avoid making
- * _another_ copy of the fullpath but now that we've used
- * it and have no other references to these strings, it is
- * time to deallocate them. We do so by temporarily
- * setting strdup_strings to 1.
- */
- opti->paths_to_free.strdup_strings = 1;
- string_list_clear(&opti->paths_to_free, 0);
- opti->paths_to_free.strdup_strings = 0;
- }
-
if (opti->attr_index.cache_nr) /* true iff opt->renormalize */
discard_index(&opti->attr_index);
@@ -623,11 +582,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
strmap_clear(&opti->output, 0);
}
-#if USE_MEMORY_POOL
- mem_pool_discard(&opti->internal_pool, 0);
- if (!reinitialize)
- opti->pool = NULL;
-#endif
+ mem_pool_discard(&opti->pool, 0);
/* Clean out callback_data as well. */
FREE_AND_NULL(renames->callback_data);
@@ -693,12 +648,9 @@ static void path_msg(struct merge_options *opt,
static struct diff_filespec *pool_alloc_filespec(struct mem_pool *pool,
const char *path)
{
+ /* Similar to alloc_filespec(), but allocate from pool and reuse path */
struct diff_filespec *spec;
- if (!pool)
- return alloc_filespec(path);
-
- /* Similar to alloc_filespec, but allocate from pool and reuse path */
spec = mem_pool_calloc(pool, 1, sizeof(*spec));
spec->path = (char*)path; /* spec won't modify it */
@@ -712,12 +664,9 @@ static struct diff_filepair *pool_diff_queue(struct mem_pool *pool,
struct diff_filespec *one,
struct diff_filespec *two)
{
+ /* Same code as diff_queue(), except allocate from pool */
struct diff_filepair *dp;
- if (!pool)
- return diff_queue(queue, one, two);
-
- /* Same code as diff_queue, except allocate from pool */
dp = mem_pool_calloc(pool, 1, sizeof(*dp));
dp->one = one;
dp->two = two;
@@ -726,27 +675,6 @@ static struct diff_filepair *pool_diff_queue(struct mem_pool *pool,
return dp;
}
-static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
-{
- if (!pool)
- return xcalloc(count, size);
- return mem_pool_calloc(pool, count, size);
-}
-
-static void *pool_alloc(struct mem_pool *pool, size_t size)
-{
- if (!pool)
- return xmalloc(size);
- return mem_pool_alloc(pool, size);
-}
-
-static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)
-{
- if (!pool)
- return xstrndup(str, len);
- return mem_pool_strndup(pool, str, len);
-}
-
/* add a string to a strbuf, but converting "/" to "_" */
static void add_flattened_path(struct strbuf *out, const char *s)
{
@@ -875,9 +803,9 @@ static void setup_path_info(struct merge_options *opt,
assert(!df_conflict || !resolved); /* df_conflict implies !resolved */
assert(resolved == (merged_version != NULL));
- mi = pool_calloc(opt->priv->pool, 1,
- resolved ? sizeof(struct merged_info) :
- sizeof(struct conflict_info));
+ mi = mem_pool_calloc(&opt->priv->pool, 1,
+ resolved ? sizeof(struct merged_info) :
+ sizeof(struct conflict_info));
mi->directory_name = current_dir_name;
mi->basename_offset = current_dir_name_len;
mi->clean = !!resolved;
@@ -924,7 +852,6 @@ static void add_pair(struct merge_options *opt,
unsigned dir_rename_mask)
{
struct diff_filespec *one, *two;
- struct mem_pool *pool = opt->priv->pool;
struct rename_info *renames = &opt->priv->renames;
int names_idx = is_add ? side : 0;
@@ -975,11 +902,11 @@ static void add_pair(struct merge_options *opt,
return;
}
- one = pool_alloc_filespec(pool, pathname);
- two = pool_alloc_filespec(pool, pathname);
+ one = pool_alloc_filespec(&opt->priv->pool, pathname);
+ two = pool_alloc_filespec(&opt->priv->pool, pathname);
fill_filespec(is_add ? two : one,
&names[names_idx].oid, 1, names[names_idx].mode);
- pool_diff_queue(pool, &renames->pairs[side], one, two);
+ pool_diff_queue(&opt->priv->pool, &renames->pairs[side], one, two);
}
static void collect_rename_info(struct merge_options *opt,
@@ -1170,7 +1097,7 @@ static int collect_merge_info_callback(int n,
len = traverse_path_len(info, p->pathlen);
/* +1 in both of the following lines to include the NUL byte */
- fullpath = pool_alloc(opt->priv->pool, len + 1);
+ fullpath = mem_pool_alloc(&opt->priv->pool, len + 1);
make_traverse_path(fullpath, len + 1, info, p->path, p->pathlen);
/*
@@ -1425,7 +1352,7 @@ static int handle_deferred_entries(struct merge_options *opt,
copy = renames->deferred[side].possible_trivial_merges;
strintmap_init_with_options(&renames->deferred[side].possible_trivial_merges,
0,
- opt->priv->pool,
+ &opt->priv->pool,
0);
strintmap_for_each_entry(©, &iter, entry) {
const char *path = entry->key;
@@ -2377,21 +2304,17 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
VERIFY_CI(ci);
/* Find parent directories missing from opt->priv->paths */
- if (opt->priv->pool) {
- cur_path = mem_pool_strdup(opt->priv->pool, new_path);
- free((char*)new_path);
- new_path = (char *)cur_path;
- } else {
- cur_path = new_path;
- }
+ cur_path = mem_pool_strdup(&opt->priv->pool, new_path);
+ free((char*)new_path);
+ new_path = (char *)cur_path;
while (1) {
/* Find the parent directory of cur_path */
char *last_slash = strrchr(cur_path, '/');
if (last_slash) {
- parent_name = pool_strndup(opt->priv->pool,
- cur_path,
- last_slash - cur_path);
+ parent_name = mem_pool_strndup(&opt->priv->pool,
+ cur_path,
+ last_slash - cur_path);
} else {
parent_name = opt->priv->toplevel_dir;
break;
@@ -2400,8 +2323,6 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
/* Look it up in opt->priv->paths */
entry = strmap_get_entry(&opt->priv->paths, parent_name);
if (entry) {
- if (!opt->priv->pool)
- free((char*)parent_name);
parent_name = entry->key; /* reuse known pointer */
break;
}
@@ -2428,16 +2349,6 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
parent_name = cur_dir;
}
- if (!opt->priv->pool) {
- /*
- * We are removing old_path from opt->priv->paths.
- * old_path also will eventually need to be freed, but it
- * may still be used by e.g. ci->pathnames. So, store it
- * in another string-list for now.
- */
- string_list_append(&opt->priv->paths_to_free, old_path);
- }
-
assert(ci->filemask == 2 || ci->filemask == 4);
assert(ci->dirmask == 0);
strmap_remove(&opt->priv->paths, old_path, 0);
@@ -2471,8 +2382,6 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
new_ci->stages[index].mode = ci->stages[index].mode;
oidcpy(&new_ci->stages[index].oid, &ci->stages[index].oid);
- if (!opt->priv->pool)
- free(ci);
ci = new_ci;
}
@@ -2888,7 +2797,6 @@ static void use_cached_pairs(struct merge_options *opt,
{
struct hashmap_iter iter;
struct strmap_entry *entry;
- struct mem_pool *pool = opt->priv->pool;
/*
* Add to side_pairs all entries from renames->cached_pairs[side_index].
@@ -2900,30 +2808,24 @@ static void use_cached_pairs(struct merge_options *opt,
const char *new_name = entry->value;
if (!new_name)
new_name = old_name;
- if (pool) {
- /*
- * cached_pairs has _copies* of old_name and new_name,
- * because it has to persist across merges. When
- * pool != NULL
- * pool_alloc_filespec() will just re-use the existing
- * filenames, which will also get re-used by
- * opt->priv->paths if they become renames, and then
- * get freed at the end of the merge, leaving the copy
- * in cached_pairs dangling. Avoid this by making a
- * copy here.
- *
- * When pool == NULL, pool_alloc_filespec() calls
- * alloc_filespec(), which makes a copy; we don't want
- * to add another.
- */
- old_name = mem_pool_strdup(pool, old_name);
- new_name = mem_pool_strdup(pool, new_name);
- }
+
+ /*
+ * cached_pairs has *copies* of old_name and new_name,
+ * because it has to persist across merges. Since
+ * pool_alloc_filespec() will just re-use the existing
+ * filenames, which will also get re-used by
+ * opt->priv->paths if they become renames, and then
+ * get freed at the end of the merge, that would leave
+ * the copy in cached_pairs dangling. Avoid this by
+ * making a copy here.
+ */
+ old_name = mem_pool_strdup(&opt->priv->pool, old_name);
+ new_name = mem_pool_strdup(&opt->priv->pool, new_name);
/* We don't care about oid/mode, only filenames and status */
- one = pool_alloc_filespec(pool, old_name);
- two = pool_alloc_filespec(pool, new_name);
- pool_diff_queue(pool, pairs, one, two);
+ one = pool_alloc_filespec(&opt->priv->pool, old_name);
+ two = pool_alloc_filespec(&opt->priv->pool, new_name);
+ pool_diff_queue(&opt->priv->pool, pairs, one, two);
pairs->queue[pairs->nr-1]->status = entry->value ? 'R' : 'D';
}
}
@@ -3031,7 +2933,7 @@ static int detect_regular_renames(struct merge_options *opt,
diff_queued_diff = renames->pairs[side_index];
trace2_region_enter("diff", "diffcore_rename", opt->repo);
diffcore_rename_extended(&diff_opts,
- opt->priv->pool,
+ &opt->priv->pool,
&renames->relevant_sources[side_index],
&renames->dirs_removed[side_index],
&renames->dir_rename_count[side_index],
@@ -3082,7 +2984,7 @@ static int collect_renames(struct merge_options *opt,
if (p->status != 'A' && p->status != 'R') {
possibly_cache_new_pair(renames, p, side_index, NULL);
- pool_diff_free_filepair(opt->priv->pool, p);
+ pool_diff_free_filepair(&opt->priv->pool, p);
continue;
}
@@ -3095,7 +2997,7 @@ static int collect_renames(struct merge_options *opt,
possibly_cache_new_pair(renames, p, side_index, new_path);
if (p->status != 'R' && !new_path) {
- pool_diff_free_filepair(opt->priv->pool, p);
+ pool_diff_free_filepair(&opt->priv->pool, p);
continue;
}
@@ -3213,7 +3115,7 @@ cleanup:
side_pairs = &renames->pairs[s];
for (i = 0; i < side_pairs->nr; ++i) {
struct diff_filepair *p = side_pairs->queue[i];
- pool_diff_free_filepair(opt->priv->pool, p);
+ pool_diff_free_filepair(&opt->priv->pool, p);
}
}
@@ -3226,7 +3128,7 @@ simple_cleanup:
if (combined.nr) {
int i;
for (i = 0; i < combined.nr; i++)
- pool_diff_free_filepair(opt->priv->pool,
+ pool_diff_free_filepair(&opt->priv->pool,
combined.queue[i]);
free(combined.queue);
}
@@ -3701,7 +3603,7 @@ static void process_entry(struct merge_options *opt,
* the directory to remain here, so we need to move this
* path to some new location.
*/
- new_ci = pool_calloc(opt->priv->pool, 1, sizeof(*new_ci));
+ new_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*new_ci));
/* We don't really want new_ci->merged.result copied, but it'll
* be overwritten below so it doesn't matter. We also don't
@@ -3794,7 +3696,8 @@ static void process_entry(struct merge_options *opt,
const char *a_path = NULL, *b_path = NULL;
int rename_a = 0, rename_b = 0;
- new_ci = pool_alloc(opt->priv->pool, sizeof(*new_ci));
+ new_ci = mem_pool_alloc(&opt->priv->pool,
+ sizeof(*new_ci));
if (S_ISREG(a_mode))
rename_a = 1;
@@ -3863,19 +3766,8 @@ static void process_entry(struct merge_options *opt,
b_path = path;
strmap_put(&opt->priv->paths, b_path, new_ci);
- if (rename_a && rename_b) {
+ if (rename_a && rename_b)
strmap_remove(&opt->priv->paths, path, 0);
- /*
- * We removed path from opt->priv->paths. path
- * will also eventually need to be freed if not
- * part of a memory pool...but it may still be
- * used by e.g. ci->pathnames. So, store it in
- * another string-list for now in that case.
- */
- if (!opt->priv->pool)
- string_list_append(&opt->priv->paths_to_free,
- path);
- }
/*
* Do special handling for b_path since process_entry()
@@ -4482,13 +4374,8 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
/* Initialization of various renames fields */
renames = &opt->priv->renames;
-#if USE_MEMORY_POOL
- mem_pool_init(&opt->priv->internal_pool, 0);
- opt->priv->pool = &opt->priv->internal_pool;
-#else
- opt->priv->pool = NULL;
-#endif
- pool = opt->priv->pool;
+ mem_pool_init(&opt->priv->pool, 0);
+ pool = &opt->priv->pool;
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
strintmap_init_with_options(&renames->dirs_removed[i],
NOT_RELEVANT, pool, 0);
@@ -4525,15 +4412,13 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
* Although we initialize opt->priv->paths with strdup_strings=0,
* that's just to avoid making yet another copy of an allocated
* string. Putting the entry into paths means we are taking
- * ownership, so we will later free it. paths_to_free is similar.
+ * ownership, so we will later free it.
*
* In contrast, conflicted just has a subset of keys from paths, so
* we don't want to free those (it'd be a duplicate free).
*/
strmap_init_with_options(&opt->priv->paths, pool, 0);
strmap_init_with_options(&opt->priv->conflicted, pool, 0);
- if (!opt->priv->pool)
- string_list_init_nodup(&opt->priv->paths_to_free);
/*
* keys & strbufs in output will sometimes need to outlive "paths",
--
gitgitgadget
