merge-ort: remove compile-time ability to turn off usage of memory pools
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@peff.net> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren committed
Jul 31, 2021 at 17:27 UTC
62a15162fe60f11c515bec116c9a1f58173a032f
1 file changed
+47
-162
merge-ort.c
+47
-162
@@ -37,8 +37,6 @@
37
#include "unpack-trees.h"
38
#include "xdiff-interface.h"
39
40
-#define USE_MEMORY_POOL 1 /* faster, but obscures memory leak hunting */
41
-
40
/*
41
* We have many arrays of size 3. Whenever we have such an array, the
42
* indices refer to one of the sides of the three-way merge. This is so
@@ -305,8 +303,6 @@ struct merge_options_internal {
303
* * these keys serve to intern all the path strings, which allows
304
* us to do pointer comparison on directory names instead of
305
* strcmp; we just have to be careful to use the interned strings.
308
- * (Technically paths_to_free may track some strings that were
309
- * removed from froms paths.)
306
*
307
* The values of paths:
308
* * either a pointer to a merged_info, or a conflict_info struct
@@ -349,18 +345,7 @@ struct merge_options_internal {
345
* freed together too. Using a memory pool for these provides a
346
* nice speedup.
347
*/
352
- struct mem_pool internal_pool;
353
- struct mem_pool *pool; /* NULL, or pointer to internal_pool */
354
-
355
- /*
356
- * paths_to_free: additional list of strings to free
357
- *
358
- * If keys are removed from "paths", they are added to paths_to_free
359
- * to ensure they are later freed. We avoid free'ing immediately since
360
- * other places (e.g. conflict_info.pathnames[]) may still be
361
- * referencing these paths.
362
- */
363
- struct string_list paths_to_free;
348
+ struct mem_pool pool;
349
350
/*
351
* 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,
524
void (*strset_clear_func)(struct strset *) =
525
reinitialize ? strset_partial_clear : strset_clear;
526
542
- if (opti->pool)
543
- strmap_clear_func(&opti->paths, 0);
544
- else {
545
- /*
546
- * We marked opti->paths with strdup_strings = 0, so that
547
- * we wouldn't have to make another copy of the fullpath
548
- * created by make_traverse_path from setup_path_info().
549
- * But, now that we've used it and have no other references
550
- * to these strings, it is time to deallocate them.
551
- */
552
- free_strmap_strings(&opti->paths);
553
- strmap_clear_func(&opti->paths, 1);
554
- }
527
+ strmap_clear_func(&opti->paths, 0);
528
529
/*
530
* 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,
533
*/
534
strmap_clear_func(&opti->conflicted, 0);
535
563
- if (!opti->pool) {
564
- /*
565
- * opti->paths_to_free is similar to opti->paths; we
566
- * created it with strdup_strings = 0 to avoid making
567
- * _another_ copy of the fullpath but now that we've used
568
- * it and have no other references to these strings, it is
569
- * time to deallocate them. We do so by temporarily
570
- * setting strdup_strings to 1.
571
- */
572
- opti->paths_to_free.strdup_strings = 1;
573
- string_list_clear(&opti->paths_to_free, 0);
574
- opti->paths_to_free.strdup_strings = 0;
575
- }
576
-
536
if (opti->attr_index.cache_nr) /* true iff opt->renormalize */
537
discard_index(&opti->attr_index);
538
@@ -623,11 +582,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
582
strmap_clear(&opti->output, 0);
583
}
584
626
-#if USE_MEMORY_POOL
627
- mem_pool_discard(&opti->internal_pool, 0);
628
- if (!reinitialize)
629
- opti->pool = NULL;
630
-#endif
585
+ mem_pool_discard(&opti->pool, 0);
586
587
/* Clean out callback_data as well. */
588
FREE_AND_NULL(renames->callback_data);
@@ -693,12 +648,9 @@ static void path_msg(struct merge_options *opt,
648
static struct diff_filespec *pool_alloc_filespec(struct mem_pool *pool,
649
const char *path)
650
{
651
+ /* Similar to alloc_filespec(), but allocate from pool and reuse path */
652
struct diff_filespec *spec;
653
698
- if (!pool)
699
- return alloc_filespec(path);
700
-
701
- /* Similar to alloc_filespec, but allocate from pool and reuse path */
654
spec = mem_pool_calloc(pool, 1, sizeof(*spec));
655
spec->path = (char*)path; /* spec won't modify it */
656
@@ -712,12 +664,9 @@ static struct diff_filepair *pool_diff_queue(struct mem_pool *pool,
664
struct diff_filespec *one,
665
struct diff_filespec *two)
666
{
667
+ /* Same code as diff_queue(), except allocate from pool */
668
struct diff_filepair *dp;
669
717
- if (!pool)
718
- return diff_queue(queue, one, two);
719
-
720
- /* Same code as diff_queue, except allocate from pool */
670
dp = mem_pool_calloc(pool, 1, sizeof(*dp));
671
dp->one = one;
672
dp->two = two;
@@ -726,27 +675,6 @@ static struct diff_filepair *pool_diff_queue(struct mem_pool *pool,
675
return dp;
676
}
677
729
-static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
730
-{
731
- if (!pool)
732
- return xcalloc(count, size);
733
- return mem_pool_calloc(pool, count, size);
734
-}
735
-
736
-static void *pool_alloc(struct mem_pool *pool, size_t size)
737
-{
738
- if (!pool)
739
- return xmalloc(size);
740
- return mem_pool_alloc(pool, size);
741
-}
742
-
743
-static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)
744
-{
745
- if (!pool)
746
- return xstrndup(str, len);
747
- return mem_pool_strndup(pool, str, len);
748
-}
749
-
678
/* add a string to a strbuf, but converting "/" to "_" */
679
static void add_flattened_path(struct strbuf *out, const char *s)
680
{
@@ -875,9 +803,9 @@ static void setup_path_info(struct merge_options *opt,
803
assert(!df_conflict || !resolved); /* df_conflict implies !resolved */
804
assert(resolved == (merged_version != NULL));
805
878
- mi = pool_calloc(opt->priv->pool, 1,
879
- resolved ? sizeof(struct merged_info) :
880
- sizeof(struct conflict_info));
806
+ mi = mem_pool_calloc(&opt->priv->pool, 1,
807
+ resolved ? sizeof(struct merged_info) :
808
+ sizeof(struct conflict_info));
809
mi->directory_name = current_dir_name;
810
mi->basename_offset = current_dir_name_len;
811
mi->clean = !!resolved;
@@ -924,7 +852,6 @@ static void add_pair(struct merge_options *opt,
852
unsigned dir_rename_mask)
853
{
854
struct diff_filespec *one, *two;
927
- struct mem_pool *pool = opt->priv->pool;
855
struct rename_info *renames = &opt->priv->renames;
856
int names_idx = is_add ? side : 0;
857
@@ -975,11 +902,11 @@ static void add_pair(struct merge_options *opt,
902
return;
903
}
904
978
- one = pool_alloc_filespec(pool, pathname);
979
- two = pool_alloc_filespec(pool, pathname);
905
+ one = pool_alloc_filespec(&opt->priv->pool, pathname);
906
+ two = pool_alloc_filespec(&opt->priv->pool, pathname);
907
fill_filespec(is_add ? two : one,
908
&names[names_idx].oid, 1, names[names_idx].mode);
982
- pool_diff_queue(pool, &renames->pairs[side], one, two);
909
+ pool_diff_queue(&opt->priv->pool, &renames->pairs[side], one, two);
910
}
911
912
static void collect_rename_info(struct merge_options *opt,
@@ -1170,7 +1097,7 @@ static int collect_merge_info_callback(int n,
1097
len = traverse_path_len(info, p->pathlen);
1098
1099
/* +1 in both of the following lines to include the NUL byte */
1173
- fullpath = pool_alloc(opt->priv->pool, len + 1);
1100
+ fullpath = mem_pool_alloc(&opt->priv->pool, len + 1);
1101
make_traverse_path(fullpath, len + 1, info, p->path, p->pathlen);
1102
1103
/*
@@ -1425,7 +1352,7 @@ static int handle_deferred_entries(struct merge_options *opt,
1352
copy = renames->deferred[side].possible_trivial_merges;
1353
strintmap_init_with_options(&renames->deferred[side].possible_trivial_merges,
1354
0,
1428
- opt->priv->pool,
1355
+ &opt->priv->pool,
1356
0);
1357
strintmap_for_each_entry(©, &iter, entry) {
1358
const char *path = entry->key;
@@ -2377,21 +2304,17 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
2304
VERIFY_CI(ci);
2305
2306
/* Find parent directories missing from opt->priv->paths */
2380
- if (opt->priv->pool) {
2381
- cur_path = mem_pool_strdup(opt->priv->pool, new_path);
2382
- free((char*)new_path);
2383
- new_path = (char *)cur_path;
2384
- } else {
2385
- cur_path = new_path;
2386
- }
2307
+ cur_path = mem_pool_strdup(&opt->priv->pool, new_path);
2308
+ free((char*)new_path);
2309
+ new_path = (char *)cur_path;
2310
2311
while (1) {
2312
/* Find the parent directory of cur_path */
2313
char *last_slash = strrchr(cur_path, '/');
2314
if (last_slash) {
2392
- parent_name = pool_strndup(opt->priv->pool,
2393
- cur_path,
2394
- last_slash - cur_path);
2315
+ parent_name = mem_pool_strndup(&opt->priv->pool,
2316
+ cur_path,
2317
+ last_slash - cur_path);
2318
} else {
2319
parent_name = opt->priv->toplevel_dir;
2320
break;
@@ -2400,8 +2323,6 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
2323
/* Look it up in opt->priv->paths */
2324
entry = strmap_get_entry(&opt->priv->paths, parent_name);
2325
if (entry) {
2403
- if (!opt->priv->pool)
2404
- free((char*)parent_name);
2326
parent_name = entry->key; /* reuse known pointer */
2327
break;
2328
}
@@ -2428,16 +2349,6 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
2349
parent_name = cur_dir;
2350
}
2351
2431
- if (!opt->priv->pool) {
2432
- /*
2433
- * We are removing old_path from opt->priv->paths.
2434
- * old_path also will eventually need to be freed, but it
2435
- * may still be used by e.g. ci->pathnames. So, store it
2436
- * in another string-list for now.
2437
- */
2438
- string_list_append(&opt->priv->paths_to_free, old_path);
2439
- }
2440
-
2352
assert(ci->filemask == 2 || ci->filemask == 4);
2353
assert(ci->dirmask == 0);
2354
strmap_remove(&opt->priv->paths, old_path, 0);
@@ -2471,8 +2382,6 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
2382
new_ci->stages[index].mode = ci->stages[index].mode;
2383
oidcpy(&new_ci->stages[index].oid, &ci->stages[index].oid);
2384
2474
- if (!opt->priv->pool)
2475
- free(ci);
2385
ci = new_ci;
2386
}
2387
@@ -2888,7 +2797,6 @@ static void use_cached_pairs(struct merge_options *opt,
2797
{
2798
struct hashmap_iter iter;
2799
struct strmap_entry *entry;
2891
- struct mem_pool *pool = opt->priv->pool;
2800
2801
/*
2802
* 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,
2808
const char *new_name = entry->value;
2809
if (!new_name)
2810
new_name = old_name;
2903
- if (pool) {
2904
- /*
2905
- * cached_pairs has _copies* of old_name and new_name,
2906
- * because it has to persist across merges. When
2907
- * pool != NULL
2908
- * pool_alloc_filespec() will just re-use the existing
2909
- * filenames, which will also get re-used by
2910
- * opt->priv->paths if they become renames, and then
2911
- * get freed at the end of the merge, leaving the copy
2912
- * in cached_pairs dangling. Avoid this by making a
2913
- * copy here.
2914
- *
2915
- * When pool == NULL, pool_alloc_filespec() calls
2916
- * alloc_filespec(), which makes a copy; we don't want
2917
- * to add another.
2918
- */
2919
- old_name = mem_pool_strdup(pool, old_name);
2920
- new_name = mem_pool_strdup(pool, new_name);
2921
- }
2811
+
2812
+ /*
2813
+ * cached_pairs has *copies* of old_name and new_name,
2814
+ * because it has to persist across merges. Since
2815
+ * pool_alloc_filespec() will just re-use the existing
2816
+ * filenames, which will also get re-used by
2817
+ * opt->priv->paths if they become renames, and then
2818
+ * get freed at the end of the merge, that would leave
2819
+ * the copy in cached_pairs dangling. Avoid this by
2820
+ * making a copy here.
2821
+ */
2822
+ old_name = mem_pool_strdup(&opt->priv->pool, old_name);
2823
+ new_name = mem_pool_strdup(&opt->priv->pool, new_name);
2824
2825
/* We don't care about oid/mode, only filenames and status */
2924
- one = pool_alloc_filespec(pool, old_name);
2925
- two = pool_alloc_filespec(pool, new_name);
2926
- pool_diff_queue(pool, pairs, one, two);
2826
+ one = pool_alloc_filespec(&opt->priv->pool, old_name);
2827
+ two = pool_alloc_filespec(&opt->priv->pool, new_name);
2828
+ pool_diff_queue(&opt->priv->pool, pairs, one, two);
2829
pairs->queue[pairs->nr-1]->status = entry->value ? 'R' : 'D';
2830
}
2831
}
@@ -3031,7 +2933,7 @@ static int detect_regular_renames(struct merge_options *opt,
2933
diff_queued_diff = renames->pairs[side_index];
2934
trace2_region_enter("diff", "diffcore_rename", opt->repo);
2935
diffcore_rename_extended(&diff_opts,
3034
- opt->priv->pool,
2936
+ &opt->priv->pool,
2937
&renames->relevant_sources[side_index],
2938
&renames->dirs_removed[side_index],
2939
&renames->dir_rename_count[side_index],
@@ -3082,7 +2984,7 @@ static int collect_renames(struct merge_options *opt,
2984
2985
if (p->status != 'A' && p->status != 'R') {
2986
possibly_cache_new_pair(renames, p, side_index, NULL);
3085
- pool_diff_free_filepair(opt->priv->pool, p);
2987
+ pool_diff_free_filepair(&opt->priv->pool, p);
2988
continue;
2989
}
2990
@@ -3095,7 +2997,7 @@ static int collect_renames(struct merge_options *opt,
2997
2998
possibly_cache_new_pair(renames, p, side_index, new_path);
2999
if (p->status != 'R' && !new_path) {
3098
- pool_diff_free_filepair(opt->priv->pool, p);
3000
+ pool_diff_free_filepair(&opt->priv->pool, p);
3001
continue;
3002
}
3003
@@ -3213,7 +3115,7 @@ cleanup:
3115
side_pairs = &renames->pairs[s];
3116
for (i = 0; i < side_pairs->nr; ++i) {
3117
struct diff_filepair *p = side_pairs->queue[i];
3216
- pool_diff_free_filepair(opt->priv->pool, p);
3118
+ pool_diff_free_filepair(&opt->priv->pool, p);
3119
}
3120
}
3121
@@ -3226,7 +3128,7 @@ simple_cleanup:
3128
if (combined.nr) {
3129
int i;
3130
for (i = 0; i < combined.nr; i++)
3229
- pool_diff_free_filepair(opt->priv->pool,
3131
+ pool_diff_free_filepair(&opt->priv->pool,
3132
combined.queue[i]);
3133
free(combined.queue);
3134
}
@@ -3701,7 +3603,7 @@ static void process_entry(struct merge_options *opt,
3603
* the directory to remain here, so we need to move this
3604
* path to some new location.
3605
*/
3704
- new_ci = pool_calloc(opt->priv->pool, 1, sizeof(*new_ci));
3606
+ new_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*new_ci));
3607
3608
/* We don't really want new_ci->merged.result copied, but it'll
3609
* be overwritten below so it doesn't matter. We also don't
@@ -3794,7 +3696,8 @@ static void process_entry(struct merge_options *opt,
3696
const char *a_path = NULL, *b_path = NULL;
3697
int rename_a = 0, rename_b = 0;
3698
3797
- new_ci = pool_alloc(opt->priv->pool, sizeof(*new_ci));
3699
+ new_ci = mem_pool_alloc(&opt->priv->pool,
3700
+ sizeof(*new_ci));
3701
3702
if (S_ISREG(a_mode))
3703
rename_a = 1;
@@ -3863,19 +3766,8 @@ static void process_entry(struct merge_options *opt,
3766
b_path = path;
3767
strmap_put(&opt->priv->paths, b_path, new_ci);
3768
3866
- if (rename_a && rename_b) {
3769
+ if (rename_a && rename_b)
3770
strmap_remove(&opt->priv->paths, path, 0);
3868
- /*
3869
- * We removed path from opt->priv->paths. path
3870
- * will also eventually need to be freed if not
3871
- * part of a memory pool...but it may still be
3872
- * used by e.g. ci->pathnames. So, store it in
3873
- * another string-list for now in that case.
3874
- */
3875
- if (!opt->priv->pool)
3876
- string_list_append(&opt->priv->paths_to_free,
3877
- path);
3878
- }
3771
3772
/*
3773
* 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)
4374
4375
/* Initialization of various renames fields */
4376
renames = &opt->priv->renames;
4485
-#if USE_MEMORY_POOL
4486
- mem_pool_init(&opt->priv->internal_pool, 0);
4487
- opt->priv->pool = &opt->priv->internal_pool;
4488
-#else
4489
- opt->priv->pool = NULL;
4490
-#endif
4491
- pool = opt->priv->pool;
4377
+ mem_pool_init(&opt->priv->pool, 0);
4378
+ pool = &opt->priv->pool;
4379
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
4380
strintmap_init_with_options(&renames->dirs_removed[i],
4381
NOT_RELEVANT, pool, 0);
@@ -4525,15 +4412,13 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
4412
* Although we initialize opt->priv->paths with strdup_strings=0,
4413
* that's just to avoid making yet another copy of an allocated
4414
* string. Putting the entry into paths means we are taking
4528
- * ownership, so we will later free it. paths_to_free is similar.
4415
+ * ownership, so we will later free it.
4416
*
4417
* In contrast, conflicted just has a subset of keys from paths, so
4418
* we don't want to free those (it'd be a duplicate free).
4419
*/
4420
strmap_init_with_options(&opt->priv->paths, pool, 0);
4421
strmap_init_with_options(&opt->priv->conflicted, pool, 0);
4535
- if (!opt->priv->pool)
4536
- string_list_init_nodup(&opt->priv->paths_to_free);
4422
4423
/*
4424
* keys & strbufs in output will sometimes need to outlive "paths",