Re: [PATCH] commit: remove padding from struct commit

[Junio C Hamano <gitster@pobox.com>]

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

> From: Kristofer Karlsson <krka@spotify.com>
>
> On 64-bit platforms, "struct commit" is 80 bytes, 8 of which are
> padding:
>  - 4 bytes between .object and .date
>  - 4 bytes of tail padding after .index
> ...
> Measuring peak RSS of "git rev-list --all" over an internal monorepo
> with ~4.5 million commits (three runs each):
>
>              peak RSS     time
>   before     ~849 MiB   12.8-13.7s
>   after      ~817 MiB   12.8-13.0s
>
> The ~32 MiB saved is close to the expected 8 bytes * 4.5M commits
> (~34 MiB); running time is unchanged within noise.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
>     I also prototyped a regression test that compares the size of "struct
>     commit" and "struct object" against the sum of their members' sizes,
>     plus any expected padding. I left it out of this patch because I am not
>     sure whether:
>     
>      1. such a test is worth the cost of updating it whenever members are
>         added or removed, or
>     
>      2. a unit test is the right place for it.
>     
>     I am happy to send it as a follow-up if there is interest.

The comment before this structure says ...

> diff --git a/commit.h b/commit.h
> index 1061ed791b..66a5d245f2 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -26,6 +26,7 @@ struct commit_list {
>   */

/*
 * The size of this struct matters in full repo walk operations like
 * 'git clone' or 'git gc'. Consider using commit-slab to attach data
 * to a commit instead of adding new fields here.
 */

but apparently it did not have much effect to prevent this from
happening.

I would think it is an effective way to force people to think struct
size bloat of important and populous structure to always measure and
warn.  There may be better ways to achieve the same that others may
think of, but I very much appreciate your bringing it up, even only
as a discussion starter.

>  struct commit {
>  	struct object object;
> +	unsigned int index;
>  	timestamp_t date;
>  	struct commit_list *parents;
>  
> @@ -35,7 +36,6 @@ struct commit {
>  	 * or get_commit_tree_oid().
>  	 */
>  	struct tree *maybe_tree;
> -	unsigned int index;
>  };

Obviously, ordering members based on size, not their logical
relationships, has its downside.  The "object" being at offset 0 is
an integral part of our in-core object design (fake inheritance in
C), but the updated layout gives a false sense that somehow "index"
may have a more important meaning than say "date" or "parents".

We may want to have some comments next to "index" (which is a unique
number given to all in-core commits while they are held in memory,
to allow the commit slab subsystem to use it as an index into flat
array of things) and "date" (which is the committer date taken from
the underlying commit object, without any adjustment), which are two
of these members whose meanings are not immediately obvious.