patterncMinor
Does this simple memory pool use too much memory?
Viewed 0 times
thissimplemuchtoomemorydoespooluse
Problem
I just wrote this pool to avoid calling
I'm using some small functions that aren't really required because I find the abstraction nice when using the code later and because I noticed the compiler will optimize them away when link-time optimization is turned on.
I'm using about the same code I posted before for the stack.
```
#include "object_pool.h"
#define DEFAULT_CAPACITY 128
#define STACK_CAPACITY 128
#define OP_FAIL_SAFE
static char memory_block_start(Memory_Block block)
{
return (char *)(block + 1);
}
static Memory_Block new_memory_block(Object_Pool op)
{
Memory_Block temp = malloc(sizeof(Memory_Block) + op->object_size op->big_block_capacity);
if(temp == NULL)
return NULL;
//If the stack can't support the same number of objects created, freeing objects might fail
#ifdef OP_FAIL_SAFE
if(dsta
malloc and free when I have some code that frequently allocates and deallocates chunks of same-sized memory. I would like to know if there are any bugs I didn't notice and what would be the best solution to achieve this goal.I'm using some small functions that aren't really required because I find the abstraction nice when using the code later and because I noticed the compiler will optimize them away when link-time optimization is turned on.
I'm using about the same code I posted before for the stack.
object_pool.h#ifndef OBJECT_POOL_H
#define OBJECT_POOL_H
#include
#include "dynamic_stack.h"
#define OP_SUCCESS 1
#define OP_ERROR 0
typedef struct Memory_Block {
char *position;
char *end;
struct Memory_Block *next;
} Memory_Block;
typedef struct Object_Pool {
Dynamic_Stack free_blocks;
size_t object_size;
size_t big_block_capacity;
Memory_Block *last_memory_block;
Memory_Block *first_memory_block;
size_t total_blocks;
} Object_Pool;
int op_init_custom(Object_Pool *op, size_t object_size, size_t big_block_capacity);
int op_init(Object_Pool *op, size_t object_size);
void op_destroy(Object_Pool *op);
void op_set_big_block_capacity(Object_Pool *op, size_t new_capacity);
size_t op_total_blocks(Object_Pool *op);
void *op_get(Object_Pool *op);
void op_release(Object_Pool *op, void *object);
void op_release_all(Object_Pool *op);
#endifobject_pool.c```
#include "object_pool.h"
#define DEFAULT_CAPACITY 128
#define STACK_CAPACITY 128
#define OP_FAIL_SAFE
static char memory_block_start(Memory_Block block)
{
return (char *)(block + 1);
}
static Memory_Block new_memory_block(Object_Pool op)
{
Memory_Block temp = malloc(sizeof(Memory_Block) + op->object_size op->big_block_capacity);
if(temp == NULL)
return NULL;
//If the stack can't support the same number of objects created, freeing objects might fail
#ifdef OP_FAIL_SAFE
if(dsta
Solution
You should always use curly braces around
Something small like this:
Can be one lined:
Braces can be omitted on this one, because it is one lined and it's still very obvious that this is the entire if statement, and if someone comes along to add something to this they have to add braces anyway.
Returning in an
like this:
You especially want the fact that this is throwing an error to stick out, this is easier to read in my opinion.
The
UGH!
Don't write it like that, it looks ugly; this is much prettier I think:
if statements and for loops. It's the safe thing to do, especially when you are dealing with memory-management. Something small like this:
if(temp == NULL)
return NULL;Can be one lined:
if(temp == NULL) return NULL;Braces can be omitted on this one, because it is one lined and it's still very obvious that this is the entire if statement, and if someone comes along to add something to this they have to add braces anyway.
Returning in an
if statement should always be inside of curly braces:int dstack_decrease_capacity(Dynamic_Stack *stack, size_t remove_total)
{
if(remove_total >= (size_t)(stack->end - stack->start))
return DSTACK_ERROR;
return dstack_resize(stack, stack->end - stack->start - remove_total);
}like this:
int dstack_decrease_capacity(Dynamic_Stack *stack, size_t remove_total)
{
if(remove_total >= (size_t)(stack->end - stack->start))
{
return DSTACK_ERROR;
}
return dstack_resize(stack, stack->end - stack->start - remove_total);
}You especially want the fact that this is throwing an error to stick out, this is easier to read in my opinion.
The
for loop!UGH!
void op_release_all(Object_Pool *op)
{
for(Memory_Block *ite = op->first_memory_block; ite != NULL; ite = ite->next)
ite->position = memory_block_start(ite);
//The stack must be cleared too to avoid handling the same block twice
dstack_clear(&op->free_blocks);
}Don't write it like that, it looks ugly; this is much prettier I think:
void op_release_all(Object_Pool *op)
{
for(Memory_Block *ite = op->first_memory_block; ite != NULL; ite = ite->next)
{
ite->position = memory_block_start(ite);
}
//The stack must be cleared too to avoid handling the same block twice
dstack_clear(&op->free_blocks);
}Code Snippets
if(temp == NULL)
return NULL;if(temp == NULL) return NULL;int dstack_decrease_capacity(Dynamic_Stack *stack, size_t remove_total)
{
if(remove_total >= (size_t)(stack->end - stack->start))
return DSTACK_ERROR;
return dstack_resize(stack, stack->end - stack->start - remove_total);
}int dstack_decrease_capacity(Dynamic_Stack *stack, size_t remove_total)
{
if(remove_total >= (size_t)(stack->end - stack->start))
{
return DSTACK_ERROR;
}
return dstack_resize(stack, stack->end - stack->start - remove_total);
}void op_release_all(Object_Pool *op)
{
for(Memory_Block *ite = op->first_memory_block; ite != NULL; ite = ite->next)
ite->position = memory_block_start(ite);
//The stack must be cleared too to avoid handling the same block twice
dstack_clear(&op->free_blocks);
}Context
StackExchange Code Review Q#46005, answer score: 5
Revisions (0)
No revisions yet.