Skip to content

Commit 6697ee0

Browse files
newrengitster
newren
authored and
gitster
committed
merge-ort: switch our strmaps over to using memory pools
For all the strmaps (including strintmaps and strsets) whose memory is unconditionally freed as part of clear_or_reinit_internal_opts(), switch them over to using our new memory pool. For the testcases mentioned in commit 557ac03 ("merge-ort: begin performance work; instrument with trace2_region_* calls", 2020-10-28), this change improves the performance as follows: Before After no-renames: 202.5 ms ± 3.2 ms 198.1 ms ± 2.6 ms mega-renames: 1.072 s ± 0.012 s 715.8 ms ± 4.0 ms just-one-mega: 357.3 ms ± 3.9 ms 276.8 ms ± 4.2 ms Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 4137c54 commit 6697ee0

File tree

1 file changed

+75
-50
lines changed

1 file changed

+75
-50
lines changed

‎merge-ort.c‎

Lines changed: 75 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -539,15 +539,19 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
539539
void (*strset_clear_func)(struct strset *) =
540540
reinitialize ? strset_partial_clear : strset_clear;
541541

542-
/*
543-
* We marked opti->paths with strdup_strings = 0, so that we
544-
* wouldn't have to make another copy of the fullpath created by
545-
* make_traverse_path from setup_path_info(). But, now that we've
546-
* used it and have no other references to these strings, it is time
547-
* to deallocate them.
548-
*/
549-
free_strmap_strings(&opti->paths);
550-
strmap_clear_func(&opti->paths, 1);
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+
}
551555

552556
/*
553557
* All keys and values in opti->conflicted are a subset of those in
@@ -556,16 +560,19 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
556560
*/
557561
strmap_clear_func(&opti->conflicted, 0);
558562

559-
/*
560-
* opti->paths_to_free is similar to opti->paths; we created it with
561-
* strdup_strings = 0 to avoid making _another_ copy of the fullpath
562-
* but now that we've used it and have no other references to these
563-
* strings, it is time to deallocate them. We do so by temporarily
564-
* setting strdup_strings to 1.
565-
*/
566-
opti->paths_to_free.strdup_strings = 1;
567-
string_list_clear(&opti->paths_to_free, 0);
568-
opti->paths_to_free.strdup_strings = 0;
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+
}
569576

570577
if (opti->attr_index.cache_nr) /* true iff opt->renormalize */
571578
discard_index(&opti->attr_index);
@@ -683,23 +690,20 @@ static void path_msg(struct merge_options *opt,
683690
strbuf_addch(sb, '\n');
684691
}
685692

686-
MAYBE_UNUSED
687693
static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
688694
{
689695
if (!pool)
690696
return xcalloc(count, size);
691697
return mem_pool_calloc(pool, count, size);
692698
}
693699

694-
MAYBE_UNUSED
695700
static void *pool_alloc(struct mem_pool *pool, size_t size)
696701
{
697702
if (!pool)
698703
return xmalloc(size);
699704
return mem_pool_alloc(pool, size);
700705
}
701706

