Subject: Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable

From: Ramsay Jones <[email-masked]>
Date: Fri, 11 Jul 2014 10:41:32 +0100

On 11/07/14 09:32, Jeff King wrote:
> On Fri, Jul 11, 2014 at 01:59:53AM +0100, Ramsay Jones wrote:
> 
>>> The code you're touching here was trying to make sure that each commit
>>> gets a unique index, under the assumption that commits only get
>>> allocated via alloc_commit_node. But I think that assumption is wrong.
>>> We can also get commit objects by allocating an OBJ_NONE (e.g., via
>>> lookup_unknown_object) and then converting it into an OBJ_COMMIT when we
>>> find out what it is.
>>
>> Hmm, I don't know how the object is converted, but the object allocator
>> is actually allocating an 'union any_object', so it's allocating more
>> space than for a struct object anyway.
> 
> Right, we would generally want to avoid lookup_unknown_object where we
> can for that reason.
> 
>> If you add an 'index' field to struct object, (and remove it from
>> struct commit) it could be set in alloc_object_node(). ie _all_ node
>> types get an index field.
> 
> That was something I considered when we did the original commit-slab
> work, as it would let you do similar tricks for any set of objects, not
> just commits. The reasons against it are:
> 
>   1. It would bloat the size of blob and tree structs by at least 4
>      bytes (probably 8 for alignment). In most repos, commits make up
>      only 10-20% of the total objects (so for linux.git, we're talking
>      about 25MB extra in the working set).
> 
>   2. It makes single types sparse in the index space. In cases where you
>      do just want to keep data on commits (and that is the main use),
>      you end up allocating a slab entry per object, rather than per
>      commit. That wastes memory (much worse than 25MB if your slab items
>      are large), and reduces cache locality.
> 

Ahem, yeah I keep telling myself not to post at 2am ... ;-)

Although I haven't given this too much extra thought, I'm beginning
to think that your best course would be to revert commit 969eba63
and add an convert_object_to_commit() function to commit.c which
would set c->index. Then track down each place an OBJ_NONE gets
converted to an OBJ_COMMIT and insert a call to
convert_object_to_commit(). (which may do more than just set the
index field ...)

Hmm, I've just noticed a new series in my in-box. I guess I will
discover what you decided to do shortly ... ;-P

ATB,
Ramsay Jones