From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 14 May 2018 18:14:38 GMT
Subject: Re: [PATCH v2 14/14] commit.h: delete 'util' field in struct commit
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:
> 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