[PATCH] commit: remove padding from struct commit

[PATCH] commit: remove padding from struct commit

[Kristofer Karlsson via GitGitGadget]

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

Reduce memory usage by moving .index into the hole after .object.
On 64-bit platforms this shrinks the struct to 72 bytes and
eliminates the padding, resulting in 10% less memory for every commit
allocated, with no change in behaviour.

On platforms where 8-byte members only need 4-byte alignment (e.g.
i386) both layouts are already free of padding, so the size stays the
same there.

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>
---
    commit: remove padding from struct commit
    
    I stumbled on this while rabbit-holing on an unrelated change to the
    commit walk, by looking at the layout with gdb's "ptype /o".
    
    Digging into the history showed that the size and padding of "struct
    commit" have changed several times, sometimes undoing earlier
    optimizations.
    
    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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2309%2Fspkrka%2Fcommit-index-hole-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2309/spkrka/commit-index-hole-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2309

 commit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 {
  */
 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;
 };
 
 extern int save_commit_buffer;

base-commit: 1891de46f79dc444a47db1c87bd0c8c76b5463f2
-- 
gitgitgadget

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

[Derrick Stolee]

On 11/12/2026 11:14 AM, Kristofer Karlsson via GitGitGadget wrote:
> 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

Good find.

> Reduce memory usage by moving .index into the hole after .object.
> On 64-bit platforms this shrinks the struct to 72 bytes and
> eliminates the padding, resulting in 10% less memory for every commit
> allocated, with no change in behaviour.
> 
> On platforms where 8-byte members only need 4-byte alignment (e.g.
> i386) both layouts are already free of padding, so the size stays the
> same there.
> 
> 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.

Thanks for measuring the memory load improvement. And of course this
won't have any impact on 32-bit machines as the struct will use
4-byte alignment by default.

>  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;
>  };
This certainly looks like it should be obviously correct. I
believe that the only order-critical part is the object
struct needing to be first so we can "cast" a commit to an
object.

Thanks,
-Stolee

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

[Junio C Hamano]

"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.

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

[Kristofer Karlsson]

On Thu, 12 Nov 2026 at 20:22, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

That does indeed appear to be true.  I looked into the history of
changes here for my related test prototype and found that the
layout had changed several times.  These are all measured from
x86_64 builds of the respective commits, using gdb's "ptype /o":

                  size  padding
  2018              72        8
  2018 25f859fdf4   64        0   move "index" next to "graph_pos"
  2018 9d2c97016f   56        0   remove "util"
  2018 83073cc994   64        4   add "generation"
  2018 b45424181e   72        8   FLAG_BITS 27 -> 29
  2019 13eeedb5d1   80        4   SHA-256 grows object_id
  2020 c49c82aa4c   72        4   graph_pos/generation to a slab
  2020 23c4319f0d   72        8   FLAG_BITS 29 -> 28
  2021 cf0983213c   72        4   object_id gains "algo"
  2026 b4e8f60a3c   80        8   FLAG_BITS 28 -> 29
  now               72        0   this patch

While each individual change was good, I think it shows that at the
very least, it's hard to reason about the total size.  Adding a field
may either reuse existing padding and thus come for free, or create
additional padding and thus cost double.

> 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.

I can send a test as a separate follow-up, so people can weigh in
on the approach independently of this patch.  One thing that couples
them a bit is that the expected padding is more complex to express
without this patch, since it depends on the alignment rules of each
platform.

Another option I have tried is -Wpadded scoped to the struct via
a pair of macros:

  BEGIN_NO_PADDING
  struct commit {
struct object object;
unsigned int index;
...
  };
  END_NO_PADDING

The macros would live in git-compat-util.h next to
DISABLE_WARNING(), and expand to nothing on other compilers, which
is fine, since we only need to catch it once in the CI builds.

This works with gcc and clang and is lighter than a unit test, but
can only enforce zero padding, so it would not work for "struct
object", which has unavoidable padding with the current FLAG_BITS.
If "struct commit" ever ends up with padding we cannot avoid, we
would have to drop the macros, but that would only lose the extra
protection.

> 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".

I agree that ordering fields for alignment rather than logical
relationships makes the structure harder to read.  Ideally the
compiler could arrange them for us, if we could tell it that only
"object" needs to come first.

It is also somewhat fiddly: growing "struct object" by 4 bytes can
change the padding in "struct commit" without touching commit.h at
all, which is what happened with b4e8f60a3c above.

Still, given how many instances of this structure we allocate, it
may be worth optimizing the layout.  The history above suggests that
a comment alone may not be enough to catch accidental regressions.

> 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.

I can add short comments on "index" and "date" for a v2, along
with a note that only "object" needs to come first.

Thanks for the review!
Kristofer

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

[Kristofer Karlsson]

On Thu, 12 Nov 2026 at 19:00, Derrick Stolee <stolee@gmail.com> wrote:
>
> >  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;
> >  };
> This certainly looks like it should be obviously correct. I
> believe that the only order-critical part is the object
> struct needing to be first so we can "cast" a commit to an
> object.

