threads / patch / 61784

patchFix to avoid high memory footprint

Subject: [PATCH] Fix to avoid high memory footprint

## tl;dr

15 messages between Jul 16, 2024 and Jul 31, 2024. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Haritha via GitGitGadget· Jul 16, 2024, 08:03 UTC · lore
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.

Signed-off-by: Haritha D <harithamma.d@ibm.com>
---
    Fix to avoid high memory footprint
    
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1744%2FHarithaIBM%2FmemFootprintFix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1744/HarithaIBM/memFootprintFix-v1
Pull-Request: https://github.com/git/git/pull/1744
 convert.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to convert.c +5 −0
diff --git a/convert.c b/convert.c
index d8737fe0f2d..e765bcd53d6 100644
--- a/convert.c
+++ b/convert.c
@@ -324,6 +324,11 @@ static void trace_encoding(const char *context, const char *path,
 	struct strbuf trace = STRBUF_INIT;
 	int i;
 
+	// If tracing is not on, exit early to avoid high memory footprint
+	if (!trace_pass_fl(&coe)) {
+		return;
+	}
+
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(

base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9

-- 
gitgitgadget
Jeff King· Jul 17, 2024, 06:16 UTC · re: Haritha via GitGitGadget · lore

Re: [PATCH] Fix to avoid high memory footprint

On Tue, Jul 16, 2024 at 08:03:59AM +0000, Haritha  via GitGitGadget wrote:
Show 20 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.
> 
> Signed-off-by: Haritha D <harithamma.d@ibm.com>

Good find. Any trace function should verify that tracing is enabled before doing any substantial work.

Let's take a look at your patch. First, your line wrapping is unusual, making the commit message a bit hard to read. We'd usually shoot for ~72 characters per line. So more like:

Show 10 quoted lines
> 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.

Second, we'd like a full real name in the Signed-off-by line, as you're agreeing to the DCO. See:

  https://git-scm.com/docs/SubmittingPatches#sign-off

Likewise, the author name should match the signoff name (you can use "git commit --amend --author=..." to fix it).

For the patch itself:
Show 10 quoted lines
> --- a/convert.c
> +++ b/convert.c
> @@ -324,6 +324,11 @@ static void trace_encoding(const char *context, const char *path,
>  	struct strbuf trace = STRBUF_INIT;
>  	int i;
>  
> +	// If tracing is not on, exit early to avoid high memory footprint
> +	if (!trace_pass_fl(&coe)) {
> +		return;
> +	}

I don't think trace_pass_fl() is what you want. It will return true if the trace fd is non-zero (so tracing was requested), but also if the key has not yet been initialized (i.e., nobody has used this key to try printing anything yet).

I think you'd just use trace_want(&coe) instead.
Also, two style nits:
 - our usual style (see Documentation/CodingGuidelines) is to avoid
   braces for one-liners.
 - we only use the /* */ comment form, not //. Though IMHO you could
   skip the comment completely here, as an early-return check in a
   tracing function is pretty obvious.

It would be nice if we could test this, but besides the wasted work, I don't think there's any user-visible behavior (the problem is that we are computing things when we're _not_ tracing, so there's nothing for the user to see). And there's no provision in our test suite for measuring memory usage of a program. So I think we can live without it, and just manually verifying that it works (but it would be good to show the measurements you did manually in the commit message).

-Peff
Haritha via GitGitGadget· Jul 24, 2024, 11:45 UTC · re: Haritha via GitGitGadget · lore

[PATCH v2] Fix to avoid high memory footprint

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.

Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
    Fix to avoid high memory footprint
    
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1744%2FHarithaIBM%2FmemFootprintFix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1744/HarithaIBM/memFootprintFix-v2
Pull-Request: https://github.com/git/git/pull/1744
Range-diff vs v1:
 1:  51c02f58fd6 ! 1:  500b7eacf2a Fix to avoid high memory footprint
     @@ Metadata
       ## Commit message ##
          Fix to avoid high memory footprint
      
     -    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.
     +    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.
      
     -    Signed-off-by: Haritha D <harithamma.d@ibm.com>
     +    Signed-off-by: Harithamma D <harithamma.d@ibm.com>
      
       ## convert.c ##
      @@ convert.c: static void trace_encoding(const char *context, const char *path,
       	struct strbuf trace = STRBUF_INIT;
       	int i;
       
     -+	// If tracing is not on, exit early to avoid high memory footprint
     -+	if (!trace_pass_fl(&coe)) {
     ++	if (!trace_want(&coe))
      +		return;
     -+	}
      +
       	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
       	for (i = 0; i < len && buf; ++i) {
 convert.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to convert.c +3 −0
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;
+
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(

base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9

-- 
gitgitgadget
Junio C Hamano· Jul 24, 2024, 21:41 UTC · re: Haritha via GitGitGadget · lore

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

"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 ;-)
Jeff King· Jul 24, 2024, 22:16 UTC · re: Haritha via GitGitGadget · lore

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

On Wed, Jul 24, 2024 at 11:45:03AM +0000, Haritha  via GitGitGadget wrote:
Show 14 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;
> +
>  	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
>  	for (i = 0; i < len && buf; ++i) {
>  		strbuf_addf(
The patch itself looks good. I confirmed that running:
  git init
  dd if=/dev/zero of=foo.bin bs=1M count=50
  echo '*.bin working-tree-encoding=UTF-16LE' >.gitattributes
  valgrind --tool=massif git add .

goes from a max heap of 1.7G down to 51MB with your patch (whereas I think with the previous iteration it would not have, since the old check did the wrong thing on the first call to trace_encoding()).

-Peff
Haritha via GitGitGadget· Jul 26, 2024, 06:27 UTC · re: Haritha via GitGitGadget · lore

[PATCH v3] Fix to avoid high memory footprint

From: D Harithamma <harithamma.d@ibm.com>

When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING environment variable, the `trace_encoding()` function still allocates & prepares "human readable" copies of the file contents before and after conversion to show in the trace. This results in a high memory footprint and increased runtime without providing any user-visible benefit.

This fix introduces an early exit from the `trace_encoding()` function when tracing is not requested, preventing unnecessary memory allocation and processing.

Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
    Fix to avoid high memory footprint
    
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1744%2FHarithaIBM%2FmemFootprintFix-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1744/HarithaIBM/memFootprintFix-v3
Pull-Request: https://github.com/git/git/pull/1744
Range-diff vs v2:
 1:  500b7eacf2a ! 1:  d864de64380 Fix to avoid high memory footprint
     @@ Metadata
       ## Commit message ##
          Fix to avoid high memory footprint
      
     -    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.
     +    When Git adds a file requiring encoding conversion and tracing of encoding
     +    conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING
     +    environment variable, the `trace_encoding()` function still allocates &
     +    prepares "human readable" copies of the file contents before and after
     +    conversion to show in the trace. This results in a high memory footprint
     +    and increased runtime without providing any user-visible benefit.
     +
     +    This fix introduces an early exit from the `trace_encoding()` function
     +    when tracing is not requested, preventing unnecessary memory allocation
     +    and processing.
      
          Signed-off-by: Harithamma D <harithamma.d@ibm.com>
      
 convert.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to convert.c +3 −0
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;
+
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(

base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9

-- 
gitgitgadget
Torsten Bögershausen· Jul 26, 2024, 09:55 UTC · re: Haritha via GitGitGadget · lore

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

On Fri, Jul 26, 2024 at 06:27:14AM +0000, Haritha  via GitGitGadget wrote:
Show 17 quoted lines
> From: D Harithamma <harithamma.d@ibm.com>
>
> When Git adds a file requiring encoding conversion and tracing of encoding
> conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING
> environment variable, the `trace_encoding()` function still allocates &
> prepares "human readable" copies of the file contents before and after
> conversion to show in the trace. This results in a high memory footprint
> and increased runtime without providing any user-visible benefit.
>
> This fix introduces an early exit from the `trace_encoding()` function
> when tracing is not requested, preventing unnecessary memory allocation
> and processing.
>
> Signed-off-by: Harithamma D <harithamma.d@ibm.com>
> ---
>     Fix to avoid high memory footprint
>
This head line
> Fix to avoid high memory footprint

does not tell to much when and how it happens. The word "fix" is not realy needed (in this project).

Something like
 "convert: avoid high memory footprint"

will tell the reader, that only the convert functionality is affected by this patch.

Thinking about it, another suggestion may be:
convert: Reduce memory allocation when trace_encoding() is not used

If someone browses through the whole history of Git, this is easier to follow.

The exact wording may be improved, important would be to have "convert:"

as the first keyword, and then "memory allocation" and "trace_encoding()" give hints, what this is all about in one line.

And the rest looks good.
Haritha via GitGitGadget· Jul 26, 2024, 14:00 UTC · re: Haritha via GitGitGadget · lore

[PATCH v4] convert: avoid high memory footprint

From: D Harithamma <harithamma.d@ibm.com>

When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING environment variable, the `trace_encoding()` function still allocates & prepares "human readable" copies of the file contents before and after conversion to show in the trace. This results in a high memory footprint and increased runtime without providing any user-visible benefit.

This fix introduces an early exit from the `trace_encoding()` function when tracing is not requested, preventing unnecessary memory allocation and processing.

Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
    Fix to avoid high memory footprint
    
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1744%2FHarithaIBM%2FmemFootprintFix-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1744/HarithaIBM/memFootprintFix-v4
Pull-Request: https://github.com/git/git/pull/1744
Range-diff vs v3:
 1:  d864de64380 ! 1:  50758a4fb94 Fix to avoid high memory footprint
     @@ Metadata
      Author: D Harithamma <harithamma.d@ibm.com>
      
       ## Commit message ##
     -    Fix to avoid high memory footprint
     +    convert: avoid high memory footprint
      
          When Git adds a file requiring encoding conversion and tracing of encoding
          conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING
 convert.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to convert.c +3 −0
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;
+
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(

base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9

-- 
gitgitgadget
Haritha via GitGitGadget· Jul 30, 2024, 03:42 UTC · re: Haritha via GitGitGadget · lore

[PATCH v5] convert: return early when not tracing

From: D Harithamma <harithamma.d@ibm.com>

When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING environment variable, the `trace_encoding()` function still allocates & prepares "human readable" copies of the file contents before and after conversion to show in the trace. This results in a high memory footprint and increased runtime without providing any user-visible benefit.

This fix introduces an early exit from the `trace_encoding()` function when tracing is not requested, preventing unnecessary memory allocation and processing.

Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
    Fix to avoid high memory footprint
    
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1744%2FHarithaIBM%2FmemFootprintFix-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1744/HarithaIBM/memFootprintFix-v5
Pull-Request: https://github.com/git/git/pull/1744
Range-diff vs v4:
 1:  50758a4fb94 ! 1:  e2518b28f1c convert: avoid high memory footprint
     @@ Metadata
      Author: D Harithamma <harithamma.d@ibm.com>
      
       ## Commit message ##
     -    convert: avoid high memory footprint
     +    convert: return early when not tracing
      
          When Git adds a file requiring encoding conversion and tracing of encoding
          conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING
 convert.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to convert.c +3 −0
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;
+
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(

base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9

-- 
gitgitgadget
Junio C Hamano· Jul 31, 2024, 02:42 UTC · re: Haritha via GitGitGadget · lore

Re: [PATCH v5] convert: return early when not tracing

"Haritha  via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 15 quoted lines
> From: D Harithamma <harithamma.d@ibm.com>
>
> When Git adds a file requiring encoding conversion and tracing of encoding
> conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING
> environment variable, the `trace_encoding()` function still allocates &
> prepares "human readable" copies of the file contents before and after
> conversion to show in the trace. This results in a high memory footprint
> and increased runtime without providing any user-visible benefit.
>
> This fix introduces an early exit from the `trace_encoding()` function
> when tracing is not requested, preventing unnecessary memory allocation
> and processing.
>
> Signed-off-by: Harithamma D <harithamma.d@ibm.com>
> ---
It seems that you forgot to adjust to
 https://lore.kernel.org/git/xmqqed7gyyyd.fsf@gitster.g/

where I asked you to be consistent in the authorship name and sign off.

For now, as I like to allow "git shortlog --author=..." to group contributions by a single author to a single bucket, I'll rewrite both to the same name as used in d254e650 (build: support z/OS (OS/390)., 2024-03-06), but will not merge it down to 'next' before I hear what your response is.

Thanks.
Haritha D· Jul 31, 2024, 09:32 UTC · re: Junio C Hamano · lore

RE: [PATCH v5] convert: return early when not tracing

Hey Junio,

You're right. I forgot to respond to that question. I used full name based on Jeff's suggestion. I will ensure it remains consistent. Thank you reminding me.

Best Regards Haritha

On 31/07/24, 8:12 AM, "Junio C Hamano" <gitster@pobox.com <mailto:gitster@pobox.com>> wrote:
"Haritha via GitGitGadget" <gitgitgadget@gmail.com <mailto:gitgitgadget@gmail.com>> writes:
Show 15 quoted lines
> From: D Harithamma <harithamma.d@ibm.com <mailto:harithamma.d@ibm.com>>
>
> When Git adds a file requiring encoding conversion and tracing of encoding
> conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING
> environment variable, the `trace_encoding()` function still allocates &
> prepares "human readable" copies of the file contents before and after
> conversion to show in the trace. This results in a high memory footprint
> and increased runtime without providing any user-visible benefit.
>
> This fix introduces an early exit from the `trace_encoding()` function
> when tracing is not requested, preventing unnecessary memory allocation
> and processing.
>
> Signed-off-by: Harithamma D <harithamma.d@ibm.com <mailto:harithamma.d@ibm.com>>
> ---
It seems that you forgot to adjust to
https://lore.kernel.org/git/xmqqed7gyyyd.fsf@gitster.g/ <https://lore.kernel.org/git/xmqqed7gyyyd.fsf@gitster.g/>

where I asked you to be consistent in the authorship name and sign off.

For now, as I like to allow "git shortlog --author=..." to group contributions by a single author to a single bucket, I'll rewrite both to the same name as used in d254e650 (build: support z/OS (OS/390)., 2024-03-06), but will not merge it down to 'next' before I hear what your response is.

Thanks.
Haritha via GitGitGadget· Jul 31, 2024, 13:33 UTC · re: Haritha via GitGitGadget · lore

[PATCH v6] convert: return early when not tracing

From: D Harithamma <harithamma.d@ibm.com>

When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the GIT_TRACE_WORKING_TREE_ENCODING environment variable, the `trace_encoding()` function still allocates & prepares "human readable" copies of the file contents before and after conversion to show in the trace. This results in a high memory footprint and increased runtime without providing any user-visible benefit.

This fix introduces an early exit from the `trace_encoding()` function when tracing is not requested, preventing unnecessary memory allocation and processing.

Signed-off-by: D Harithamma <harithamma.d@ibm.com>
---
    Fix to avoid high memory footprint
    
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1744%2FHarithaIBM%2FmemFootprintFix-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1744/HarithaIBM/memFootprintFix-v6
Pull-Request: https://github.com/git/git/pull/1744
Range-diff vs v5:
 1:  e2518b28f1c ! 1:  7b68aa9de1d convert: return early when not tracing
     @@ Commit message
          when tracing is not requested, preventing unnecessary memory allocation
          and processing.
      
     -    Signed-off-by: Harithamma D <harithamma.d@ibm.com>
     +    Signed-off-by: D Harithamma <harithamma.d@ibm.com>
      
       ## convert.c ##
      @@ convert.c: static void trace_encoding(const char *context, const char *path,
 convert.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to convert.c +3 −0
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;
+
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(

base-commit: 557ae147e6cdc9db121269b058c757ac5092f9c9

-- 
gitgitgadget
Junio C Hamano· Jul 26, 2024, 15:06 UTC · re: Haritha via GitGitGadget · lore

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

"Haritha  via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: D Harithamma <harithamma.d@ibm.com>
> ...
> Signed-off-by: Harithamma D <harithamma.d@ibm.com>

I am assuming that you and the person who did d254e650 (build: support z/OS (OS/390)., 2024-03-06) are the same person? That commit was signed off like so:

    commit d254e65092daba8667d6b4d5b4f59c099c1edd1f
    Author: Haritha D <harithamma.d@ibm.com>
    Date:   Wed Mar 6 05:44:17 2024 +0000
        build: support z/OS (OS/390).
        Introduced z/OS (OS/390) as a platform in config.mak.uname
        Signed-off-by: Haritha D <harithamma.d@ibm.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

It is OK if you really want to use a longer name this time, but then please be consistent within a single commit. The author name of the proposed commit is "D Harithamma" and a different name "Harithamma D" is used to sign off the commit, which is not what we want to see.

Thanks.
Junio C Hamano· Jul 26, 2024, 15:12 UTC · re: Haritha via GitGitGadget · lore

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

"Haritha  via GitGitGadget" <gitgitgadget@gmail.com> writes:
Another thing.
> Subject: Re: [PATCH v3] Fix to avoid high memory footprint

This does not tell us much about what area had problem under what condition. "git shortlog --no-merges -200 master" may give us good examples of how we typically write the title of our commits.

In the case of this change, ideally we should be able to tell that this is about tracing the conversion codepath.

    Subject: [PATCH vN] encoding: return early when not tracing conversion
or something, perhaps?
Haritha D· Jul 30, 2024, 03:41 UTC · re: Junio C Hamano · lore

RE: [PATCH v3] Fix to avoid high memory footprint

Hello Team,
I have retained convert as per Torsten's comment. Rest i have changed as per Junio's suggestion. 
Thank you, everyone.
On 26/07/24, 8:42 PM, "Junio C Hamano" <gitster@pobox.com <mailto:gitster@pobox.com>> wrote:
"Haritha via GitGitGadget" <gitgitgadget@gmail.com <mailto:gitgitgadget@gmail.com>> writes:
Another thing.
> Subject: Re: [PATCH v3] Fix to avoid high memory footprint

This does not tell us much about what area had problem under what condition. "git shortlog --no-merges -200 master" may give us good examples of how we typically write the title of our commits.

In the case of this change, ideally we should be able to tell that this is about tracing the conversion codepath.

Subject: [PATCH vN] encoding: return early when not tracing conversion
or something, perhaps?

← back to recent threads