Re: [PATCH v2 14/14] commit.h: delete 'util' field in struct commit
- From
Derrick Stolee <stolee@gmail.com>
- Date
- May 14, 2018, 18:14 UTC
- Message-ID
- <5ee75fc6-562e-d424-0bc3-c9997dba92db@gmail.com>
- In-Reply-To
- <CACsJy8DhUw6L7j3Pkqji_K0WMjFZxs315emmvdyNQJJN4VTwgw@mail.gmail.com>
On 5/14/2018 1:30 PM, Duy Nguyen wrote:
Show 38 quoted lines
> On Mon, May 14, 2018 at 6:07 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Mon, May 14, 2018 at 04:52:29PM +0900, Junio C Hamano wrote:
>>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>>
>>>> diff --git a/commit.h b/commit.h
>>>> index 838f6a6b26..70371e111e 100644
>>>> --- a/commit.h
>>>> +++ b/commit.h
>>>> @@ -18,12 +18,16 @@ struct commit_list {
>>>>
>>>> struct commit {
>>>> struct object object;
>>>> - void *util;
>>>> unsigned int index;
>>>> timestamp_t date;
>>>> struct commit_list *parents;
>>>> struct tree *tree;
>>>> uint32_t graph_pos;
>>>> + /*
>>>> + * Do not add more fields here unless it's _very_ often
>>>> + * used. Use commit-slab to associate more data with a commit
>>>> + * instead.
>>>> + */
>>>> };
>>> That's a logical consequence and a natural endgame of this
>>> pleasent-to-read series. THanks.
>>>
>>> Unfortunately we are gaining more and more stuff in "struct commit"
>>> with recent topics, and we may want to see if we can evict some of
>>> them out to shrink it again.
>> Sigh.. ds/lazy-load-trees already enters 'next' so a fixup patch is
>> something like this.
> Sorry I take this patch back. I didn't realize it was a rename and the
> old field named 'tree' was already there (I vaguely recalled some
> "tree" in this struct but didn't stop to think about it). Moving
> graph_pos out is an option, but only 32-bit arch gains from it (64-bit
> arch will have 4 bytes padding anyway) so probably not worth the
> effort. "generation" field should probably be moved out though.I'm happy to take a look at this patch and figure out the best way to integrate it with the changes I've been doing.
I disagree with the removal of "generation". My intention is to make the commit-graph feature a standard feature that most repos (of reasonable size) want to have. In that case, 'graph_pos' and 'generation' will be set during every parse and used by every commit walk. This is just my gut reaction.
As I investigate these changes, I'll try to see what performance hit is caused by converting the graph_pos and/or generation to commit slabs. (Again, I'm assuming the slabs will make this slower. I may be wrong here.)
Thanks, -Stolee