Fix to avoid high memory footprint
PATCH 1/1 Fix to avoid high memory footprint
From: D Harithamma <[email protected]>
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.
---
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(+)
convert.c +5 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 19 lines · Haritha via GitGitGadget
From: D Harithamma <[email protected]>
This fix avoids high memory footprint when adding files that require conversion.
Git has atrace_encodingroutine 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 forGIT_TRACE_WORKING_TREE_ENCODINGis done after the string is allocated. This resolves high memory footprints even whenGIT_TRACE_WORKING_TREE_ENCODINGis not active. This fix adds an early exit to avoid the unnecessary memory allocation.
Quoted 1 line
Signed-off-by Haritha D <[email protected]>
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:
Quoted 10 lines
This fix avoids high memory footprint when adding files that require conversion. Git has a
trace_encodingroutine 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 forGIT_TRACE_WORKING_TREE_ENCODINGis done after the string is allocated. This resolves high memory footprints even whenGIT_TRACE_WORKING_TREE_ENCODINGis 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:
convert.c static void trace_encoding(const char *context, const char *path, · line 330
Quoted 10 lines
convert.c +4
-0@@ -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
PATCH v2 1/1 Fix to avoid high memory footprint
In reply to Haritha via GitGitGadgetFrom: D Harithamma <[email protected]>
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.
---
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 <[email protected]>
+ Signed-off-by: Harithamma D <[email protected]>
## 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(+)
convert.c +3 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 12 lines · Haritha via GitGitGadget
From: D Harithamma <[email protected]>
This fix avoids high memory footprint when adding files that require conversion. Git has a
trace_encodingroutine 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 forGIT_TRACE_WORKING_TREE_ENCODINGis done after the string is allocated. This resolves high memory footprints even whenGIT_TRACE_WORKING_TREE_ENCODINGis 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.
Quoted 1 line
Signed-off-by Harithamma D <[email protected]>
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.
convert.c static void trace_encoding(const char *context, const char *path, · line 329
Quoted 11 lines
convert.c +3
-0@@ -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 ;-)
Quoted 5 lines · Haritha via GitGitGadget
convert.c +0
-0
convert.c static void trace_encoding(const char *context, const char *path, · line 332
Quoted 9 lines
convert.c +3
-0@@ -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
PATCH v3 1/1 Fix to avoid high memory footprint
In reply to Haritha via GitGitGadgetFrom: D Harithamma <[email protected]>
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.
---
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.
convert.c | 3 +++ 1 file changed, 3 insertions(+)
convert.c +3 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 9 lines · Haritha via GitGitGadget
From: D Harithamma <[email protected]>
When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the
GIT_TRACE_WORKING_TREE_ENCODINGenvironment variable, thetrace_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.
Quoted 8 lines
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 <[email protected]>---
Fix to avoid high memory footprint
This head line
Quoted 1 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.
PATCH v4 1/1 convert: avoid high memory footprint
In reply to Haritha via GitGitGadgetFrom: D Harithamma <[email protected]>
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.
---
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 <[email protected]>
## 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(+)
convert.c +3 -0
@@ -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
Signature · 1 line
gitgitgadget
PATCH v5 1/1 convert: return early when not tracing
From: D Harithamma <[email protected]>
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.
---
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 <[email protected]>
## 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(+)
convert.c +3 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v5] convert: return early when not tracing
Quoted 9 lines · Haritha via GitGitGadget
From: D Harithamma <[email protected]>
When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the
GIT_TRACE_WORKING_TREE_ENCODINGenvironment variable, thetrace_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.
Quoted 6 lines
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 <[email protected]>---
It seems that you forgot to adjust to
https://lore.kernel.org/git/[email protected]/
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.
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" <[email protected] <mailto:[email protected]>> wrote:
Quoted 9 lines · "Haritha via GitGitGadget" <[email protected]…
From: D Harithamma <[email protected] <mailto:[email protected]>>
When Git adds a file requiring encoding conversion and tracing of encoding conversion is not requested via the
GIT_TRACE_WORKING_TREE_ENCODINGenvironment variable, thetrace_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.
Quoted 6 lines
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 <[email protected] <mailto:[email protected]>>---
It seems that you forgot to adjust to
https://lore.kernel.org/git/[email protected]/ <https://lore.kernel.org/git/[email protected]/>
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.
PATCH v6 1/1 convert: return early when not tracing
In reply to Haritha via GitGitGadgetFrom: D Harithamma <[email protected]>
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.
---
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 <[email protected]>
+ Signed-off-by: D Harithamma <[email protected]>
## convert.c ##
@@ convert.c: static void trace_encoding(const char *context, const char *path,
convert.c | 3 +++ 1 file changed, 3 insertions(+)
convert.c +3 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 3 lines · Haritha via GitGitGadget
From: D Harithamma <[email protected]>
...Signed-off-by Harithamma D <[email protected]>
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 <[email protected]>
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
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.
"Haritha via GitGitGadget" <[email protected]> writes:
Another thing.
Quoted 1 line
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?
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" <[email protected] <mailto:[email protected]>> wrote:
"Haritha via GitGitGadget" <[email protected] <mailto:[email protected]>> writes:
Another thing.
Quoted 1 line
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?