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

[Stefan Beller <sbeller@google.com>]

On Wed, May 2, 2018 at 1:50 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> On Tue,  1 May 2018 14:34:03 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> +void *allocate_alloc_state(void)
>> +{
>> +     return xcalloc(1, sizeof(struct alloc_state));
>> +}
>> +
>> +void clear_alloc_state(struct alloc_state *s)
>> +{
>> +     while (s->slab_nr > 0) {
>> +             s->slab_nr--;
>> +             free(s->slabs[s->slab_nr]);
>> +     }
>> +}
>
> These functions are asymmetrical. I understand why it is this way
> (because we use pointers, and we want to use FREE_AND_NULL), but would
> still prefer _INIT and _release().
>
>>  static inline void *alloc_node(struct alloc_state *s, size_t node_size)
>>  {
>>       void *ret;
>> @@ -45,54 +63,56 @@ static inline void *alloc_node(struct alloc_state *s, size_t node_size)
>>       ret = s->p;
>>       s->p = (char *)s->p + node_size;
>>       memset(ret, 0, node_size);
>> +
>> +     ALLOC_GROW(s->slabs, s->slab_nr + 1, s->slab_alloc);
>> +     s->slabs[s->slab_nr++] = ret;
>> +
>>       return ret;
>>  }
>
> This unconditionally grows the slabs for each node allocation. Shouldn't
> more than one node fit in each slab?

Yes. I'll go with Duy's idea and make it a linked list by using the first
object as a pointer to the next slab.

>
>> +extern struct alloc_state the_repository_blob_state;
>> +extern struct alloc_state the_repository_tree_state;
>> +extern struct alloc_state the_repository_commit_state;
>> +extern struct alloc_state the_repository_tag_state;
>> +extern struct alloc_state the_repository_object_state;
>
> (Context: these were in alloc.h)
>
> These seem to be used only in object.c, and only in one function - could
> we declare them "static" inside that function instead?

ok

>
>> -struct object_parser *object_parser_new(void)
>> +struct object_parser *object_parser_new(int is_the_repo)
>>  {
>>       struct object_parser *o = xmalloc(sizeof(*o));
>>       memset(o, 0, sizeof(*o));
>> +
>> +     if (is_the_repo) {
>> +             o->blob_state = &the_repository_blob_state;
>> +             o->tree_state = &the_repository_tree_state;
>> +             o->commit_state = &the_repository_commit_state;
>> +             o->tag_state = &the_repository_tag_state;
>> +             o->object_state = &the_repository_object_state;
>> +     } else {
>> +             o->blob_state = allocate_alloc_state();
>> +             o->tree_state = allocate_alloc_state();
>> +             o->commit_state = allocate_alloc_state();
>> +             o->tag_state = allocate_alloc_state();
>> +             o->object_state = allocate_alloc_state();
>> +     }
>>       return o;
>>  }
>
> I don't think saving 5 allocations is worth the code complexity (of the
> additional parameter).

Ok, I'll remove this overhead.

Thanks,
Stefan