On Wed, May 2, 2018 at 1:50 PM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> On Tue, 1 May 2018 14:34:03 -0700
> Stefan Beller <sbeller@xxxxxxxxxx> 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
