Re: [PATCH v2] alloc.h|c: migrate alloc_states to mem-pool

["René Scharfe" <l.s.r@web.de>]

Am 01.02.21 um 11:39 schrieb 阿德烈 via GitGitGadget:
> From: ZheNing Hu <adlternative@gmail.com>
>
> "alloc_state" may have similar effects with "mem_pool".
> Using the new memory pool API may be more beneficial
> to our memory management in the future.

Replacing the custom object allocator with mem-pool would allow reducing
the code size.  What other effects might it have?  Do you expect changes
in memory use and/or performance with the current code and your patch?

> functions "alloc_*_node" now change to "mem_pool_alloc_*_node".

Why rename these functions?  Do callers need to care about the
underlying allocator?  The function signatures stay the same.  In any
case, this renaming would be easier to review if it was moved to a
separate patch.

> At the same time ,I add the member `alloc_count` of
> struct mem_pool ,so that we can effective track
> node alloc count,and adapt to the original interface `alloc_report`.

This function has no callers.  Why not remove it (in a separate patch)?

> diff --git a/alloc.c b/alloc.c
> index 957a0af3626..951ef3e4ed7 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -71,30 +71,30 @@ static inline void *alloc_node(struct alloc_state *s, size_t node_size)
>  	return ret;
>  }

This keeps the now unused function alloc_node(), which breaks the build
with -Werror.

allocate_alloc_state() and clear_alloc_state() become unused as well,
but the compiler doesn't complain because those functions are
exported.  Nevertheless this patch should remove them, no?

> diff --git a/object.h b/object.h
> index 59daadce214..43031d8dc04 100644
> --- a/object.h
> +++ b/object.h
> @@ -10,11 +10,11 @@ struct parsed_object_pool {
>  	int nr_objs, obj_hash_size;
>
>  	/* TODO: migrate alloc_states to mem-pool? */

This comment becomes stale with this patch and should be removed at
the same time.

> -	struct alloc_state *blob_state;
> -	struct alloc_state *tree_state;
> -	struct alloc_state *commit_state;
> -	struct alloc_state *tag_state;
> -	struct alloc_state *object_state;
> +	struct mem_pool *blob_pool;
> +	struct mem_pool *tree_pool;
> +	struct mem_pool *commit_pool;
> +	struct mem_pool *tag_pool;
> +	struct mem_pool *object_pool;

Why have pointers here instead of the structs themselves?  It's not like
a struct parsed_object_pool is of much use without them, right?

The same question applies to the original code as well, of course.

René