git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 9/9] merge-ort: remove compile-time ability to turn off usage of memory pools

From
Jeff King <peff@peff.net>
Date
Jul 30, 2021, 16:24 UTC
Message-ID
<YQQn1yS/l8j6ymZO@coredump.intra.peff.net>
In-Reply-To
<de30dbac25e78e5ad81053b60c3ec2ab6256fde2.1627645665.git.gitgitgadget@gmail.com>
On Fri, Jul 30, 2021 at 11:47:44AM +0000, Elijah Newren via GitGitGadget wrote:
Show 8 quoted lines
> -#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;
> +	pool = opt->priv->pool = &opt->priv->internal_pool;

Since opt->priv->pool always points at internal_pool now, I think we can simplify much more. Every "if (!opt->priv->pool)" can go away, and in turn paths_to_free does, too. Those are really the spots I was most worried about in terms of complexity.

An easy way to find these spots is to get rid of internal_pool, and just make "pool" the struct. Then the compiler helpfully complains about all of the places that check the boolean value of a struct. :)

The patch below is from a fairly mechanical conversion I did. All of the spots were found by the compiler, except the one in use_cached_pairs (it assigns to a local "pool" pointer, which is always non-NULL, but that's not necessarily obvious to the compiler).

You might spot further opportunities for cleanup, as somebody who's more familiar with the allocation patterns (I happened to notice manually that paths_to_free is not needed anymore, but I don't know if there are any other subtle bits).

-Peff
 merge-ort.c | 151 +++++++++--------------------------
 1 file changed, 37 insertions(+), 114 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 63829f5cac..441dc4e094 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -303,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
@@ -347,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
@@ -537,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
@@ -558,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);
 
@@ -621,9 +582,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
 		strmap_clear(&opti->output, 0);
 	}
 
-	mem_pool_discard(&opti->internal_pool, 0);
-	if (!reinitialize)
-		opti->pool = NULL;
+	mem_pool_discard(&opti->pool, 0);
 
 	/* Clean out callback_data as well. */
 	FREE_AND_NULL(renames->callback_data);
@@ -846,7 +805,7 @@ static void setup_path_info(struct merge_options *opt,
 	assert(!df_conflict || !resolved); /* df_conflict implies !resolved */
 	assert(resolved == (merged_version != NULL));
 
-	mi = mem_pool_calloc(opt->priv->pool, 1,
+	mi = mem_pool_calloc(&opt->priv->pool, 1,
 			     resolved ? sizeof(struct merged_info) :
 					sizeof(struct conflict_info));
 	mi->directory_name = current_dir_name;
@@ -895,7 +854,7 @@ 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 mem_pool *pool = &opt->priv->pool;
 	struct rename_info *renames = &opt->priv->renames;
 	int names_idx = is_add ? side : 0;
 
@@ -1141,7 +1100,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 = mem_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);
 
 	/*
@@ -1396,7 +1355,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(&copy, &iter, entry) {
 			const char *path = entry->key;
@@ -2348,19 +2307,15 @@ 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 = mem_pool_strndup(opt->priv->pool,
+			parent_name = mem_pool_strndup(&opt->priv->pool,
 						       cur_path,
 						       last_slash - cur_path);
 		} else {
@@ -2371,8 +2326,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;
 		}
@@ -2399,16 +2352,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);
@@ -2442,8 +2385,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;
 	}
 
@@ -2859,7 +2800,7 @@ static void use_cached_pairs(struct merge_options *opt,
 {
 	struct hashmap_iter iter;
 	struct strmap_entry *entry;
-	struct mem_pool *pool = opt->priv->pool;
+	struct mem_pool *pool = &opt->priv->pool;
 
 	/*
 	 * Add to side_pairs all entries from renames->cached_pairs[side_index].
@@ -2871,25 +2812,20 @@ 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.
+		 *
+		 * 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.
+		 */
+		old_name = mem_pool_strdup(pool, old_name);
+		new_name = mem_pool_strdup(pool, new_name);
 
 		/* We don't care about oid/mode, only filenames and status */
 		one = pool_alloc_filespec(pool, old_name);
@@ -3002,7 +2938,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],
@@ -3053,7 +2989,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;
 		}
 
