@matheusbsbs / matheus-bagliano / commits / 6697ee01b5

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 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>

Elijah Newren committed Jul 30, 2021 at 11:47 UTC 6697ee01b5d31df5c83ace9eabb6285cf42ff172

1 file changed +75 -50

merge-ort.c

+75 -50

@@ -539,15 +539,19 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,

539 void (*strset_clear_func)(struct strset *) =

540 reinitialize ? strset_partial_clear : strset_clear;

541

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 + }

555

556 /*

557 * 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,

560 */

561 strmap_clear_func(&opti->conflicted, 0);

562

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 + }

576

577 if (opti->attr_index.cache_nr) /* true iff opt->renormalize */

578 discard_index(&opti->attr_index);

@@ -683,7 +690,6 @@ static void path_msg(struct merge_options *opt,

690 strbuf_addch(sb, '\n');

691 }

692

686 -MAYBE_UNUSED

693 static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)

694 {

695 if (!pool)

@@ -691,7 +697,6 @@ static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)

697 return mem_pool_calloc(pool, count, size);

698 }

699

694 -MAYBE_UNUSED

700 static void *pool_alloc(struct mem_pool *pool, size_t size)

701 {

702 if (!pool)

@@ -699,7 +704,6 @@ static void *pool_alloc(struct mem_pool *pool, size_t size)

704 return mem_pool_alloc(pool, size);

705 }

706

702 -MAYBE_UNUSED

707 static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)

708 {

709 if (!pool)

@@ -835,8 +839,9 @@ static void setup_path_info(struct merge_options *opt,

839 assert(!df_conflict || !resolved); /* df_conflict implies !resolved */

840 assert(resolved == (merged_version != NULL));

841

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));

845 mi->directory_name = current_dir_name;

846 mi->basename_offset = current_dir_name_len;

847 mi->clean = !!resolved;

@@ -1128,7 +1133,7 @@ static int collect_merge_info_callback(int n,

1133 len = traverse_path_len(info, p->pathlen);

1134

1135 /* +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);

1137 make_traverse_path(fullpath, len + 1, info, p->path, p->pathlen);

1138

1139 /*

@@ -1383,7 +1388,7 @@ static int handle_deferred_entries(struct merge_options *opt,

1388 copy = renames->deferred[side].possible_trivial_merges;

1389 strintmap_init_with_options(&renames->deferred[side].possible_trivial_merges,

1390 0,

1386 - NULL,

1391 + opt->priv->pool,

1392 0);

1393 strintmap_for_each_entry(&copy, &iter, entry) {

1394 const char *path = entry->key;

@@ -2335,12 +2340,21 @@ static void apply_directory_rename_modifications(struct merge_options *opt,

2340 VERIFY_CI(ci);

2341

2342 /* 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 +

2351 while (1) {

2352 /* Find the parent directory of cur_path */

2353 char *last_slash = strrchr(cur_path, '/');

2354 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);

2358 } else {

2359 parent_name = opt->priv->toplevel_dir;

2360 break;

@@ -2349,7 +2363,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,

2363 /* Look it up in opt->priv->paths */

2364 entry = strmap_get_entry(&opt->priv->paths, parent_name);

2365 if (entry) {

2352 - free((char*)parent_name);

2366 + if (!opt->priv->pool)

2367 + free((char*)parent_name);

2368 parent_name = entry->key; /* reuse known pointer */

2369 break;

2370 }

@@ -2376,12 +2391,15 @@ static void apply_directory_rename_modifications(struct merge_options *opt,

2391 parent_name = cur_dir;

2392 }

2393

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 + }

2403

2404 assert(ci->filemask == 2 || ci->filemask == 4);

2405 assert(ci->dirmask == 0);

@@ -2416,7 +2434,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,

2434 new_ci->stages[index].mode = ci->stages[index].mode;

2435 oidcpy(&new_ci->stages[index].oid, &ci->stages[index].oid);

2436

2419 - free(ci);

2437 + if (!opt->priv->pool)

2438 + free(ci);

2439 ci = new_ci;

2440 }

2441

@@ -3623,7 +3642,8 @@ static void process_entry(struct merge_options *opt,

3642 * the directory to remain here, so we need to move this

3643 * path to some new location.

3644 */

3626 - CALLOC_ARRAY(new_ci, 1);

3645 + new_ci = pool_calloc(opt->priv->pool, 1, sizeof(*new_ci));

3646 +

3647 /* We don't really want new_ci->merged.result copied, but it'll

3648 * be overwritten below so it doesn't matter. We also don't

3649 * want any directory mode/oid values copied, but we'll zero

@@ -3715,7 +3735,7 @@ static void process_entry(struct merge_options *opt,

3735 const char *a_path = NULL, *b_path = NULL;

3736 int rename_a = 0, rename_b = 0;

3737

3718 - new_ci = xmalloc(sizeof(*new_ci));

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

3739

3740 if (S_ISREG(a_mode))

3741 rename_a = 1;

@@ -3788,12 +3808,14 @@ static void process_entry(struct merge_options *opt,

3808 strmap_remove(&opt->priv->paths, path, 0);

3809 /*

3810 * 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.

3815 */

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);

3819 }

3820

3821 /*

@@ -4335,6 +4357,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)

4357 {

4358 struct rename_info *renames;

4359 int i;

4360 + struct mem_pool *pool = NULL;

4361

4362 /* Sanity checks on opt */

4363 trace2_region_enter("merge", "sanity checks", opt->repo);

@@ -4406,9 +4429,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)

4429 #else

4430 opt->priv->pool = NULL;

4431 #endif

4432 + pool = opt->priv->pool;

4433 for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {

4434 strintmap_init_with_options(&renames->dirs_removed[i],

4411 - NOT_RELEVANT, NULL, 0);

4435 + NOT_RELEVANT, pool, 0);

4436 strmap_init_with_options(&renames->dir_rename_count[i],

4437 NULL, 1);

4438 strmap_init_with_options(&renames->dir_renames[i],

@@ -4422,7 +4446,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)

4446 */

4447 strintmap_init_with_options(&renames->relevant_sources[i],

4448 -1 /* explicitly invalid */,

4425 - NULL, 0);

4449 + pool, 0);

4450 strmap_init_with_options(&renames->cached_pairs[i],

4451 NULL, 1);

4452 strset_init_with_options(&renames->cached_irrelevant[i],

@@ -4432,9 +4456,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)

4456 }

4457 for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {

4458 strintmap_init_with_options(&renames->deferred[i].possible_trivial_merges,

4435 - 0, NULL, 0);

4459 + 0, pool, 0);

4460 strset_init_with_options(&renames->deferred[i].target_dirs,

4437 - NULL, 1);

4461 + pool, 1);

4462 renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */

4463 }

4464

@@ -4447,9 +4471,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)

4471 * In contrast, conflicted just has a subset of keys from paths, so

4472 * we don't want to free those (it'd be a duplicate free).

4473 */

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);

4478

4479 /*

4480 * keys & strbufs in output will sometimes need to outlive "paths",