9
\$\begingroup\$

This is my first time doing any kind of serious coding in C. I watched a Youtube video on what an Arena Allocator should do and I tried my best to make an implementation for it.

As of now, we have the arena itself and a way to handle errors in the arena. I prefer a way to just warn the programmer about errors and put on them to decide what to do with it (be it crash or something else).

Any criticism are greatly appreciated!

/* ArenaAlloc.h */

#ifndef ARENA_ALLOC_H
#define ARENA_ALLOC_H

#include <stdlib.h>
#include <stdint.h>
#include <assert.h>
#include <string.h>
#include <stdio.h>

#define KB(x) (x*1024)

typedef char* Arena_ptr;

typedef struct {
    size_t offset;
    Arena_ptr buffer;
    size_t size;
} Arena;

enum Arena_ErrorType
{
    ARENA_ERR_NONE,
    ARENA_ERR_MALLOC,
    ARENA_ERR_NOT_ENOUGH_SPACE
};

typedef struct {
    char errMsg[256];
    size_t requested;
    size_t available;
    uint8_t status;
} Arena_Error;

void Arena_init(Arena *arena, size_t size, Arena_Error *err);
// Allocates space in the arena (in bytes) and returns a pointer to the location
void *Arena_alloc(Arena *arena, size_t size, Arena_Error *err);
// Cleans the array and resets the offset back to the start
void Arena_clean(Arena *arena);
void Arena_delete(Arena *arena);

#endif /* ARENA_ALLOC_H */


/* ArenaAlloc.c */
#include "ArenaAlloc.h"

void Arena_init(Arena *arena, size_t size, Arena_Error *err)
{
    err->status = ARENA_ERR_NONE;

    *arena = (Arena) {
        .offset = 0,
        .buffer = malloc(size),
        .size = size
    };

    if (!arena->buffer)
    {
        err->status = ARENA_ERR_MALLOC;
        err->requested = size;
        err->available = 0;
        strcpy(err->errMsg, "Could not allocate memory for the arena");
        return;
    }
    
    Arena_clean(arena);
}

void *Arena_alloc(Arena *arena, size_t size, Arena_Error *err)
{
    if (arena->offset + size > arena->size)
    {
        err->status = ARENA_ERR_NOT_ENOUGH_SPACE;
        err->requested = size;
        err->available = arena->size - arena->offset;
        sprintf(err->errMsg, "Not enough space. Requested: %zu\nAvailable: %zu", size, arena->size - arena->offset);
        return NULL;
    }

    Arena_ptr ptr = arena->buffer;

    arena->offset = size;
    arena->buffer += arena->offset;

    return ptr;
}

void Arena_clean(Arena *arena)
{
    arena->buffer -= arena->offset;
    for (size_t i = 0; i < arena->size; i++, arena->buffer++)
        *arena->buffer = 0;

    arena->buffer -= arena->size;
    arena->offset = 0;
}

void Arena_delete(Arena *arena)
{
    Arena_clean(arena);
    free(arena->buffer);
}

here's a main.c that i've been using to debug it:

#include "ArenaAlloc.h"

int main(int argc, char const *argv[])
{
    Arena myArena = {};
    Arena_Error err = {};
    Arena_init(&myArena, KB(10), &err);
    if (err.status)
        printf("%s\n", err.errMsg);

    int *ptr = Arena_alloc(&myArena, sizeof(int) * 6, &err);
    if (!ptr)
        printf("%s\n", err.errMsg);

    size_t i;
    for (i = 0; i < 6; i++, ptr++)
    {
        *ptr = KB(i);
        printf("ptr: %p\tvalue: %d\n", ptr, *ptr);
    }
    ptr -= i;
    
    for (i = 0; i < 6; i++, ptr++)
    {
        printf("myArena.buffer: %p\tvalue: %d\n", ptr, *ptr);
    }

    Arena_delete(&myArena);

    return 0;
}
\$\endgroup\$
3
  • \$\begingroup\$ Doesn't look like an allocator to me. Where is free? \$\endgroup\$ Commented Nov 11 at 4:57
  • 3
    \$\begingroup\$ @vnp, that is the nature of an arena allocator. When the pattern of requests is "alloc, alloc, alloc, free everything", we don't need to worry about e.g. a fancy buddy allocator that attempts to merge holes in the middle, since everything will soon be nuked anyway. The ancient Apache httpd used this approach to build each result page. To answer your free() question, it is named Arena_delete(). One could optionally use #define's to let malloc() / free() style source code transparently use this (rather limited) allocator. \$\endgroup\$ Commented Nov 11 at 15:21
  • \$\begingroup\$ @J_H That said, it would be possible to remember the address of the last allocation, or even of the last few, so that we can at least free the last data structure we allocated. In the common case where we allocate and then free a large array before allocating anything else, that could let us reclaim the memory with minimal overhead. The implementation still allocates from a single free block at the end of the arena. \$\endgroup\$ Commented Nov 12 at 0:02

