Jameson Miller <jam...@microsoft.com> writes:
> Introduce the mem_pool type which encapsulates all the information
> necessary to manage a pool of memory.This change moves the existing
> variables in fast-import used to support the global memory pool to use
> this structure.
>
> These changes allow for the multiple instances of a memory pool to
> exist and be reused outside of fast-import. In a future commit the
> mem_pool type will be moved to its own file.
>
> Signed-off-by: Jameson Miller <jam...@microsoft.com>
> ---
> fast-import.c | 80
> +++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 51 insertions(+), 29 deletions(-)
Thanks, this got much easier to read and reason about.
> @@ -304,9 +317,7 @@ static int global_argc;
> static const char **global_argv;
>
> /* Memory pools */
> -static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mp_block);
> -static size_t total_allocd;
> -static struct mp_block *mp_block_head;
> +static struct mem_pool fi_mem_pool = {0, 2*1024*1024 - sizeof(struct
> mp_block), 0 };
>
> /* Atom management */
> static unsigned int atom_table_sz = 4451;
> @@ -324,6 +335,7 @@ static off_t pack_size;
> /* Table of objects we've written. */
> static unsigned int object_entry_alloc = 5000;
> static struct object_entry_pool *blocks;
> +static size_t total_allocd;
So the design decision made at this step is that pool_alloc field
keeps track of the per-pool allocation, while total_allocd is a sum
across instances of pools. That sounds appropriate for stats.
> @@ -634,7 +646,21 @@ static unsigned int hc_str(const char *s, size_t len)
> return r;
> }
>
> -static void *pool_alloc(size_t len)
> +static struct mp_block *mem_pool_alloc_block(struct mem_pool *mem_pool,
> size_t block_alloc)
> +{
> + struct mp_block *p;
> +
> + mem_pool->pool_alloc += sizeof(struct mp_block) + block_alloc;
> + p = xmalloc(st_add(sizeof(struct mp_block), block_alloc));
> + p->next_block = mem_pool->mp_block;
> + p->next_free = (char *)p->space;
> + p->end = p->next_free + block_alloc;
> + mem_pool->mp_block = p;
This, compared to what used to happen in mem_pool_alloc()'s original
code, ignores total_allocd. I cannot tell if that is an intentional
change or a mistake.
> +
> + return p;
> +}
> +
> +static void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)
> {
> struct mp_block *p;
> void *r;
> @@ -643,21 +669,17 @@ static void *pool_alloc(size_t len)
> if (len & (sizeof(uintmax_t) - 1))
> len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
>
> - for (p = mp_block_head; p; p = p->next_block)
> - if ((p->end - p->next_free >= len))
> - break;
> + for (p = mem_pool->mp_block; p; p = p->next_block)
> + if (p->end - p->next_free >= len)
> + break;
>
> if (!p) {
> - if (len >= (mem_pool_alloc/2)) {
> - total_allocd += len;
> + if (len >= (mem_pool->block_alloc / 2)) {
> + mem_pool->pool_alloc += len;
> return xmalloc(len);
It is unfair to account this piece of memory to the mem_pool, as we
ended up not carving it out from here. Did you mean to increment
total_allocd by len instead, perhaps?
And I do agree with the idea in the previous round to make these
oversized pieces of memory allocated here to be freeable via the
mem_pool instance (I only found it questionable to use the main
"here are the list of blocks that we could carve small pieces out"
list), and anticipate that a later step in the series would change
this part to do so. With that anticipation, I very much appreciate
that this step did not mix that and stayed as close to the original
(except for the possible mis-accounting). It makes it very clear
what is going on in each separate step in the series.
> }
> - total_allocd += sizeof(struct mp_block) + mem_pool_alloc;
This is what I noticed got lost in the pool-alloc-block helper above.
> - p = xmalloc(st_add(sizeof(struct mp_block), mem_pool_alloc));
> - p->next_block = mp_block_head;
> - p->next_free = (char *) p->space;
> - p->end = p->next_free + mem_pool_alloc;
> - mp_block_head = p;
> +
> + p = mem_pool_alloc_block(mem_pool, mem_pool->block_alloc);
> }
>
> r = p->next_free;
> @@ -665,10 +687,10 @@ static void *pool_alloc(size_t len)
> return r;
> }
>
> -static void *pool_calloc(size_t count, size_t size)
> +static void *mem_pool_calloc(struct mem_pool *mem_pool, size_t count, size_t
> size)
> {
> - size_t len = count * size;
> - void *r = pool_alloc(len);
> + size_t len = st_mult(count, size);
Nice ;-)
> + void *r = mem_pool_alloc(mem_pool, len);
> memset(r, 0, len);
> return r;
> }
