From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>,
man dog <dogman888888@gmail.com>
Subject: Re: [PATCH 0/3] line-log: plug some memory leaks
Date: Thu, 03 Nov 2022 10:05:18 +0100 [thread overview]
Message-ID: <221103.864jvg2yit.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20221102220142.574890-1-szeder.dev@gmail.com>
On Wed, Nov 02 2022, SZEDER Gábor wrote:
> The first patch plugs the reported big memory leak, the second one
> plugs a minor leak, and the little cleanup in the third puts the
> cherry on top.
Looks good as far as it goes.
The "further" part seems a real mess though, e.g. I came up with the
below on top as a quick test, i.e. we have other existing users that
also loop over the same struct, and free "queue[n]", but do so
differently.
I handled the combine-diff.c one (rough WIP, only compiled it), but then
diffcore-rename.c has another such case.
I wonder if you looked at that further, and if the free function we're
adding now should anticipate that case or not.
And, orthagonally I came up with this rough WIP yesterday:
@@ -6640,9 +6633,7 @@ static void diffcore_apply_filter(struct diff_options *options)
{
int i;
struct diff_queue_struct *q = &diff_queued_diff;
- struct diff_queue_struct outq;
-
- DIFF_QUEUE_CLEAR(&outq);
+ struct diff_queue_struct outq = DIFF_QUEUE_STRUCT_INIT;
if (!options->filter)
return;
@@ -6735,8 +6726,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
{
int i;
struct diff_queue_struct *q = &diff_queued_diff;
- struct diff_queue_struct outq;
- DIFF_QUEUE_CLEAR(&outq);
+ struct diff_queue_struct outq = DIFF_QUEUE_STRUCT_INIT;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
diff --git a/diffcore.h b/diffcore.h
index badc2261c20..a0a89568cec 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -150,12 +150,7 @@ struct diff_queue_struct {
int alloc;
int nr;
};
-
-#define DIFF_QUEUE_CLEAR(q) \
- do { \
- (q)->queue = NULL; \
- (q)->nr = (q)->alloc = 0; \
- } while (0)
+ #define DIFF_QUEUE_STRUCT_INIT { 0 }
You leave the DIFF_QUEUE_CLEAR in place, but I wonder given that that's
the common pattern whether you shouldn't have a *_reset() and
*_{free,release}() (one of the two, your current naming is fine) which
resets it too, as some callers seen below & in your diff context want
that.
diff --git a/combine-diff.c b/combine-diff.c
index b0ece954808..509c58ad556 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1297,12 +1297,6 @@ void show_combined_diff(struct combine_diff_path *p,
show_patch_diff(p, num_parent, 1, rev);
}
-static void free_combined_pair(struct diff_filepair *pair)
-{
- free(pair->two);
- free(pair);
-}
-
/*
* A combine_diff_path expresses N parents on the LHS against 1 merge
* result. Synthesize a diff_filepair that has N entries on the "one"
@@ -1355,9 +1349,7 @@ static void handle_combined_callback(struct diff_options *opt,
for (i = 0, p = paths; p; p = p->next)
q.queue[i++] = combined_pair(p, num_parent);
opt->format_callback(&q, opt, opt->format_callback_data);
- for (i = 0; i < num_paths; i++)
- free_combined_pair(q.queue[i]);
- free(q.queue);
+ diff_free_queue(&q, 1);
}
static const char *path_path(void *obj)
diff --git a/diff.c b/diff.c
index 03e6ffb5e4e..b48105c070f 100644
--- a/diff.c
+++ b/diff.c
@@ -5773,10 +5773,20 @@ void diff_free_filepair(struct diff_filepair *p)
free(p);
}
-void diff_free_queue(struct diff_queue_struct *q)
+static void free_combined_pair(struct diff_filepair *pair)
{
- for (int i = 0; i < q->nr; i++)
- diff_free_filepair(q->queue[i]);
+ free(pair->two);
+ free(pair);
+}
+
+void diff_free_queue(struct diff_queue_struct *q, int combined)
+{
+ for (int i = 0; i < q->nr; i++) {
+ if (combined)
+ diff_free_filepair(q->queue[i]);
+ else
+ free_combined_pair(q->queue[i]);
+ }
free(q->queue);
}
@@ -6339,7 +6349,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int
struct diff_queue_struct *q = &diff_queued_diff;
int result = diff_get_patch_id(options, oid, diff_header_only);
- diff_free_queue(q);
+ diff_free_queue(q, 0);
DIFF_QUEUE_CLEAR(q);
return result;
@@ -6609,7 +6619,7 @@ void diff_flush(struct diff_options *options)
options->format_callback(q, options, options->format_callback_data);
free_queue:
- diff_free_queue(q);
+ diff_free_queue(q, 0);
DIFF_QUEUE_CLEAR(q);
diff_free(options);
diff --git a/diffcore-rename.c b/diffcore-rename.c
index c0422d9e709..1754411a916 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -1686,6 +1686,7 @@ void diffcore_rename_extended(struct diff_options *options,
pair_to_free = p;
if (pair_to_free)
+ /* ??? */
pool_diff_free_filepair(pool, pair_to_free);
}
diff_debug_queue("done copying original", &outq);
diff --git a/diffcore.h b/diffcore.h
index 9b588a1ee15..6cb74e6eadf 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -162,7 +162,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,
struct diff_filespec *,
struct diff_filespec *);
void diff_q(struct diff_queue_struct *, struct diff_filepair *);
-void diff_free_queue(struct diff_queue_struct *q);
+void diff_free_queue(struct diff_queue_struct *q, int combined);
/* dir_rename_relevance: the reason we want rename information for a dir */
enum dir_rename_relevance {
diff --git a/line-log.c b/line-log.c
index a7f3e7f6ce4..88c22b20c0f 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1090,7 +1090,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)
static void free_diffqueues(int n, struct diff_queue_struct *dq)
{
for (int i = 0; i < n; i++)
- diff_free_queue(&dq[i]);
+ diff_free_queue(&dq[i], 0);
free(dq);
}
@@ -1193,7 +1193,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
if (parent)
add_line_range(rev, parent, parent_range);
free_line_log_data(parent_range);
- diff_free_queue(&queue);
+ diff_free_queue(&queue, 0);
return changed;
}
prev parent reply other threads:[~2022-11-03 9:11 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-29 16:59 Bug report: git -L requires excessive memory man dog
2022-10-31 21:45 ` SZEDER Gábor
2022-10-31 21:56 ` Taylor Blau
2022-11-02 22:01 ` [PATCH 0/3] line-log: plug some memory leaks SZEDER Gábor
2022-11-02 22:01 ` [PATCH 1/3] line-log: free diff queue when processing non-merge commits SZEDER Gábor
2022-11-03 0:20 ` Taylor Blau
2022-11-07 15:11 ` SZEDER Gábor
2022-11-07 15:29 ` Ævar Arnfjörð Bjarmason
2022-11-07 15:57 ` SZEDER Gábor
2022-11-08 2:14 ` Taylor Blau
2022-11-02 22:01 ` [PATCH 2/3] line-log: free the diff queues' arrays when processing merge commits SZEDER Gábor
2022-11-03 0:21 ` Taylor Blau
2022-11-02 22:01 ` [PATCH 3/3] diff.c: use diff_free_queue() SZEDER Gábor
2022-11-03 0:24 ` Taylor Blau
2022-11-07 16:13 ` SZEDER Gábor
2022-11-08 2:14 ` Taylor Blau
2022-11-03 9:05 ` Ævar Arnfjörð Bjarmason [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221103.864jvg2yit.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=dogman888888@gmail.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).