Agreed, and it is an important invariant, since nothing enforces
it at compile time.  Out of curiosity I tried moving "object" to
the end of the struct: it builds without any warnings, and then
fails in a variety of ways at runtime:

  git cat-file -t HEAD     works (does not use struct commit)
  git log                  fatal: HEAD is unknown object
  git log HEAD~5           error: object ... is a (null), not a commit
  git commit               segfault

If we want the compiler to enforce it as well, something like

  (void)BUILD_ASSERT_OR_ZERO(offsetof(struct commit, object) == 0);

in init_commit_node() could work, and the same could be done for
trees, blobs and tags.

I added some more detailed followup in the parallel reply to Junio.

Thanks for the review!
Kristofer

[PATCH v2] commit: remove padding from struct commit

[Kristofer Karlsson via GitGitGadget]

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

Reduce memory usage by moving .index into the hole after .object.
On 64-bit platforms this shrinks the struct to 72 bytes and
eliminates the padding, resulting in 10% less memory for every commit
allocated, with no change in behaviour.

On platforms where 8-byte members only need 4-byte alignment (e.g.
i386) both layouts are already free of padding, so the size stays the
same there.

Since the order of the members no longer reflects their importance,
document that only "object" has to come first, and describe "index"
and "date", whose meanings are not obvious from their names.

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>
---
    commit: remove padding from struct commit
    
    I stumbled on this while rabbit-holing on an unrelated change to the
    commit walk, by looking at the layout with gdb's "ptype /o".
    
    Changes since v1:
    
     * Document that "object" must be the first member, and that the order
       of the other members is partly chosen for a compact memory layout
       rather than reflecting their importance (Junio).
    
     * Add comments describing "index" and "date" (Junio).
    
    A regression check for the padding (unit test or a scoped -Wpadded) and
    a build-time assertion that "object" comes first are discussed in the v1
    thread; I will send those separately.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2309%2Fspkrka%2Fcommit-index-hole-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2309/spkrka/commit-index-hole-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2309

