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

Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits

From
Taylor Blau <me@ttaylorr.com>
Date
Nov 3, 2022, 00:20 UTC
Message-ID
<Y2MJRRfwG7rSp6Ra@nand.local>
In-Reply-To
<20221102220142.574890-2-szeder.dev@gmail.com>
On Wed, Nov 02, 2022 at 11:01:40PM +0100, SZEDER Gábor wrote:
Show 16 quoted lines
> When processing a non-merge commit, the line-level log first asks the
> tree-diff machinery whether any of the files in the given line ranges
> were modified between the current commit and its parent, and if some
> of them were, then it loads the contents of those files from both
> commits to see whether their line ranges were modified and/or need to
> be adjusted.  Alas, it doesn't free() the diff queue holding the
> results of that query and the contents of those files once its done.
> This can add up to a substantial amount of leaked memory, especially
> when the file in question is big and is frequently modified: a user
> reported "Out of memory, malloc failed" errors with a 2MB text file
> that was modified ~2800 times [1] (I estimate the leak would use up
> almost 11GB memory in that case).
>
> Free that diff queue to plug this memory leak.  However, instead of
> simply open-coding the necessary three lines, add them as a helper
> function to the diff API, because it will be useful elsewhere as well.
Nicely explained.
Show 5 quoted lines
> ---
>  diff.c     | 7 +++++++
>  diffcore.h | 1 +
>  line-log.c | 1 +
>  3 files changed, 9 insertions(+)
And all looks reasonable here, good...
Show 14 quoted lines
> diff --git a/diff.c b/diff.c
> index 35e46dd968..ef94175163 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5773,6 +5773,13 @@ void diff_free_filepair(struct diff_filepair *p)
>  	free(p);
>  }
>
> +void diff_free_queue(struct diff_queue_struct *q)
> +{
> +	for (int i = 0; i < q->nr; i++)
> +		diff_free_filepair(q->queue[i]);
> +	free(q->queue);
> +}

Though I wonder, should diff_free_queue() be a noop when q is NULL? The caller in process_ranges_ordinary_commit() doesn't care, of course, since q is always non-NULL there.

But if we're making it part of the diff API, we should probably err on the side of flexibility.

Thanks, Taylor

Previous: SZEDER GáborNext: SZEDER Gábor
Message 6 of 17 in “Bug report: git -L requires excessive memory.”
  1. man dogOct 29, 2022
  2. SZEDER GáborOct 31, 2022
  3. Taylor BlauOct 31, 2022
  4. 0/3 line-log: plug some memory leaksSZEDER Gábor, Nov 2, 2022
  5. 1/3 line-log: free diff queue when processing non-merge commitsSZEDER Gábor, Nov 2, 2022
  6. Taylor BlauNov 3, 2022
  7. SZEDER GáborNov 7, 2022
  8. Ævar Arnfjörð BjarmasonNov 7, 2022
  9. SZEDER GáborNov 7, 2022
  10. Taylor BlauNov 8, 2022
  11. 2/3 line-log: free the diff queues' arrays when processing merge commitsSZEDER Gábor, Nov 2, 2022
  12. Taylor BlauNov 3, 2022
  13. 3/3 diff.c: use diff_free_queue()SZEDER Gábor, Nov 2, 2022
  14. Taylor BlauNov 3, 2022
  15. SZEDER GáborNov 7, 2022
  16. Taylor BlauNov 8, 2022
  17. Ævar Arnfjörð BjarmasonNov 3, 2022

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.