undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merge-ort: switch our strmaps over to using memory pools

Browse Source 
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 557ac0350d ("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>
This commit is contained in:
Elijah Newren 2021-07-30 11:47:40 +00:00 committed by Junio C Hamano
parent 4137c54b90
commit 6697ee01b5
1 changed files with 75 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

85
merge-ort.c
View File

@ -539,15 +539,19 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
void (*strset_clear_func)(struct strset *) = void (*strset_clear_func)(struct strset *) =
reinitialize ? strset_partial_clear : strset_clear; 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 * We marked opti->paths with strdup_strings = 0, so that
* wouldn't have to make another copy of the fullpath created by * we wouldn't have to make another copy of the fullpath
* make_traverse_path from setup_path_info(). But, now that we've * created by make_traverse_path from setup_path_info().
* used it and have no other references to these strings, it is time * But, now that we've used it and have no other references
* to deallocate them. * to these strings, it is time to deallocate them.
*/ */
free_strmap_strings(&opti->paths); free_strmap_strings(&opti->paths);
strmap_clear_func(&opti->paths, 1); strmap_clear_func(&opti->paths, 1);
}
/* /*
* All keys and values in opti->conflicted are a subset of those in * 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,
*/ */
strmap_clear_func(&opti->conflicted, 0); strmap_clear_func(&opti->conflicted, 0);
if (!opti->pool) {
/* /*
* opti->paths_to_free is similar to opti->paths; we created it with * opti->paths_to_free is similar to opti->paths; we
* strdup_strings = 0 to avoid making _another_ copy of the fullpath * created it with strdup_strings = 0 to avoid making
* but now that we've used it and have no other references to these * _another_ copy of the fullpath but now that we've used
* strings, it is time to deallocate them. We do so by temporarily * 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. * setting strdup_strings to 1.
*/ */
opti->paths_to_free.strdup_strings = 1; opti->paths_to_free.strdup_strings = 1;
string_list_clear(&opti->paths_to_free, 0); string_list_clear(&opti->paths_to_free, 0);
opti->paths_to_free.strdup_strings = 0; opti->paths_to_free.strdup_strings = 0;
}
if (opti->attr_index.cache_nr) /* true iff opt->renormalize */ if (opti->attr_index.cache_nr) /* true iff opt->renormalize */
discard_index(&opti->attr_index); discard_index(&opti->attr_index);
@ -683,7 +690,6 @@ static void path_msg(struct merge_options *opt,
strbuf_addch(sb, '\n'); strbuf_addch(sb, '\n');
} }
MAYBE_UNUSED
static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size) static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
{ {
if (!pool) if (!pool)
@ -691,7 +697,6 @@ static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
return mem_pool_calloc(pool, count, size); return mem_pool_calloc(pool, count, size);
} }
MAYBE_UNUSED
static void *pool_alloc(struct mem_pool *pool, size_t size) static void *pool_alloc(struct mem_pool *pool, size_t size)
{ {
if (!pool) if (!pool)
@ -699,7 +704,6 @@ static void *pool_alloc(struct mem_pool *pool, size_t size)
return mem_pool_alloc(pool, size); return mem_pool_alloc(pool, size);
} }
MAYBE_UNUSED
static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len) static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)
{ {
if (!pool) if (!pool)
@ -835,7 +839,8 @@ static void setup_path_info(struct merge_options *opt,
assert(!df_conflict || !resolved); /* df_conflict implies !resolved */ assert(!df_conflict || !resolved); /* df_conflict implies !resolved */
assert(resolved == (merged_version != NULL)); assert(resolved == (merged_version != NULL));
mi = xcalloc(1, resolved ? sizeof(struct merged_info) : mi = pool_calloc(opt->priv->pool, 1,
resolved ? sizeof(struct merged_info) :
sizeof(struct conflict_info)); sizeof(struct conflict_info));
mi->directory_name = current_dir_name; mi->directory_name = current_dir_name;
mi->basename_offset = current_dir_name_len; mi->basename_offset = current_dir_name_len;
@ -1128,7 +1133,7 @@ static int collect_merge_info_callback(int n,
len = traverse_path_len(info, p->pathlen); len = traverse_path_len(info, p->pathlen);
/* +1 in both of the following lines to include the NUL byte */ /* +1 in both of the following lines to include the NUL byte */
fullpath = xmalloc(len + 1); fullpath = pool_alloc(opt->priv->pool, len + 1);
make_traverse_path(fullpath, len + 1, info, p->path, p->pathlen); make_traverse_path(fullpath, len + 1, info, p->path, p->pathlen);
/* /*
@ -1383,7 +1388,7 @@ static int handle_deferred_entries(struct merge_options *opt,
copy = renames->deferred[side].possible_trivial_merges; copy = renames->deferred[side].possible_trivial_merges;
strintmap_init_with_options(&renames->deferred[side].possible_trivial_merges, strintmap_init_with_options(&renames->deferred[side].possible_trivial_merges,
0, 0,
NULL, opt->priv->pool,
0); 0);
strintmap_for_each_entry(&copy, &iter, entry) { strintmap_for_each_entry(&copy, &iter, entry) {
const char *path = entry->key; const char *path = entry->key;
@ -2335,12 +2340,21 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
VERIFY_CI(ci); VERIFY_CI(ci);
/* Find parent directories missing from opt->priv->paths */ /* 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 = new_path;
}
while (1) { while (1) {
/* Find the parent directory of cur_path */ /* Find the parent directory of cur_path */
char *last_slash = strrchr(cur_path, '/'); char *last_slash = strrchr(cur_path, '/');
if (last_slash) { if (last_slash) {
parent_name = xstrndup(cur_path, last_slash - cur_path); parent_name = pool_strndup(opt->priv->pool,
cur_path,
last_slash - cur_path);
} else { } else {
parent_name = opt->priv->toplevel_dir; parent_name = opt->priv->toplevel_dir;
break; break;
@ -2349,6 +2363,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
/* Look it up in opt->priv->paths */ /* Look it up in opt->priv->paths */
entry = strmap_get_entry(&opt->priv->paths, parent_name); entry = strmap_get_entry(&opt->priv->paths, parent_name);
if (entry) { if (entry) {
if (!opt->priv->pool)
free((char*)parent_name); free((char*)parent_name);
parent_name = entry->key; /* reuse known pointer */ parent_name = entry->key; /* reuse known pointer */
break; break;
@ -2376,12 +2391,15 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
parent_name = cur_dir; parent_name = cur_dir;
} }
if (!opt->priv->pool) {
/* /*
* We are removing old_path from opt->priv->paths. old_path also will * We are removing old_path from opt->priv->paths.
* eventually need to be freed, but it may still be used by e.g. * old_path also will eventually need to be freed, but it
* ci->pathnames. So, store it in another string-list for now. * 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); string_list_append(&opt->priv->paths_to_free, old_path);
}
assert(ci->filemask == 2 || ci->filemask == 4); assert(ci->filemask == 2 || ci->filemask == 4);
assert(ci->dirmask == 0); assert(ci->dirmask == 0);
@ -2416,6 +2434,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
new_ci->stages[index].mode = ci->stages[index].mode; new_ci->stages[index].mode = ci->stages[index].mode;
oidcpy(&new_ci->stages[index].oid, &ci->stages[index].oid); oidcpy(&new_ci->stages[index].oid, &ci->stages[index].oid);
if (!opt->priv->pool)
free(ci); free(ci);
ci = new_ci; ci = new_ci;
} }
@ -3623,7 +3642,8 @@ static void process_entry(struct merge_options *opt,
* the directory to remain here, so we need to move this * the directory to remain here, so we need to move this
* path to some new location. * path to some new location.
*/ */
CALLOC_ARRAY(new_ci, 1); new_ci = pool_calloc(opt->priv->pool, 1, sizeof(*new_ci));
/* We don't really want new_ci->merged.result copied, but it'll /* 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 * be overwritten below so it doesn't matter. We also don't
* want any directory mode/oid values copied, but we'll zero * want any directory mode/oid values copied, but we'll zero
@ -3715,7 +3735,7 @@ static void process_entry(struct merge_options *opt,
const char *a_path = NULL, *b_path = NULL; const char *a_path = NULL, *b_path = NULL;
int rename_a = 0, rename_b = 0; int rename_a = 0, rename_b = 0;
new_ci = xmalloc(sizeof(*new_ci)); new_ci = pool_alloc(opt->priv->pool, sizeof(*new_ci));
if (S_ISREG(a_mode)) if (S_ISREG(a_mode))
rename_a = 1; rename_a = 1;
@ -3788,10 +3808,12 @@ static void process_entry(struct merge_options *opt,
strmap_remove(&opt->priv->paths, path, 0); strmap_remove(&opt->priv->paths, path, 0);
/* /*
* We removed path from opt->priv->paths. path * We removed path from opt->priv->paths. path
* will also eventually need to be freed, but * will also eventually need to be freed if not
* it may still be used by e.g. ci->pathnames. * part of a memory pool...but it may still be
* So, store it in another string-list for now. * 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, string_list_append(&opt->priv->paths_to_free,
path); path);
} }
@ -4335,6 +4357,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
{ {
struct rename_info *renames; struct rename_info *renames;
int i; int i;
struct mem_pool *pool = NULL;
/* Sanity checks on opt */ /* Sanity checks on opt */
trace2_region_enter("merge", "sanity checks", opt->repo); trace2_region_enter("merge", "sanity checks", opt->repo);
@ -4406,9 +4429,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
#else #else
opt->priv->pool = NULL; opt->priv->pool = NULL;
#endif #endif
pool = opt->priv->pool;
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) { for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
strintmap_init_with_options(&renames->dirs_removed[i], strintmap_init_with_options(&renames->dirs_removed[i],
NOT_RELEVANT, NULL, 0); NOT_RELEVANT, pool, 0);
strmap_init_with_options(&renames->dir_rename_count[i], strmap_init_with_options(&renames->dir_rename_count[i],
NULL, 1); NULL, 1);
strmap_init_with_options(&renames->dir_renames[i], strmap_init_with_options(&renames->dir_renames[i],
@ -4422,7 +4446,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
*/ */
strintmap_init_with_options(&renames->relevant_sources[i], strintmap_init_with_options(&renames->relevant_sources[i],
-1 /* explicitly invalid */, -1 /* explicitly invalid */,
NULL, 0); pool, 0);
strmap_init_with_options(&renames->cached_pairs[i], strmap_init_with_options(&renames->cached_pairs[i],
NULL, 1); NULL, 1);
strset_init_with_options(&renames->cached_irrelevant[i], strset_init_with_options(&renames->cached_irrelevant[i],
@ -4432,9 +4456,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
} }
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) { for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
strintmap_init_with_options(&renames->deferred[i].possible_trivial_merges, strintmap_init_with_options(&renames->deferred[i].possible_trivial_merges,
0, NULL, 0); 0, pool, 0);
strset_init_with_options(&renames->deferred[i].target_dirs, strset_init_with_options(&renames->deferred[i].target_dirs,
NULL, 1); pool, 1);
renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */ renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */
} }
@ -4447,8 +4471,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
* In contrast, conflicted just has a subset of keys from paths, so * 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). * we don't want to free those (it'd be a duplicate free).
*/ */
strmap_init_with_options(&opt->priv->paths, NULL, 0); strmap_init_with_options(&opt->priv->paths, pool, 0);
strmap_init_with_options(&opt->priv->conflicted, NULL, 0); strmap_init_with_options(&opt->priv->conflicted, pool, 0);
if (!opt->priv->pool)
string_list_init_nodup(&opt->priv->paths_to_free); string_list_init_nodup(&opt->priv->paths_to_free);
/* /*