HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Does this simple memory pool use too much memory?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thissimplemuchtoomemorydoespooluse

Problem

I just wrote this pool to avoid calling 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);

#endif


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