Range-diff vs v1:

 1:  20de8db00d ! 1:  f8f0dbcf46 commit: remove padding from struct commit
     @@ Commit message
          i386) both layouts are already free of padding, so the size stays the
          same there.
      
     +    Since the order of the members no longer reflects their importance,
     +    document that only "object" has to come first, and describe "index"
     +    and "date", whose meanings are not obvious from their names.
     +
          Measuring peak RSS of "git rev-list --all" over an internal monorepo
          with ~4.5 million commits (three runs each):
      
     @@ Commit message
      
       ## commit.h ##
      @@ commit.h: 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.
     ++ *
     ++ * The order of the members is partly chosen to keep the memory layout
     ++ * compact (avoiding alignment padding), and does not necessarily
     ++ * reflect their importance or grouping.
        */
       struct commit {
     ++	/*
     ++	 * Must be the first member, so that a "struct object *" can be
     ++	 * cast to a "struct commit *".
     ++	 */
       	struct object object;
     ++
     ++	/*
     ++	 * Index into the commit-slab arrays: unique per in-core commit,
     ++	 * and only meaningful within the current process.
     ++	 */
      +	unsigned int index;
     ++
     ++	/*
     ++	 * The committer date taken from the commit object, without any
     ++	 * adjustment.
     ++	 */
       	timestamp_t date;
     ++
       	struct commit_list *parents;
       
     + 	/*
      @@ commit.h: struct commit {
       	 * or get_commit_tree_oid().
       	 */


 commit.h | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/commit.h b/commit.h
index 1061ed791b..d43cb1e46d 100644
--- a/commit.h
+++ b/commit.h
@@ -23,10 +23,30 @@ 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.
+ *
+ * The order of the members is partly chosen to keep the memory layout
+ * compact (avoiding alignment padding), and does not necessarily
+ * reflect their importance or grouping.
  */
 struct commit {
+	/*
+	 * Must be the first member, so that a "struct object *" can be
+	 * cast to a "struct commit *".
+	 */
 	struct object object;
+
+	/*
+	 * Index into the commit-slab arrays: unique per in-core commit,
+	 * and only meaningful within the current process.
+	 */
+	unsigned int index;
+
+	/*
+	 * The committer date taken from the commit object, without any
+	 * adjustment.
+	 */
 	timestamp_t date;
+
 	struct commit_list *parents;
 
 	/*
@@ -35,7 +55,6 @@ struct commit {
 	 * or get_commit_tree_oid().
 	 */
 	struct tree *maybe_tree;
-	unsigned int index;
 };
 
 extern int save_commit_buffer;

base-commit: 1891de46f79dc444a47db1c87bd0c8c76b5463f2
-- 
gitgitgadget

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

[Derrick Stolee]

On 11/13/2026 8:30 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>

>  struct commit {
> +	/*
> +	 * Must be the first member, so that a "struct object *" can be
> +	 * cast to a "struct commit *".
> +	 */
>  	struct object object;

I think your comment is backwards: We cast a commit into an object
and it works. We can't cast an arbitrary object to a commit.

But maybe that direction _does_ happen if we check the object type
and see that it's actually a commit underneath.

> +
> +	/*
> +	 * Index into the commit-slab arrays: unique per in-core commit,
> +	 * and only meaningful within the current process.
> +	 */
> +	unsigned int index;
> +
> +	/*
> +	 * The committer date taken from the commit object, without any
> +	 * adjustment.
> +	 */
>  	timestamp_t date;
> +

Perhaps instead of "taken from the commit object" you could say
"as specified by the commit object" because we may actually load
the date from the commit-graph file instead of parsing a commit
object.

My comments are only nitpicks that _might_ improve the comments.
But they might not, so do what you will.

Thanks,
-Stolee

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

[Oswald Buddenhagen]

On Thu, Nov 12, 2026 at 10:35:42PM +0100, Kristofer Karlsson wrote:
>If "struct commit" ever ends up with padding we cannot avoid,

this is a non-concern - one can always introduce (arch-dependent) dummy 
members to make the compiler happy. not beautiful, but justifiable.

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

[Kristofer Karlsson]

On Fri, 13 Nov 2026 at 15:57, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 11/13/2026 8:30 AM, Kristofer Karlsson via GitGitGadget wrote:
> > From: Kristofer Karlsson <krka@spotify.com>
>
> >  struct commit {
> > +     /*
> > +      * Must be the first member, so that a "struct object *" can be
> > +      * cast to a "struct commit *".
> > +      */
> >       struct object object;
>
> I think your comment is backwards: We cast a commit into an object
> and it works. We can't cast an arbitrary object to a commit.
>
> But maybe that direction _does_ happen if we check the object type
> and see that it's actually a commit underneath.

Hm, I suppose both directions do happen:

  1. From object to commit requires knowing the type first to be
     safe, and relies on "object" being the first member.
  2. From commit to object could use &commit->object instead, which
     is what most of the code does, and is why I only mentioned the
     other direction.

But looking more closely, there are also 24 plain casts to "struct
object *", next to about 100 uses of &x->object (and about 110
casts the other way).  The casts fall into two groups.

Some cast the result of a lookup function, e.g. in tag.c:

    item->tagged = (struct object *)lookup_commit(r, &oid);

Here the cast is arguably the right tool, since lookup_commit() can
return NULL and the cast passes that through, while taking
&lookup_commit(r, &oid)->object would be undefined behaviour.

The rest cast a commit we already have, e.g. in bundle.c:

    object = (struct object *)commit;

where &commit->object, as used in e.g. revision.c:

    if (get_name_decoration(&commit->object))

would do the same without relying on the layout.  I am not sure it
is worth unifying these, but it might be worth settling on one style
for new development.

> Perhaps instead of "taken from the commit object" you could say
> "as specified by the commit object" because we may actually load
> the date from the commit-graph file instead of parsing a commit
> object.

Good point.  "commit object" is also a bit ambiguous in a comment
for "struct commit", so I could instead say "The committer date
of this commit, without any adjustment.", which covers both cases.

Thanks!
Kristofer

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

[Junio C Hamano]

Kristofer Karlsson <krka@spotify.com> writes:

> On Fri, 13 Nov 2026 at 15:57, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 11/13/2026 8:30 AM, Kristofer Karlsson via GitGitGadget wrote:
>> > From: Kristofer Karlsson <krka@spotify.com>
>>
>> >  struct commit {
>> > +     /*
>> > +      * Must be the first member, so that a "struct object *" can be
>> > +      * cast to a "struct commit *".
>> > +      */
>> >       struct object object;
>>
>> I think your comment is backwards: We cast a commit into an object
>> and it works. We can't cast an arbitrary object to a commit.

I had the exact reaction.  In our "cheap C inheritance" pattern
downcasting commit to object can be done unconditionally, but
upcasting needs to be done with out of band knowledge (because these
struct types do not have "type" signature).  So

    ... so that a "struct commit *" can be cast to a "struct object
    *".  The code casts in other direction only when the object is
    known to be a commit.

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

[Junio C Hamano]

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

> Since the order of the members no longer reflects their importance,

Micronit: "no longer reflects logical grouping or impotance" or
something, just like you already have in the comment in the header
file.

> diff --git a/commit.h b/commit.h
> index 1061ed791b..d43cb1e46d 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -23,10 +23,30 @@ 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.
> + *
> + * The order of the members is partly chosen to keep the memory layout
> + * compact (avoiding alignment padding), and does not necessarily
> + * reflect their importance or grouping.
>   */
>  struct commit {
> +	/*
> +	 * Must be the first member, so that a "struct object *" can be
> +	 * cast to a "struct commit *".
> +	 */

Either append "... when we know it is a commit", swap the order, or
do both.  My preference is the second one which would leave the
shortest result.

>  	struct object object;
> +
> +	/*
> +	 * Index into the commit-slab arrays: unique per in-core commit,
> +	 * and only meaningful within the current process.
> +	 */
> +	unsigned int index;

Nice.  It is on a rather longer side and "and only meaningful..."
may be redundant after saying "in-core", but nothing is wrong.

Thanks.  Will queue.