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

From: jnareb@gmail.com (Jakub Narębski)
To: Abhishek Kumar <abhishekkumar8222@gmail.com>
Cc: git@vger.kernel.org, "Derrick Stolee" <stolee@gmail.com>,
	"Jakub Narębski" <jnareb@gmail.com>
Subject: Re: [GSoC Patch 0/3] Move generation, graph_pos to a slab
Date: Fri, 05 Jun 2020 21:00:56 +0200	[thread overview]
Message-ID: <85ftb9wd5j.fsf@gmail.com> (raw)
In-Reply-To: <20200604072759.19142-1-abhishekkumar8222@gmail.com> (Abhishek Kumar's message of "Thu, 4 Jun 2020 12:57:56 +0530")

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

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

Very minor nitpick: this sentence would read better if the names of
`generation` and `graph_pos` fields (but especially the 'generation')
were quoted.

>
> This wastage would have been more pronounced as transistion to
> generation number v2, which uses 64-bit generation number instead of
> current 32-bits.

Good.  Moving reachability index value into a commit slab was one of
prerequisites to switching to the generation number v2, see [2]

[2]: https://public-inbox.org/git/cfa2c367-5cd7-add5-0293-caa75b103f34@gmail.com/t/#u

The other prerequisite was proper handling of commit-graph format
change, either by using "metadata chunk" as more flexible replacement of
mishandled format version field in the commit-graph file header, or as
proposed in [3] (and subsequent posts), removing "CDAT" chunk and
replacing it with "CDA2" chunk.

[3]: https://public-inbox.org/git/xmqq369z7i1b.fsf@gitster.c.googlers.com/t/#u

Also, we should probably stop mishandling the format version field, that
is do not error out [4] when commit-graph version of the file does not
match version supported by git code running the command, but just simply
not use the commit-graph (like it is done for Bloom filter chunks).

[4]: https://github.com/git/git/blob/master/commit-graph.c#L253

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

It is nice to see the use of Coccinelle scripts.

>  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

Best,
-- 
Jakub Narębski