Re: [GSoC Patch 0/3] Move generation, graph_pos to a slab

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 4, 2020, 14:22 UTC
Message-ID: <b850637d-a7ca-e8f9-5009-657096ea2975@gmail.com>
In-Reply-To: <20200604072759.19142-1-abhishekkumar8222@gmail.com>

On 6/4/2020 3:27 AM, Abhishek Kumar wrote:

Show 7 quoted lines

> The struct commit is used in many contexts. However, members generation
> and graph_pos are only used for commit-graph related operations and
> otherwise waste memory.
> 
> This wastage would have been more pronounced as transistion to
> generation number v2, which uses 64-bit generation number instead of
> current 32-bits.

Thanks! This is an important step, and will already improve performance in subtle ways.

Show 11 quoted lines

> The third patch ("commit: convert commit->graph_pos to a slab",
> 2020-06-04) is currently failing diff-submodule related tests (t4041,
> t4059 and t4060) for gcc [1]. I am going to send a second version soon,
> fixing that.
> 
> [1]: https://travis-ci.com/github/abhishekkumar2718/git/jobs/343441189
> 
> Abhishek Kumar (3):
>   commit: introduce helpers for generation slab
>   commit: convert commit->generation to a slab
>   commit: convert commit->graph_pos to a slab

If we have a commit-graph file, then we have graph_pos and generation both coming from that file. Perhaps it would be better to combine the data into a single slab that stores a "struct commit_graph_data" or something?

This would change only the slab definitions, since you already do a good job of wrapping the slab access in methods.

Show 14 quoted lines

>  alloc.c                             |   2 -
>  blame.c                             |   2 +-
>  bloom.c                             |   6 +-
>  commit-graph.c                      | 116 +++++++++++++++++++++-------
>  commit-graph.h                      |   8 ++
>  commit-reach.c                      |  50 ++++++------
>  commit.c                            |   6 +-
>  commit.h                            |   6 --
>  contrib/coccinelle/generation.cocci |  12 +++
>  contrib/coccinelle/graph_pos.cocci  |  12 +++
>  revision.c                          |  16 ++--
>  11 files changed, 158 insertions(+), 78 deletions(-)
>  create mode 100644 contrib/coccinelle/generation.cocci
>  create mode 100644 contrib/coccinelle/graph_pos.cocci

I appreciate the Coccinelle scripts to help identify automatic fixes for other topics in-flight. However, I wonder if they would be better placed inside the existing commit.cocci file?

Thanks, -Stolee