diff mbox series

[v4,1/3] merge-ort: fix massive leak

Message ID 20210124060112.1258291-2-newren@gmail.com (mailing list archive)
State New
Headers show
Series And so it begins...merge/rename performance work | expand

Commit Message

Elijah Newren Jan. 24, 2021, 6:01 a.m. UTC
When a series of merges was performed (such as for a rebase or series of
cherry-picks), only the data structures allocated by the final merge
operation were being freed.  The problem was that while picking out
pieces of merge-ort to upstream, I previously misread a certain section
of merge_start() and assumed it was associated with a later
optimization.  Include that section now, which ensures that if there was
a previous merge operation, that we clear out result->priv and then
re-use it for opt->priv, and otherwise we allocate opt->priv.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Derrick Stolee Jan. 24, 2021, 7:11 p.m. UTC | #1
On 1/24/2021 1:01 AM, Elijah Newren wrote:
> When a series of merges was performed (such as for a rebase or series of
> cherry-picks), only the data structures allocated by the final merge
> operation were being freed.  The problem was that while picking out
> pieces of merge-ort to upstream, I previously misread a certain section
> of merge_start() and assumed it was associated with a later
> optimization.  Include that section now, which ensures that if there was
> a previous merge operation, that we clear out result->priv and then
> re-use it for opt->priv, and otherwise we allocate opt->priv.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/merge-ort.c b/merge-ort.c
> index 05c6b2e0dc..b5845ff6e9 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3227,11 +3227,28 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
>  	assert(opt->obuf.len == 0);
>  
>  	assert(opt->priv == NULL);
> +	if (result->priv) {
> +		opt->priv = result->priv;
> +		result->priv = NULL;
> +		/*
> +		 * opt->priv non-NULL means we had results from a previous
> +		 * run; do a few sanity checks that user didn't mess with
> +		 * it in an obvious fashion.
> +		 */
> +		assert(opt->priv->call_depth == 0);
> +		assert(!opt->priv->toplevel_dir ||
> +		       0 == strlen(opt->priv->toplevel_dir));
> +	}

So instead of simply leaking result->priv, we re-use the
data for the next round.

>  
>  	/* Default to histogram diff.  Actually, just hardcode it...for now. */
>  	opt->xdl_opts = DIFF_WITH_ALG(opt, HISTOGRAM_DIFF);
>  
>  	/* Initialization of opt->priv, our internal merge data */
> +	if (opt->priv) {
> +		clear_or_reinit_internal_opts(opt->priv, 1);
> +		trace2_region_leave("merge", "allocate/init", opt->repo);
> +		return;
> +	}
>  	opt->priv = xcalloc(1, sizeof(*opt->priv));

and here you reset the data instead of reallocating it. OK.

-Stolee
diff mbox series

Patch

diff --git a/merge-ort.c b/merge-ort.c
index 05c6b2e0dc..b5845ff6e9 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3227,11 +3227,28 @@  static void merge_start(struct merge_options *opt, struct merge_result *result)
 	assert(opt->obuf.len == 0);
 
 	assert(opt->priv == NULL);
+	if (result->priv) {
+		opt->priv = result->priv;
+		result->priv = NULL;
+		/*
+		 * opt->priv non-NULL means we had results from a previous
+		 * run; do a few sanity checks that user didn't mess with
+		 * it in an obvious fashion.
+		 */
+		assert(opt->priv->call_depth == 0);
+		assert(!opt->priv->toplevel_dir ||
+		       0 == strlen(opt->priv->toplevel_dir));
+	}
 
 	/* Default to histogram diff.  Actually, just hardcode it...for now. */
 	opt->xdl_opts = DIFF_WITH_ALG(opt, HISTOGRAM_DIFF);
 
 	/* Initialization of opt->priv, our internal merge data */
+	if (opt->priv) {
+		clear_or_reinit_internal_opts(opt->priv, 1);
+		trace2_region_leave("merge", "allocate/init", opt->repo);
+		return;
+	}
 	opt->priv = xcalloc(1, sizeof(*opt->priv));
 
 	/* Initialization of various renames fields */