Re: [PATCH v3 2/3] fast-import: introduce mem_pool type
From: Junio C Hamano <hidden>
Date: 2018-03-27 16:10:07
Jameson Miller [off-list ref] 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 <redacted> --- fast-import.c | 80 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 29 deletions(-)
Thanks, this got much easier to read and reason about.
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
+ + 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.
quoted hunk
- 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; }