| Message ID | 20210124060112.1258291-2-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | And so it begins...merge/rename performance work | expand |
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 --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 */
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(+)