702-
MAYBE_UNUSED
703707
static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)
704708
{
705709
if (!pool)
@@ -835,8 +839,9 @@ static void setup_path_info(struct merge_options *opt,
835839
assert(!df_conflict || !resolved); /* df_conflict implies !resolved */
836840
assert(resolved == (merged_version != NULL));
837841

838-
mi = xcalloc(1, resolved ? sizeof(struct merged_info) :
839-
sizeof(struct conflict_info));
842+
mi = pool_calloc(opt->priv->pool, 1,
843+
resolved ? sizeof(struct merged_info) :
844+
sizeof(struct conflict_info));
840845
mi->directory_name = current_dir_name;
841846
mi->basename_offset = current_dir_name_len;
842847
mi->clean = !!resolved;
@@ -1128,7 +1133,7 @@ static int collect_merge_info_callback(int n,
11281133
len = traverse_path_len(info, p->pathlen);
11291134

11301135
/* +1 in both of the following lines to include the NUL byte */
1131-
fullpath = xmalloc(len + 1);
1136+
fullpath = pool_alloc(opt->priv->pool, len + 1);
11321137
make_traverse_path(fullpath, len + 1, info, p->path, p->pathlen);
11331138

11341139
/*
@@ -1383,7 +1388,7 @@ static int handle_deferred_entries(struct merge_options *opt,
13831388
copy = renames->deferred[side].possible_trivial_merges;
13841389
strintmap_init_with_options(&renames->deferred[side].possible_trivial_merges,
13851390
0,
1386-
NULL,
1391+
opt->priv->pool,
13871392
0);
13881393
strintmap_for_each_entry(&copy, &iter, entry) {
13891394
const char *path = entry->key;
@@ -2335,12 +2340,21 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
23352340
VERIFY_CI(ci);
23362341

23372342
/* Find parent directories missing from opt->priv->paths */
2338-
cur_path = new_path;
2343+
if (opt->priv->pool) {
2344+
cur_path = mem_pool_strdup(opt->priv->pool, new_path);
2345+
free((char*)new_path);
2346+
new_path = (char *)cur_path;
2347+
} else {
2348+
cur_path = new_path;
2349+
}
2350+
23392351
while (1) {
23402352
/* Find the parent directory of cur_path */
23412353
char *last_slash = strrchr(cur_path, '/');
23422354
if (last_slash) {
2343-
parent_name = xstrndup(cur_path, last_slash - cur_path);
2355+
parent_name = pool_strndup(opt->priv->pool,
2356+
cur_path,
2357+
last_slash - cur_path);
23442358
} else {
23452359
parent_name = opt->priv->toplevel_dir;
23462360
break;
@@ -2349,7 +2363,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
23492363
/* Look it up in opt->priv->paths */
23502364
entry = strmap_get_entry(&opt->priv->paths, parent_name);
23512365
if (entry) {
2352-
free((char*)parent_name);
2366+
if (!opt->priv->pool)
2367+
free((char*)parent_name);
23532368
parent_name = entry->key; /* reuse known pointer */
23542369
break;
23552370
}
@@ -2376,12 +2391,15 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
23762391
parent_name = cur_dir;
23772392
}
23782393

2379-
/*
2380-
* We are removing old_path from opt->priv->paths. old_path also will
2381-
* eventually need to be freed, but it may still be used by e.g.
2382-
* ci->pathnames. So, store it in another string-list for now.
2383-
*/
2384-
string_list_append(&opt->priv->paths_to_free, old_path);
2394+
if (!opt->priv->pool) {
2395+
/*
2396+
* We are removing old_path from opt->priv->paths.
2397+
* old_path also will eventually need to be freed, but it
2398+
* may still be used by e.g. ci->pathnames. So, store it
2399+
* in another string-list for now.
2400+
*/
2401+
string_list_append(&opt->priv->paths_to_free, old_path);
2402+
}
23852403

23862404
assert(ci->filemask == 2 || ci->filemask == 4);
23872405
assert(ci->dirmask == 0);
@@ -2416,7 +2434,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
24162434
new_ci->stages[index].mode = ci->stages[index].mode;
24172435
oidcpy(&new_ci->stages[index].oid, &ci->stages[index].oid);
24182436

2419-
free(ci);
2437+
if (!opt->priv->pool)
2438+
free(ci);
24202439
ci = new_ci;
24212440
}
24222441

@@ -3623,7 +3642,8 @@ static void process_entry(struct merge_options *opt,
36233642
* the directory to remain here, so we need to move this
36243643
* path to some new location.
36253644
*/
3626-
CALLOC_ARRAY(new_ci, 1);
3645+
new_ci = pool_calloc(opt->priv->pool, 1, sizeof(*new_ci));
3646+
36273647
/* We don't really want new_ci->merged.result copied, but it'll
36283648
* be overwritten below so it doesn't matter. We also don't
36293649
* want any directory mode/oid values copied, but we'll zero
@@ -3715,7 +3735,7 @@ static void process_entry(struct merge_options *opt,
37153735
const char *a_path = NULL, *b_path = NULL;
37163736
int rename_a = 0, rename_b = 0;
37173737

3718-
new_ci = xmalloc(sizeof(*new_ci));
3738+
new_ci = pool_alloc(opt->priv->pool, sizeof(*new_ci));
37193739

37203740
if (S_ISREG(a_mode))
37213741
rename_a = 1;
@@ -3788,12 +3808,14 @@ static void process_entry(struct merge_options *opt,
37883808
strmap_remove(&opt->priv->paths, path, 0);
37893809
/*
37903810
* We removed path from opt->priv->paths. path
3791-
* will also eventually need to be freed, but
3792-
* it may still be used by e.g. ci->pathnames.
3793-
* So, store it in another string-list for now.
3811+
* will also eventually need to be freed if not
3812+
* part of a memory pool...but it may still be
3813+
* used by e.g. ci->pathnames. So, store it in
3814+
* another string-list for now in that case.
37943815
*/
3795-
string_list_append(&opt->priv->paths_to_free,
3796-
path);
3816+
if (!opt->priv->pool)
3817+
string_list_append(&opt->priv->paths_to_free,
3818+
path);
37973819
}
37983820

37993821
/*
@@ -4335,6 +4357,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
43354357
{
43364358
struct rename_info *renames;
43374359
int i;
4360+
struct mem_pool *pool = NULL;
43384361

43394362
/* Sanity checks on opt */
43404363
trace2_region_enter("merge", "sanity checks", opt->repo);
@@ -4406,9 +4429,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
44064429
#else
44074430
opt->priv->pool = NULL;
44084431
#endif
4432+
pool = opt->priv->pool;
44094433
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
44104434
strintmap_init_with_options(&renames->dirs_removed[i],
4411-
NOT_RELEVANT, NULL, 0);
4435+
NOT_RELEVANT, pool, 0);
44124436
strmap_init_with_options(&renames->dir_rename_count[i],
44134437
NULL, 1);
44144438
strmap_init_with_options(&renames->dir_renames[i],
@@ -4422,7 +4446,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
44224446
*/
44234447
strintmap_init_with_options(&renames->relevant_sources[i],
44244448
-1 /* explicitly invalid */,
4425-
NULL, 0);
4449+
pool, 0);
44264450
strmap_init_with_options(&renames->cached_pairs[i],
44274451
NULL, 1);
44284452
strset_init_with_options(&renames->cached_irrelevant[i],
@@ -4432,9 +4456,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
44324456
}
44334457
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
44344458
strintmap_init_with_options(&renames->deferred[i].possible_trivial_merges,
4435-
0, NULL, 0);
4459+
0, pool, 0);
44364460
strset_init_with_options(&renames->deferred[i].target_dirs,
4437-
NULL, 1);
4461+
pool, 1);
44384462
renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */
44394463
}
44404464

@@ -4447,9 +4471,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
44474471
* In contrast, conflicted just has a subset of keys from paths, so
44484472
* we don't want to free those (it'd be a duplicate free).
44494473
*/
4450-
strmap_init_with_options(&opt->priv->paths, NULL, 0);
4451-
strmap_init_with_options(&opt->priv->conflicted, NULL, 0);
4452-
string_list_init_nodup(&opt->priv->paths_to_free);
4474+
strmap_init_with_options(&opt->priv->paths, pool, 0);
4475+
strmap_init_with_options(&opt->priv->conflicted, pool, 0);
4476+
if (!opt->priv->pool)
4477+
string_list_init_nodup(&opt->priv->paths_to_free);
44534478

44544479
/*
44554480
* keys & strbufs in output will sometimes need to outlive "paths",

0 commit comments

Comments
 (0)