Re: [PATCH v2 13/13] alloc: allow arbitrary repositories for alloc functions

[Stefan Beller <sbeller@google.com>]

On Tue, May 8, 2018 at 8:00 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, May 8, 2018 at 12:59 AM, Stefan Beller <sbeller@google.com> wrote:
>> @@ -501,9 +509,31 @@ void raw_object_store_clear(struct raw_object_store *o)
>>  void parsed_object_pool_clear(struct parsed_object_pool *o)
>>  {
>>         /*
>> -        * TOOD free objects in o->obj_hash.
>> -        *
>>          * As objects are allocated in slabs (see alloc.c), we do
>>          * not need to free each object, but each slab instead.
>> +        *
>> +        * Before doing so, we need to free any additional memory
>> +        * the objects may hold.
>>          */
>> +       unsigned i;
>> +
>> +       for (i = 0; i < o->obj_hash_size; i++) {
>> +               struct object *obj = o->obj_hash[i];
>> +
>> +               if (!obj)
>> +                       continue;
>> +
>> +               if (obj->type == OBJ_TREE) {
>> +                       free(((struct tree*)obj)->buffer);
>
> It would be nicer to keep this in separate functions, e.g.
> release_tree_node() and release_commit_node() to go with
> alloc_xxx_node().

ok, I can introduce that, although it seems unnecessary complicated
for now.

On top of this series I started an experiment (which rewrites alloc
and object.c a whole lot more; for performance reasons), which gets
rid of the multiple alloc_states. There will be only one allocation for
one repository, it can allocate across multiple types without alignment
overhead. It will reduce memory footprint of obj_hash by half, via
storing indexes instead of pointers in there.
That said, the experiment shall not influence the
direction of this series. Will fix.

>> +               } else if (obj->type == OBJ_COMMIT) {
>> +                       free_commit_list(((struct commit*)obj)->parents);
>> +                       free(&((struct commit*)obj)->util);
>> +               }
>> +       }
>
> I still don't see who frees obj_hash[] (or at least clears it if not
> freed). If I'm going to use this to free memory in pack-objects then
> I'd really prefer obj_hash[] freed because it's a big _big_ array.

gah!

> Just to be clear, what I mean is
>
> FREE_AND_NULL(o->obj_hash);
> o->obj_hash_size = 0;

ok, I just put it here, just before the calls
to clear_alloc_state()s.