2 Answers 2

7
\$\begingroup\$

char pointer

typedef char* Arena_ptr;

That seems weird.

I mean, it's your Public API, you get to define it however you want, signed char or unsigned or whatever. But if we're shooting for malloc() compatibility, I kind of thought that void * is what we'd be looking for. Then we match POSIX behavior, and the return value is compatible with uchar *, int *, Foo *, whatever the caller is using. The OP code will often require caller to cast the result.

cache line behavior

typedef struct {
    char errMsg[256];
    size_t requested;
    size_t available;
    uint8_t status;
} Arena_Error;

A cache line typically is 64 bytes. It happens that that evenly divides 256. But I would rather see errMsg banished to the end of the struct.

If we're lucky, the message will be "small", and will fit within one or two cache lines, along with the sizes and status.

If a maintainer changed the message length to e.g. 300 or 400 or another length, I would worry about whether both sizes would appear within the same cache line.

alignment

Caller was "encouraged" to offer KiB alignment with the KB macro. But Arena_init() does not mask nor otherwise enforce that.

    *arena = ...
        .buffer = malloc(size),

If you want to bench competively, it might be worth your while to signal error for odd size values, or to coerce them to a more fortuitous alignment.

bzero()

In Arena_clean(), I don't understand this:

    for (size_t i = 0; i < arena->size; i++, arena->buffer++)
        *arena->buffer = 0;

I mean, sure, we're clearing to zero. But why do we believe that this source code will compile to something faster than memset(buffer, 0, size)? It has been optimized to within an inch of its life. Just call it! Do not trust that your compiler at optimization level -On will recognize the pattern and make that call for you.

Via induction we essentially do
arena->buffer += arena->size; and then
arena->buffer -= arena->size;, a no-op, which seems like placing more trust in your compiler than is necessary.

DRY

    sprintf(err->errMsg, "Not enough space. Requested: %zu\nAvailable: %zu",
        size, arena->size - arena->offset);

Prefer ... , size, err->available);, as you've already computed that.

automated tests

Thank you for including a main(); I appreciate it.

The if (err.status) is clearly not enough, since we should bail out with fatal error if that triggers.

Consider writing some unit or integrations tests that are part of CI/CD, so they can "break the build" if unfortunate allocation behavior is introduced next month by some hapless maintenance engineer.


don't get most of this thing with struct allignment; I was taught that you should sort the members from bigger to smaller.

Right, that is a "padding" argument. Suppose we have three 64-bit doubles and two chars. Here is an unfortunate memory layout:

typedef struct {
    double d1;
    char   c2;
    double d3;
    char   c4;
    double d5;
} Foo;

Why? Because c2 consumes 1 byte, followed by 7 bytes of padding, in order to get d3 to align properly. We consume 40 bytes.

Better to put the big ones up front. For one thing it will fill the cache line nicely, and if later the struct grows and spills beyond 64 bytes into another cache line, we still get sensible behavior. This is an efficient 26 bytes, with no padding.

    double d1;
    double d3;
    double d5;
    char   c2;
    char   c4;

In contrast, char errMsg[256] is exactly four cache lines on common architectures. By shoving it to the end of the struct, I was hoping that a single cache line would typically satisfy the library's workload. The first 17 bytes are for a pair of pointers plus a status, and then any error message of 46 characters or less will fit within that same cache line that we've already retrieved. If the terminating NUL character appears within 64 bytes, we win. Else we may suffer cache miss and retrieve the next cache line, no biggie.

Now, honestly, optimizing the performance of repeated ARENA_ERR_MALLOC or ARENA_ERR_NOT_ENOUGH_SPACE failures in a tight loop is not exactly how benchmarks are won or lost. I was just describing my thought process when examining how a struct will be laid out in memory.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the answer! Onto the specifics: char pointer: I genuinenly dont know why choosing void * would be preferable. I chose char * because it always jumps 1 when doing pointer arithmetics cache line behavior: i still dont get a most of this thing with struct allignment, i was taught that you should put sort the members from bigger to smaller. automated tests: Im still to look after what even is CI/CD and unit tests, specially on C. But i think its a great time for such a small project bzero(), DRY, alignment: that i can do, thanks! Actually, i just forgot about memset. \$\endgroup\$ Commented Nov 11 at 13:03
5
\$\begingroup\$

Poor naming

Based on its name and type and type alias, I expect Arena.buffer to be a pointer to the starting address of the arena from which memory is being allocated, but that is not how it is used. Instead, it is used to track the beginning of the unallocated space, which would be better named something like next_available or start_unallocated.

Don't modify the pointer to mallocated memory!

