parse_object_buffer(): respect save_commit_buffer

If the global variable "save_commit_buffer" is set to 0, then
parse_commit() will throw away the commit object data after parsing it,
rather than sticking it into a commit slab. This goes all the way back
to 60ab26de99 ([PATCH] Avoid wasting memory in git-rev-list,
2005-09-15).

But there's another code path which may similarly stash the buffer:
parse_object_buffer(). This is where we end up if we parse a commit via
parse_object(), and it's used directly in a few other code paths like
git-fsck.

The original goal of 60ab26de99 was avoiding extra memory usage for
rev-list. And there it's not all that important to catch parse_object().
We use that function only for looking at the tips of the traversal, and
the majority of the commits are parsed by following parent links, where
we use parse_commit() directly. So we were wasting some memory, but only
a small portion.

It's much easier to see the effect with fsck. Since we now turn off
save_commit_buffer by default there, we _should_ be able to drop the
freeing of the commit buffer in fsck_obj(). But if we do so (taking the
first hunk of this patch without the rest), then the peak heap of "git
fsck" in a clone of git.git goes from 136MB to 194MB. Teaching
parse_object_buffer() to respect save_commit_buffer brings that down to
134.5MB (it's hard to tell from massif's output, but I suspect the
savings comes from avoiding the overhead of the mostly-empty commit
slab).

Other programs should see a small improvement. Both "rev-list --all" and
"fsck --connectivity-only" improve by a few hundred kilobytes, as they'd
avoid loading the tip objects of their traversals.

Most importantly, no code should be hurt by doing this. Any program that
turns off save_commit_buffer is already making the assumption that any
commit it sees may need to have its object data loaded on demand, as it
doesn't know which ones were parsed by parse_commit() versus
parse_object(). Not to mention that anything parsed by the commit graph
may be in the same boat, even if save_commit_buffer was not disabled.

This should be the only spot that needs to be fixed. Grepping for
set_commit_buffer() shows that this and parse_commit() are the only
relevant calls.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 38922a369d6eff7e75e68a451bc192cc6ccec43d..a7592ef5ff8020b5ae906482ed92cfe08012134f 100644 (file)
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -439,9 +439,6 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)
 out:
        if (obj->type == OBJ_TREE)
                free_tree_buffer((struct tree *)obj);
-       if (obj->type == OBJ_COMMIT)
-               free_commit_buffer(the_repository->parsed_objects,
-                                  (struct commit *)obj);
        return err;
 }
 

diff --git a/object.c b/object.c
index 588b8156f1d634a5cb9c25038961ca9a0d28d255..868e623719325b8444f8e352308564c9b066334d 100644 (file)
--- a/object.c
+++ b/object.c
@@ -233,7 +233,8 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id
                if (commit) {
                        if (parse_commit_buffer(r, commit, buffer, size, 1))
                                return NULL;
-                       if (!get_cached_commit_buffer(r, commit, NULL)) {
+                       if (save_commit_buffer &&
+                           !get_cached_commit_buffer(r, commit, NULL)) {
                                set_commit_buffer(r, commit, buffer, size);
                                *eaten_p = 1;
                        }