patterncppMinor
C++ custom memory allocator
Viewed 0 times
customallocatormemory
Problem
I'm working on a C++ custom memory allocator, that would be kind of a replacement for the C flexible array syntax, you know, the stuff like that:
I want to have this in C++ like this:
I'm not an expert C++ programmer. I had to learn a lot while I've been working on this problem. I probably made some mistakes. In particular I didn't think ahead about exception safety of my code. I'd like to get an advice on how to amend this problem. And any other suggestions on improving my code are welcome.
Please note that this is a low level allocator. Its interface is not compatible with libc++ requirements. I am going to add a libc++-compatible wrapper for this one as a separate class.
So far I have the following:
```
// Round down to a power of two multiple.
constexpr std::size_t Align(std::size_t n, std::size_t a) {
return n & ~(a - 1);
}
// Round up to a power of two multiple.
constexpr std::size_t AlignUp(std::size_t n, std::size_t a) {
return Align(n + a - 1, a);
}
namespace memory {
namespace detail {
// Calculate a data item alignment according to its size.
constexpr std::size_t Align(std::size_t size, std::size_t offset) {
return size class EntryLayout {
public:
using Type = T;
using Pointer = T *;
static constexpr std::size_t Size = sizeof(Type);
static constexpr std::size_t Offset = Align(Size, S);
static constexpr std::size_t EndOffset = Offset + Size;
static Pointer Instance(char *ptr) {
return reinterpret_cast(RawData(ptr));
}
template
static Pointer Construct(char *ptr, Args &&... args) {
return new (RawData(ptr)) Type(std::forward(args)...);
}
static void Destruct(char *p
struct C_Arena
{
struct Header header;
size_t size;
size_t used;
char* data[];
};
...
struct C_Arena *arena = malloc(1000);
arena->size = 1000 - sizeof(struct C_flexible_array_struct));
arena->used = 0;
struct Header *header = &arena->header;
...I want to have this in C++ like this:
auto arena = Chunk(new char[1000], 1000);
Header *header = arena.Get();
...I'm not an expert C++ programmer. I had to learn a lot while I've been working on this problem. I probably made some mistakes. In particular I didn't think ahead about exception safety of my code. I'd like to get an advice on how to amend this problem. And any other suggestions on improving my code are welcome.
Please note that this is a low level allocator. Its interface is not compatible with libc++ requirements. I am going to add a libc++-compatible wrapper for this one as a separate class.
So far I have the following:
```
// Round down to a power of two multiple.
constexpr std::size_t Align(std::size_t n, std::size_t a) {
return n & ~(a - 1);
}
// Round up to a power of two multiple.
constexpr std::size_t AlignUp(std::size_t n, std::size_t a) {
return Align(n + a - 1, a);
}
namespace memory {
namespace detail {
// Calculate a data item alignment according to its size.
constexpr std::size_t Align(std::size_t size, std::size_t offset) {
return size class EntryLayout {
public:
using Type = T;
using Pointer = T *;
static constexpr std::size_t Size = sizeof(Type);
static constexpr std::size_t Offset = Align(Size, S);
static constexpr std::size_t EndOffset = Offset + Size;
static Pointer Instance(char *ptr) {
return reinterpret_cast(RawData(ptr));
}
template
static Pointer Construct(char *ptr, Args &&... args) {
return new (RawData(ptr)) Type(std::forward(args)...);
}
static void Destruct(char *p
Solution
Some random observations:
Your code is a bit harder to follow than it needs to be, because of your Consistent Use Of Capital Letters Without A Specific Purpose.
Here we've got Capitals indicating a static member constant; a member typedef; an extern function; and a template non-type parameter. As a reader, I expect lowercase to be the default, and Capital Letters to indicate something specifically salient. In some codebases they mean "class names"; in others they mean "global constants"; but here they seem to mean nothing more than "I capitalize all my identifiers." This slows down the reader.
It seems like you're mixing two (maybe three) concerns here. First, you're writing a memory resource (a.k.a. allocator) that manages handing out chunks of memory from a buffer. Second, you've decided that the same buffer should also hold basically a
Third, your initial example (but not your complete final example) suggests that maybe you want the memory resource to take ownership of its buffer and free it when it's done with it?
If
I recommend splitting your code into one piece that does nothing but the handing-out-memory-blocks business, and a separate piece that reimplements
I notice that you provide both
You wrote
One more thing about your
I mistrust your
Your code is a bit harder to follow than it needs to be, because of your Consistent Use Of Capital Letters Without A Specific Purpose.
static constexpr std::size_t Size = sizeof(Type);
static constexpr std::size_t Offset = Align(Size, S);Here we've got Capitals indicating a static member constant; a member typedef; an extern function; and a template non-type parameter. As a reader, I expect lowercase to be the default, and Capital Letters to indicate something specifically salient. In some codebases they mean "class names"; in others they mean "global constants"; but here they seem to mean nothing more than "I capitalize all my identifiers." This slows down the reader.
It seems like you're mixing two (maybe three) concerns here. First, you're writing a memory resource (a.k.a. allocator) that manages handing out chunks of memory from a buffer. Second, you've decided that the same buffer should also hold basically a
std::tuple as its first block.Third, your initial example (but not your complete final example) suggests that maybe you want the memory resource to take ownership of its buffer and free it when it's done with it?
auto arena = Chunk(new char[1000], 1000);
Header *header = arena.Get();If
arena didn't take ownership of the buffer, this would be a memory leak, right? (But in your final example, you use a stack-local buffer, so it's not a problem.)I recommend splitting your code into one piece that does nothing but the handing-out-memory-blocks business, and a separate piece that reimplements
std::tuple. (Or, you know, just use std::tuple.) So then your Chunk would look something like this:template
class Chunk {
using Tuple = std::tuple;
MemoryResource mr_;
Tuple *tuple_;
public:
explicit Chunk(char *buffer, size_t size) : mr_(buffer, size) {
void *p = mr_.allocate(sizeof(Tuple));
tuple_ = ::new (p) Tuple;
}
void *allocate(size_t bytes) { return mr_.allocate(bytes); }
template auto& get() { return std::get(tuple_); }
};I notice that you provide both
T Get() and const T Get() const. First of all, why T and not T&? Second, have you considered whether the correct signature might be just a single overload T Get() const? Your Chunk structure could arguably be viewed as a handle referring to the buffer; fetching a mutable reference to the T object stored in the buffer can be done without modifying the Chunk structure itself. Food for thought, maybe.You wrote
template where I would write template — I use plural names for parameter packs, and recommend everyone else do so too. (And as for class over typename — it's just shorter.)One more thing about your
Get function: consider that you are allowed to have a std::tuple, but you are not allowed to have a Chunk with your current implementation. Functions that fetch a member out of a parameter-pack should almost invariably take an integer index, not a type, as their template parameter. Of course std::get for tuple and variant supports both syntaxes; you could, too.I mistrust your
Alloc function because it takes a size but fails to ask the caller what alignment they want. If I want space to store an int, I can call Alloc(4), but if the pointer I get back is 1-byte-aligned, that doesn't help me. As a general rule, allocation functions should always take both size and alignment parameters.Code Snippets
static constexpr std::size_t Size = sizeof(Type);
static constexpr std::size_t Offset = Align(Size, S);auto arena = Chunk<Header>(new char[1000], 1000);
Header *header = arena.Get<Header>();template<class... Ts>
class Chunk {
using Tuple = std::tuple<Ts...>;
MemoryResource mr_;
Tuple *tuple_;
public:
explicit Chunk(char *buffer, size_t size) : mr_(buffer, size) {
void *p = mr_.allocate(sizeof(Tuple));
tuple_ = ::new (p) Tuple;
}
void *allocate(size_t bytes) { return mr_.allocate(bytes); }
template<class T> auto& get() { return std::get<T>(tuple_); }
};Context
StackExchange Code Review Q#85341, answer score: 2
Revisions (0)
No revisions yet.