As a matter of good practice, however, you should maintain every pointer returned by malloc(), unchanged, until you free it. Modifying your only copy with the expectation that you can recompute the original value is error-prone and subject to code-path risk. Don't do it. Instead, maintain Arena.buffer unchanged after you initialize an Arena. Maintain Arena.offset as the offset of the unallocated region from the base, as you already seem to intend to do, and compute the next allocation at need as arena->buffer + arena->offset. This will also moot the previous point.

Buggy allocation

Arena_alloc() does not maintain the arena offset correctly. This ...

    arena->offset = size;

... should instead be:

    arena->offset += size;

As it currently stands, the offset tracks not the total space allocated from the arena but only the last space allocated, making it easily possible to overrun the arena's total space. And an overrun will definitely occur in Arena_clean() if more than one allocation of size >0 has been made from the arena prior to cleaning it. Such issues can be detected by exercising the allocator even a little more strenuously and monitoring with Valgrind.

Unreliable / buggy test

In Arena_alloc(), the test for allocation overflow ...

    if (arena->offset + size > arena->size)

... can fail as a result of arithmetic overflow. For example, if anything at all has been allocated out of the arena, then that test will not catch a request to allocate SIZE_MAX bytes, as arena->offset + size will be less than arena->offset. Such a request probably wouldn't be made intentionally, but it might happen as a result of miscalculating the needed size of an allocation as a small negative number instead of small positive one. Recommended correction:

    if (size > arena->size || arena->size - size < arena->offset)

Unnecessary work

Arena_init() has no business calling Arena_clean(). In general, allocation clients can't rely on getting all-zero memory, so it's wasteful for Arena_init() to clear the allocated arena. Even if you want to provide an initially all-zero arena anyway, just use calloc() instead of malloc() to get that as efficiently as possible, straight from the system allocator.

Inefficient implementation

Speaking of Arena_clean(), your other answer also points out that memset() is a better choice than an assignment loop for clearing the region. What's more, if you start off with zeroed memory then unless clients have misbehaved, you need only clear the first arena->offset bytes. The unallocated tail of the arena will still be all-zero.

More unnecessary work

The only reason I see for Arena_delete() to call Arena_clean() is to restore arena->buffer to its original value, but it shouldn't need to do that in the first place (see above). Even while it does need to do that, there is no good reason to zero the contents of the arena before deallocating it. Clients have no reason to expect that, and if it's something they want then it is within their power to do it themselves.

Positioning of #includes

Headers and source files should directly #include headers providing declarations of all identifiers used directly within, to improve their modularity. Preferably, they will not include any unneeded headers. Here, that calls for these headers:

main.c:

  • stdio.h (for printf)
  • ArenaAlloc.h

ArenaAlloc.c:

  • stdlib.h (for malloc, free, size_t)
  • stdio.h (for sprintf)
  • string.h (for strcpy)
  • ArenaAlloc.h

ArenaAlloc.h:

  • stdint.h (for uint8_t)
  • stdlib.h (for size_t); or size_t could be obtained from stddef.h instead, or from any of several other headers.

Positioning of macro definitions

Do not expose macro definitions more widely than you need to do. In particular, do not define macros in shared header files that you intend to be used only in a single source file. KB should be defined in main.c instead of in ArenaAlloc.h.

Wasted space

Does Arena_Error really need to contain an error message string? The other fields contain all the information needed to generate one upon demand, after the fact. I recommend removing that altogether, and instead providing a utility function that writes error messages based on Arena_Error objects at need.

Misleading data type

Why is the type of Arena_Error.status defined as uint8_t, when the values you intend to store there are apparently of type enum Arena_ErrorType? Be consistent. As a bonus, doing so would remove the only need to include stdint.h:

typedef struct {
    size_t requested;
    size_t available;
    enum Arena_ErrrorType status;
} Arena_Error;

C23-specific feature

Empty initializers ...

    Arena myArena = {};
    Arena_Error err = {};

... do not conform to versions of ISO C prior to C23, which was officially published only about a year ago (in 2024). Some compilers supported the form as an extension before that, but you're fairly likely to find compilers still in use in the wild that reject it. You would gain wider compatibility for little, if any, cost by using the longtime traditional form instead:

    Arena myArena = {0};
    Arena_Error err = {0};

.

Use compound statements

C is not Python. It is not sensitive to indentation. As a matter of C style, always use a brace-enclosed compound statement as the body of loop constructs and if and else clauses, even when there is only one statement within. Doing otherwise is error prone, as at least one major security bug has proven within recent memory.

argc and argv

As a matter of style, if you're not going to use at least one of argc and argv then exercise your option to declare main() without parameters: int main(void). That reduces noise a bit and saves people reading your code from looking for what the program does with its command line arguments.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.