Re: [PATCH v2] Fix to avoid high memory footprint

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2024, 21:41 UTC
Message-ID: <xmqqmsm6sc0q.fsf@gitster.g>
In-Reply-To: <pull.1744.v2.git.git.1721821503173.gitgitgadget@gmail.com>

"Haritha  via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> From: D Harithamma <harithamma.d@ibm.com>
>
> This fix avoids high memory footprint when adding files that require
> conversion.  Git has a trace_encoding routine that prints trace output
> when GIT_TRACE_WORKING_TREE_ENCODING=1 is set. This environment
> variable is used to debug the encoding contents.  When a 40MB file is
> added, it requests close to 1.8GB of storage from xrealloc which can
> lead to out of memory errors.  However, the check for
> GIT_TRACE_WORKING_TREE_ENCODING is done after the string is allocated.
> This resolves high memory footprints even when
> GIT_TRACE_WORKING_TREE_ENCODING is not active.  This fix adds an early
> exit to avoid the unnecessary memory allocation.

The sentences jump around and the logic flow is hard to follow. The first sentence makes a claim of what it does (but the readers have not bee told where that problem comes from). The second sentence makes a statement of a fact, but the readers do not yet know at that point what relevance the fact has to the issue at hand, etc.

The usual way to compose a log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.

    When Git needs to add a file that require encoding conversion,
    but tracing of encoding conversion is *not* requested via
    setting GIT_TRACE_WORKING_TREE_ENCODING environment variable,
    the trace_encoding() function still allocated and prepared
    "human readable" copies of the file contents before and after
    conversion to show in the trace.  This wasted a lot of memory
    footprint and runtime cycles without giving any user-visible
    benefit.

    Exit early from the function when we we are not tracing before
    we spend all the effort, not after.

or something, perhaps?

I am wondering if we should be able to test this, but "git grep GIT_TRACE_WORKING_TREE_ENCODING t/" is not finding any existing test in the area.

> Signed-off-by: Harithamma D <harithamma.d@ibm.com>

This does not match the "From: " line above. Please pick one way to spell your name and identify yourself to this project, and use it consistently.

Thanks.

Show 11 quoted lines

> diff --git a/convert.c b/convert.c
> index d8737fe0f2d..c4ddc4de81b 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -324,6 +324,9 @@ static void trace_encoding(const char *context, const char *path,
>  	struct strbuf trace = STRBUF_INIT;
>  	int i;
>  
> +	if (!trace_want(&coe))
> +		return;
> +

The actual fix is so simple and nice ;-)