@@ -3066,7 +3002,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;
 		}
 
@@ -3184,7 +3120,7 @@ static int detect_and_process_renames(struct merge_options *opt,
 		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);
 		}
 	}
 
@@ -3197,7 +3133,7 @@ static int detect_and_process_renames(struct merge_options *opt,
 	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);
 	}
@@ -3672,7 +3608,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 = mem_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
@@ -3765,7 +3701,7 @@ 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 = mem_pool_alloc(opt->priv->pool,
+			new_ci = mem_pool_alloc(&opt->priv->pool,
 						sizeof(*new_ci));
 
 			if (S_ISREG(a_mode))
@@ -3835,19 +3771,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()
@@ -4454,8 +4379,8 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
 
 	/* Initialization of various renames fields */
 	renames = &opt->priv->renames;
-	mem_pool_init(&opt->priv->internal_pool, 0);
-	pool = opt->priv->pool = &opt->priv->internal_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);
@@ -4492,15 +4417,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",
Previous: Elijah Newren via GitGitGadgetNext: Elijah Newren via GitGitGadget
Message 53 of 65 in “Final optimization batch (#15): use memory pools”
  1. 0/7 Final optimization batch (#15): use memory poolsElijah Newren via GitGitGadget, Jul 23, 2021
  2. 1/7 diffcore-rename: use a mem_pool for exact rename detection's hashmapElijah Newren via GitGitGadget, Jul 23, 2021
  3. Eric SunshineJul 23, 2021
  4. Elijah NewrenJul 23, 2021
  5. 2/7 merge-ort: set up a memory poolElijah Newren via GitGitGadget, Jul 23, 2021
  6. 3/7 merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappersElijah Newren via GitGitGadget, Jul 23, 2021
  7. Eric SunshineJul 23, 2021
  8. Derrick StoleeJul 26, 2021
  9. Elijah NewrenJul 28, 2021
  10. Jeff KingJul 29, 2021
  11. Elijah NewrenJul 30, 2021
  12. Jeff KingJul 30, 2021
  13. 5/7 diffcore-rename, merge-ort: add wrapper functions for filepair alloc/deallocElijah Newren via GitGitGadget, Jul 23, 2021
  14. 4/7 merge-ort: switch our strmaps over to using memory poolsElijah Newren via GitGitGadget, Jul 23, 2021
  15. 6/7 merge-ort: store filepairs and filespecs in our mem_poolElijah Newren via GitGitGadget, Jul 23, 2021
  16. 7/7 merge-ort: reuse path strings in pool_alloc_filespecElijah Newren via GitGitGadget, Jul 23, 2021
  17. Derrick StoleeJul 26, 2021
  18. Elijah NewrenJul 28, 2021
  19. 0/7 Final optimization batch (#15): use memory poolsElijah Newren via GitGitGadget, Jul 29, 2021
  20. 2/7 merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappersElijah Newren via GitGitGadget, Jul 29, 2021
  21. 1/7 diffcore-rename: use a mem_pool for exact rename detection's hashmapElijah Newren via GitGitGadget, Jul 29, 2021
  22. 4/7 merge-ort: switch our strmaps over to using memory poolsElijah Newren via GitGitGadget, Jul 29, 2021
  23. Jeff KingJul 29, 2021
  24. Elijah NewrenJul 29, 2021
  25. Jeff KingJul 29, 2021
  26. Elijah NewrenJul 30, 2021
  27. Jeff KingJul 30, 2021
  28. Ævar Arnfjörð BjarmasonJul 30, 2021
  29. Elijah NewrenJul 30, 2021
  30. Ævar Arnfjörð BjarmasonJul 30, 2021
  31. 3/7 merge-ort: set up a memory poolElijah Newren via GitGitGadget, Jul 29, 2021
  32. 5/7 diffcore-rename, merge-ort: add wrapper functions for filepair alloc/deallocElijah Newren via GitGitGadget, Jul 29, 2021
  33. 6/7 merge-ort: store filepairs and filespecs in our mem_poolElijah Newren via GitGitGadget, Jul 29, 2021
  34. 7/7 merge-ort: reuse path strings in pool_alloc_filespecElijah Newren via GitGitGadget, Jul 29, 2021
  35. Derrick StoleeJul 29, 2021
  36. Jeff KingJul 29, 2021
  37. Jeff KingJul 29, 2021
  38. Junio C HamanoJul 29, 2021
  39. Junio C HamanoJul 29, 2021
  40. Elijah NewrenJul 29, 2021
  41. Elijah NewrenJul 29, 2021
  42. Jeff KingJul 29, 2021
  43. 0/9 Final optimization batch (#15): use memory poolsElijah Newren via GitGitGadget, Jul 30, 2021
  44. 1/9 merge-ort: rename str{map,intmap,set}_func()Elijah Newren via GitGitGadget, Jul 30, 2021
  45. 2/9 diffcore-rename: use a mem_pool for exact rename detection's hashmapElijah Newren via GitGitGadget, Jul 30, 2021
  46. 3/9 merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappersElijah Newren via GitGitGadget, Jul 30, 2021
  47. 5/9 merge-ort: switch our strmaps over to using memory poolsElijah Newren via GitGitGadget, Jul 30, 2021
  48. 4/9 merge-ort: set up a memory poolElijah Newren via GitGitGadget, Jul 30, 2021
  49. 7/9 merge-ort: store filepairs and filespecs in our mem_poolElijah Newren via GitGitGadget, Jul 30, 2021
  50. 6/9 diffcore-rename, merge-ort: add wrapper functions for filepair alloc/deallocElijah Newren via GitGitGadget, Jul 30, 2021
  51. 8/9 merge-ort: reuse path strings in pool_alloc_filespecElijah Newren via GitGitGadget, Jul 30, 2021
  52. 9/9 merge-ort: remove compile-time ability to turn off usage of memory poolsElijah Newren via GitGitGadget, Jul 30, 2021
  53. Jeff KingJul 30, 2021
  54. 0/9 Final optimization batch (#15): use memory poolsElijah Newren via GitGitGadget, Jul 31, 2021
  55. 2/9 diffcore-rename: use a mem_pool for exact rename detection's hashmapElijah Newren via GitGitGadget, Jul 31, 2021
  56. 1/9 merge-ort: rename str{map,intmap,set}_func()Elijah Newren via GitGitGadget, Jul 31, 2021
  57. 3/9 merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappersElijah Newren via GitGitGadget, Jul 31, 2021
  58. 5/9 merge-ort: switch our strmaps over to using memory poolsElijah Newren via GitGitGadget, Jul 31, 2021
  59. 4/9 merge-ort: set up a memory poolElijah Newren via GitGitGadget, Jul 31, 2021
  60. 6/9 diffcore-rename, merge-ort: add wrapper functions for filepair alloc/deallocElijah Newren via GitGitGadget, Jul 31, 2021
  61. 8/9 merge-ort: reuse path strings in pool_alloc_filespecElijah Newren via GitGitGadget, Jul 31, 2021
  62. 7/9 merge-ort: store filepairs and filespecs in our mem_poolElijah Newren via GitGitGadget, Jul 31, 2021
  63. 9/9 merge-ort: remove compile-time ability to turn off usage of memory poolsElijah Newren via GitGitGadget, Jul 31, 2021
  64. Derrick StoleeAug 2, 2021
  65. Jeff KingAug 3, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.