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

From: Jonathan Tan <jonathantanmy@google.com>
Date: May 2, 2018, 20:50 UTC
Message-ID: <20180502135029.051dcaed75b18fb95ae6b28f@google.com>
In-Reply-To: <20180501213403.14643-14-sbeller@google.com>

On Tue, 1 May 2018 14:34:03 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 12 quoted lines

> +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().

Show 13 quoted lines

>  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?

Show 5 quoted lines

> +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?

Show 21 quoted lines

> -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).