This is programmed in Visual Studio 2019 with the C++17 feature set.
The intent with this code is to be able to create memory blocks of the specified alignment and size, so as to have contiguous cache-friendly memory and to only do one malloc and free for the whole block. Is there something wrong or missing from this implementation so that it would effectively fulfill its purpose?
MemoryBlock.h
#pragma once
class MemoryBlock
{
private:
void* p_rawMem;
void* p_currentMemLocation;
size_t p_currentMemLocationOffset;
size_t p_totalMemSize;
size_t p_remainingMemSize;
size_t p_alignment;
void *p_lastMemByteLocation;
void p_addCurrentMemLocation(size_t delta);
public:
MemoryBlock(size_t size, size_t alignment);
void* getMemoryWith(size_t size, size_t alignemnt);
~MemoryBlock();
};
MemoryBlock.cpp
#include <memory>
#include <stdexcept>
#include <cmath>
#include <assert.h>
#include "MemoryBlock.h"
void MemoryBlock::p_addCurrentMemLocation(size_t delta)
{
if (delta == 0) {
return;
}
p_currentMemLocationOffset += delta;
assert((p_currentMemLocationOffset) <= p_totalMemSize);
p_remainingMemSize -= delta;
if (p_remainingMemSize == 0) {
p_currentMemLocation = p_lastMemByteLocation;
}
else {
p_currentMemLocation = static_cast<void*>(static_cast<char*>(p_currentMemLocation) + delta);
}
}
MemoryBlock::MemoryBlock(size_t size, size_t alignment):
p_rawMem(_aligned_malloc(size, alignment)),
p_currentMemLocation(p_rawMem),
p_currentMemLocationOffset(0),
p_totalMemSize(size),
p_remainingMemSize(size),
p_alignment(alignment),
p_lastMemByteLocation(static_cast<void*>(static_cast<char*>(p_rawMem) + (size - 1)))
{
}
void* MemoryBlock::getMemoryWith(size_t size, size_t alignment)
{
if (size > p_totalMemSize) {
throw std::bad_alloc();
}
if (size == 0) {
throw std::invalid_argument("size must be greater than 0");
}
if (alignment == 0 || (alignment &(alignment-1))!= 0) {//checks it alignment is power of 2
throw std::invalid_argument("alignment should be a power of 2.");
}
if (alignment > p_alignment) {
throw std::invalid_argument("alignment requirement is greater than the memory block alignment.");
}
if (size > p_remainingMemSize) {
return nullptr;
}
if (p_totalMemSize == p_remainingMemSize) {
p_addCurrentMemLocation(size);
return p_rawMem;
}
p_addCurrentMemLocation((std::ceil(p_currentMemLocationOffset/(double)alignment)*alignment)-p_currentMemLocationOffset);
void* retVal = p_currentMemLocation;
p_addCurrentMemLocation(size);
return retVal;
}
MemoryBlock::~MemoryBlock()
{
_aligned_free(p_rawMem);
}
main.cpp
int main()
{
MemoryBlock memBlock(1024,1024);
int *memBlockIntPtr = new(memBlock.getMemoryWith(sizeof(int),alignof(int))) int(1);
std::cout << *memBlockIntPtr << std::endl;
constexpr size_t intArrSize = 4;
int *memBlockIntArrPtr = static_cast<int*>(memBlock.getMemoryWith(sizeof(int)*intArrSize, alignof(int)));
for (size_t i = 0; i < intArrSize; i++)
{
new (&memBlockIntArrPtr[i]) int(i);
}
for (size_t i = 0; i < intArrSize; i++)
{
std::cout << memBlockIntArrPtr[i] << std::endl;
}
return 0;
}
I also ask you if the usage of the code is correct in the main function, and if I'm handling the void pointers adequately in both single-variable and array cases. If not, of course, please correct me with an explanation. Say if I'm doing main, specifically, correctly or the whole thing- and if not, make it clear why. Also suggest if I should be using an underlying memory buffer of char* instead of void* (perhaps combined with static arrays).
-
\$\begingroup\$ What do you mean by "I'm doing main"? \$\endgroup\$Peter Mortensen– Peter Mortensen2020年07月20日 08:01:31 +00:00Commented Jul 20, 2020 at 8:01
-
\$\begingroup\$ "error: use of undeclared identifier 'p_lastMemByteLocation'" I cannot compile your code. I think you forgot to add the member. Can you check that? \$\endgroup\$henje– henje2020年07月20日 14:37:38 +00:00Commented Jul 20, 2020 at 14:37
-
\$\begingroup\$ @henje done. Fixed. \$\endgroup\$Matias Chara– Matias Chara2020年07月20日 14:40:35 +00:00Commented Jul 20, 2020 at 14:40
1 Answer 1
Use C++17 aligned new
or aligned_malloc()
_aligned_malloc()
is a Windows-specific, non-portable function. Luckily, C++17 added the portable aligned_malloc()
without an underscore. It also introduces a variant of new
that allows aligned allocation. The latter would look like so:
#include <new>
...
MemoryBlock::MemoryBlock(size_t size, size_t alignment):
p_rawMem((void *)new(std::align_val_t(alignemnt)) char[size]),
...
{
}
Unnecessary special case for p_remainingMemSize == 0
Why is this a special case in p_addCurrentMemLocation
? Just adding delta
to the current pointer is perfectly fine. A pointer that points just beyond the end of an array or an allocated memory region is still a valid pointer.
Consider avoiding the check for delta == 0
An if
-statement is not free, especially not if the branch predictor cannot correctly predict the result of the condition. If delta
is zero, then the rest of the code still works correctly, and it's just two additions and a subtraction.
Don't use floating point math
You should not need to use floating point math to calculate how much to advance p_currentMemLocationOffset
inside getMemoryWith()
. Depending on which processor your code is running on, converting to double
and back to size_t
can be an expensive operation. Furthermore, double
has less precision than size_t
on 64-bits machines, so the result might be wrong if very large memory blocks are used.
Instead of ceil(p_CurrentMemLocationOffset / (double)alignment)
, use the fact that integer division rounds down, and compensate for this by adding alignment - 1
first:
p_addCurrentMemLocation((((p_currentMemLocationOffset + alignment - 1) / alignment) * alignment) - p_currentMemLocationOffset);
Alternatively, use the modulo operation to determine how much to add:
p_addCurrentMemLocation((alignment - p_currentMemLocationOffset % alignment) % alignment);
The latter will be especially fast if the compiler can deduce that alignment
is always a power of two.
char *
vs. void *
It's probably easier to use char *
internally for the pointers, since you are doing arithmetic on them. And if you do that, the only time you need to cast is when returning from getMemoryWith()
.
-
1\$\begingroup\$ Very useful analysis!!! I must point out however a few things: visual studio does not provide an implementation for c++17`s aligned_alloc(), crazy right? and also, I use assert() because if everything goes right in the public function then that private function should never have that assertion fail, if it does it is I who has done a mistake not the public interface caller. \$\endgroup\$Matias Chara– Matias Chara2020年07月19日 22:31:27 +00:00Commented Jul 19, 2020 at 22:31
-
\$\begingroup\$ also, are you sure the syntax you show for the aligned overload of the c++17 new operator is correct? gives me a compile error.(again I dont know if this is just visual studio deviating from the standard again or an error on your part, please confirm.) \$\endgroup\$Matias Chara– Matias Chara2020年07月19日 22:47:26 +00:00Commented Jul 19, 2020 at 22:47
-
2\$\begingroup\$ Ok, that's a good reason for having both
assert()
andthrow()
. Keep on doing that then :) I did make a mistake in the example of alignednew
. Also, for the alignednew
operator, you might have to#include <new>
. But it might be that MSVC does not support alignednew
anyway. \$\endgroup\$G. Sliepen– G. Sliepen2020年07月20日 07:48:53 +00:00Commented Jul 20, 2020 at 7:48