revision: free commit buffers for skipped commits

Commit Message

Jeff King Aug. 30, 2024, 8:53 p.m. UTC

On Fri, Aug 30, 2024 at 03:20:15PM +0300, Yuri Karnilaev wrote:

> As you can see from the results, the peak memory usage when processing
> commits in batches is 10 times higher than when processing all commits
> in one go.
> Can you please explain why this happens? Is there a way to work around
> this? Or maybe can you fix this in future Git versions?

Try this:

-- >8 --
Subject: [PATCH] revision: free commit buffers for skipped commits

In git-log we leave the save_commit_buffer flag set to "1", which tells
the commit parsing code to store the object content after it has parsed
it to find parents, tree, etc. That lets us reuse the contents for
pretty-printing the commit in the output. And then after printing each
commit, we call free_commit_buffer(), since we don't need it anymore.

But some options may cause us to traverse commits which are not part of
the output. And so git-log does not see them at all, and doesn't free
them. One such case is something like:

  git log -n 1000 --skip=1000000

which will churn through a million commits, before showing only a
thousand. We loop through these inside get_revision(), without freeing
the contents. As a result, we end up storing the object data for those
million commits simultaneously.

We should free the stored buffers (if any) for those commits as we skip
over them, which is what this patch does. Running the above command in
linux.git drops the peak heap usage from ~1.1GB to ~200MB, according to
valgrind/massif. (I thought we might get an even bigger improvement, but
the remaining memory is going to commit/tree structs, which we do hold
on to forever).

Note that this problem doesn't occur if:

  - you're running a git-rev-list without a --format parameter; it turns
    off save_commit_buffer by default, since it only output the object
    id

  - you've built a commit-graph file, since in that case we'd use the
    optimized graph data instead of the initial parse, and then do a
    lazy parse for commits we're actually going to output

There are probably some other option combinations that can likewise
end up with useless stored commit buffers. For example, if you ask for
"foo..bar", then we'll have to walk down to the merge base, and
everything on the "foo" side won't be shown. Tuning the "save" behavior
to handle that might be tricky (I guess maybe drop buffers for anything
we mark as UNINTERESTING?). And in the long run, the right solution here
is probably to make sure the commit-graph is built (since it fixes the
memory problem _and_ drastically reduces CPU usage).

But since this "--skip" case is an easy one-liner, it's worth fixing in
the meantime. It should be OK to make this call even if there is no
saved buffer (e.g., because save_commit_buffer=0, or because a
commit-graph was used), since it's O(1) to look up the buffer and is a
noop if it isn't present. I verified by running the above command after
"git commit-graph write --reachable", and it takes the same time with
and without this patch.

Reported-by: Yuri Karnilaev <[e-mail]>
Signed-off-by: Jeff King <[e-mail]>
---
 revision.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Junio C Hamano Aug. 30, 2024, 9:27 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> But since this "--skip" case is an easy one-liner, it's worth fixing in
> the meantime.

OK.

> diff --git a/revision.c b/revision.c
> index ac94f8d429..2d7ad2bddf 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -4407,6 +4407,7 @@ static struct commit *get_revision_internal(struct rev_info *revs)
>  				c = get_revision_1(revs);
>  				if (!c)
>  					break;
> +				free_commit_buffer(revs->repo->parsed_objects, c);
>  			}

Even if we freed the buffer and then later need it, we'd read the
buffer again anyway, so this is a safe thing to do.  And because
commits skipped in this separate loop will _never_ be given to the
caller of get_revision(), this it a reasonable optimization, too.

Will queue.  Thanks.

Patch

diff --git a/revision.c b/revision.c
index ac94f8d429..2d7ad2bddf 100644
--- a/revision.c
+++ b/revision.c
@@ -4407,6 +4407,7 @@  static struct commit *get_revision_internal(struct rev_info *revs)
 				c = get_revision_1(revs);
 				if (!c)
 					break;
+				free_commit_buffer(revs->repo->parsed_objects, c);
 			}
 		}