Overall

The thing that I wonder most about the overall design is why you have opted not to use standard library interfaces wherever possible. It seems to me that an object pool is probably essentially a std::pmr::memory_resource (there is std::pmr::synchronized_pool_resource, for example), which you could then use easily in containers with a std::pmr::polymorphic_allocator... so I could have a whole vector of pooled objects automatically:

auto pool = razaron::objectpool::ObjectPool;

// ... and assuming ObjectPool is derived from std::pmr::memory_resource
auto pooled_foos = std::pmr::vector<Foo>std::pmr::polymorphic_allocator<Foo>&pool;

// Can now freely create Foo objects in the vector that are in the
// pool, and they could even be automatically erased.

Without the std::pmr::memory_resource interface, I have to manually create all the Foo objects I want in the vector, then have the vector either be a vector<Handle> (which would be clunky to use) or vector<std::reference_wrapper<Foo>> and manually get() each object to store it in the vector. And in either case, the onus is on me to do the cleanup.

(You could even take a cue from std::pmr::synchronized_pool_resource and fall back to an upstream allocator, rather than just failing for large allocations.)

And there's more than that, of course. For example, it seems to me that Handle is almost a smart pointer, so it could simply be std::unique_ptr or std::shared_ptr with a custom deleter. And AlignedArray<T, S, A> is just std::aligned_storage_t<S, A>.

But as neither Handle nor AlignedArray are part of the review, I'll focus just on the object pool itself.

Questions and other issues

... some things are too low level so they are in C-style C++ (e.g. memset and memcpy).

That is misguided thinking. C++ is every bit as low level as C, and in fact sometimes more so, while still providing all the type safety and high-level features you could want.

In particular, any half-decent standard library will define their algorithms to fall back to fast primitives whenever possible. In plain English: std::copy will automatically become std::memcpy/std::memmove if possible, and std::fill will automatically become std::memset. If you don't believe me, just grep your standard library headers to see for yourself.

I'm not saying "replace all your std::memset with std::fill!!!", because that's not really necessary (and it would require bringing in the whole algorithms library). std::memset and std::memcpy are perfectly legitimate tools for low-level stuff like this. I'm just giving you a heads up to not underestimate your friendly neighbourhood standard library implementer. Just write good, clear C++ code, and trust in your tooling (but always double check!).

Style: I have a code style that I try to be consistent with. Am I doing anything fundamentally bad?

Meh, looks perfectly legible to me.

Preprocessor: I'm banal about type safety so avoid using it in all my other code. Is it better to switch the macros over over to constexpres for added type safety?

Yes. Absolutely. In fact, I'd say your use of the preprocessor is a code smell. I wouldn't let it pass review.

Am I making proper use of modern C++?

In my opinion, you're making good use of the language facilities... not so much the library though, as I mentioned above. It's not just not using standard library stuff in the code itself, it's that the overall interface doesn't jibe with standard library practices.

Is the code easy to understand?

Yes, I'd say so.

Am I handling thread safety properly?

There are some dodgy things in there. The top level suggestion I would have is to have a more rigorously consistent practice when it comes to locking and unlocking. For example, you should only, only, ONLY lock in certain functions - say, the top-level user interface functions, for example - and then call reusable implementation functions that assume the locking is already done. You should not need a std::recursive_mutex... that's a red flag to me that code is going to be cavalier about locking.

I'll mention other, more specific concerns deeper in the review.

Would you use this over alternatives? I made it just for personal use but designed it to be stranger friendly. Did I accomplish that?

The code is clean, and it's not a bad concept, and I do see a use for object pools. The showstopper for me is the lack of integration with the rest of the language.

I mean, the point of a pool is to be able to store a bunch of objects together - for better locality - and to wipe them all out together (perhaps by "winking" them out, to borrow John Lakos's term). But if I want to get a bunch of objects in your pool... it's really clunky. I need to write a loop (or use an algo) where I construct each object individually, and then either store the handles or the addresses somewhere (they're not available to me from the pool itself), and then when I'm done, I actually have manually erase them all (in the general case - if I know they're trivially destructable, I don't need to... but then I wouldn't need to do that for any pool, including the standard library ones).

By contrast, if the library used standard library interfaces like std::pmr::memory_resource and smart pointers (particulary standard smart pointers with custom deleters)... pretty much everything I described above becomes automatic. I'd just create my container - any standard container, or any container with a standard allocator interface - and populate it... and that's it. Everything else is managed and cleaned up automatically.

Anywho, on with the review....

Preamble

#pragma once

This is non-portable, and for a very good reason (that's too technical to get into here). Standard include guards work just fine.

You already know about the problems with the #defines.

Typedefs

using ArrayA = AlignedArray<char, OBJECT_POOL_PAGE_LENGTH * OBJECT_SIZE_2, OBJECT_POOL_PAGE_ALIGNMENT>;

There is a very subtle bug in your code caused by an equally subtle flaw in your logic.

You assume that the alignment of T... is less than or equal to the size of T (rounded up to a power of 2). That's not always true.

If I have a type Foo that has very specific alignment requirements, I would run into trouble trying to put it in your object pool. Let's say Foo is defined as:

// aligned on 64 byte cache line size (for example)
struct [[some_special_alignment_intrinsic(64)]] Foo

 // but only 4 bytes in size
 std::array<std::byte, 4> data;
;

If I try to store that in your object pool, it will be stored at 4 byte alignment, rather than 64. That could be catastrophic.

To fix this, you'd probably have to go through all the code and change just about every sizeof(T) to std::max(sizeof(T), alignof(T)).

using PoolA = std::tuple<Handle *, std::list<std::unique_ptr<ArrayA>>, std::shared_ptr<std::recursive_mutex>>;

These pool types bug me for a couple reasons.

The first thing that bugs me is that you're using a tuple. Tuples are lazy hacks for grouping data in a throwaway context - like returning multiple values from a function (and only then when the order and meaning of the values is obvious). They are not for building key types in your library.

These pools should be defined as classes, perhaps templated on the alignment. The key signal to you that that's what you should have done is your init() function. It's only purpose is to construct the pools... but if they were their own type, they would construct themselves, and you wouldn't need the init() function.

The second thing that bugs me is that I can't for the life of me think of a situation where you'd need a std::list<std::unique_ptr<T>>. Any time I see a container other vector used - not counting maps and sets - I get suspicious. Vector should be your default container, unless you have particularly special needs, and you've measured to be sure a vector won't cut it. In practice, just about the only thing you gain from using std::list over std::vector is that iterators are not invalidated on insertion or removal - in other words, that the address of something in a list stays constant even as you add/remove items, whereas in a vector, adding/removing stuff can shift the other elements around. But that benefit becomes meaningless when the elements being stored are pointers themselves. std::list<std::unique_ptr<T>> offers no real benefits over std::vector<std::unique_ptr<T>>... and the vector can be much faster and simpler to use (because it's random-access).

I looked through the code, and didn't see any reason why a vector wouldn't work here, though, admittedly, there's a lot of code, so I might have missed something.

The third thing that bugs me std::shared_ptr. std::shared_ptr is a powerful tool, and sometimes you do need it... but it's also too often a tool used by people who don't really know what they're doing. "Shared" doesn't mean shared access, it means shared ownership. Using a shared pointer is essentially admitting: "I don't really know who is responsible for this object". But it seems to me you do know who should be responsible for the mutex: the pool itself.

I looked to see if there was anywhere where the ownership of the mutex was shared, and saw nothing - but again, there's a lot of code, so maybe I missed something. From what I could tell, the only thing you ever do with the mutex is lock/unlock it; it's never passed around.

And that brings me to the fourth and final thing: the fact that you're using a recursive mutex rather than a regular mutex. As with shared pointers, there's almost never any reason to use a recursive mutex. Doing so suggests you're being careless about your locking strategies, and that's a fast ride to deadlocksville. You were probably thinking that recursive mutexes are a safe way to avoid deadlocks, but the opposite is true. The best - and arguably the only - way to avoid deadlocks is to only lock mutexes together (using std::lock or std::scoped_lock), not one at a time. If you try to lock one, then the other... you're cruisin for a bruisin'. And when you're dealing with a recursive mutex, it may be already locked, so you're always in the (possible) position of locking mutexes one after the other.

There doesn't seem to be any particular reason why you need recursive mutexes here anyway.

Also, I presume the only reason you're using a smart pointer here at all is because the mutex isn't movable or copyable, and that won't work with your tuple. That problem would go away if you used a custom type.

So I would recommend:

1. Using an actual type for your pools, not a tuple.


2. Using a std::vector<std::unique_ptr<ArrayX>> rather than a list.


3. Using a std::mutex, rather than a recursive mutex.


4. Using the mutex directly as a member - not by pointer.

Using an actual type for the pool will allow you to offload a lot of the work of individual pool management to the pool class, making the wrapper class simpler.

using PoolTuple = std::tuple<PoolA, PoolB, PoolC, PoolD, PoolE, PoolF>;

This seems like something that would be better defined in the ObjectPool class, along with Page, PoolCond1, and HandleMap. None of them are in the public interface (well, ignoring init()).

ObjectPool

template <std::size_t... Is>
void ObjectPool::init(PoolTuple &p)

 ((std::get<2>(std::get<Is>(p)) = std::make_shared<std::recursive_mutex>()), ...);


inline ObjectPool::ObjectPool() noexcept
 : _hashMap, _hashMapMutex

 init<0, 1, 2, 3, 4, 5>(_pools);

All this goes away if you use a proper type for your pools, rather than a tuple. It would reduce to:

inline ObjectPool::ObjectPool() = default;

The unordered map prevents it from being either constexpr or noexcept. (The custom pool type may also, depending on how you write it, but it seems to me the pool constructors could be both constexpr and noexcept, except that the vector prevents it being constexpr.)

In any case, I'd say the way you're doing init() is clunky and brittle. This looks like a job for std::apply() and a lambda.

template <class T>
inline Handle ObjectPool::push(const T &object)

 // ...


template <class T>
inline Handle ObjectPool::push(T &&object)

 // Find the pool that fits T
 using Pool = typename PoolCond1<T>::type;

 if (sizeof(T) <= OBJECT_SIZE_64)
 
 return allocateMove<T, Pool>(std::forward<T>(object));
 
 else
 
 std::stringstream message;
 message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): "
 << ".";

 throw std::length_error(message.str());
 

Why have the T const& overload? T&& is a forwarding reference. It will work for everything: lvalues, rvalues, everything.

You may want to use decay_t<T> internally here, rather than T directly.

You do the check at runtime for whether a type cannot be handled by the pool... but why? This is something that can be done entirely at compile time, in multiple ways. First, you should probably refigure PoolCond1 to trigger a compile-time error if the size is too big. That's no biggie; probably nothing more than this:

template <typename T>
struct PoolCond1

 constexpr auto alignment = std::max(sizeof(T), alignof(T));
 static_assert(alignment > (your max align here));
 using type = std::conditional<alignment <= OBJECT_SIZE_2, PoolA,
 // and so on with the conditional as is
;

With that you don't need the if:

template <class T>
Handle ObjectPool::push(T&& object)

 // Will trigger a compile-time error if T is too big.
 using Pool = typename PoolCond1<T>::type;

 return allocateMove<T, Pool>(std::forward<T>(object));

But if you're paranoid you can add an if constexpr that checks the size and throws if it's too big. Or a static assert.

If you do opt to do that, I would strongly recommend making custom exception types, rather than simply throwing length_error. This is clunky:

if constexpr (std::max(sizeof(T), alignof(T)) > OBJECT_SIZE_64)

 std::stringstream message;
 message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): "
 << ".";

 throw std::length_error(message.str());

If you created a custom exception type like this:

template <typename T>
class alignment_overflow : public std::length_error

public:
 alignment_overflow() :
 std::length_errormake_message_()
 

private:
 static auto make_message_()
 
 auto message = std::ostringstream;
 message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): .";

 return message.str();
 
;

That could boil down to:

if constexpr (std::max(sizeof(T), alignof(T)) > OBJECT_SIZE_64)
 throw alignment_overflow<T>;

Even better, you could make a base class for all pool exceptions:

class pool_error

public:
 constexpr explicit pool_error(std::size_t alignment) :
 alignment_alignment

 virtual ~pool_error() = default;

 virtual auto alignment() const noexcept -> std::size_t return alignment_; 
 // maybe other functions with useful info, like the typeid or type name

private:
 std::size_t alignment_;
;

And use that to define your exception types:

template <typename T>
class alignment_overflow :
 public virtual pool_error,
 public virtual std::length_error

public:
 alignment_overflow() :
 pool_errorstd::max(sizeof(T), alignof(T)),
 std::length_errormake_message_()
 

 // ...
;

And now you can do something like this:

try

 // pool operations

// all these catches match, so first come first serve
catch (alignment_overflow const& x) 
catch (pool_error const& x) 
catch (std::length_error const& x) 
catch (std::exception const& x)

 // if you want the extra info in a general catch...
 if (auto p = dynamic_cast<pool_error const*>(&x); p)
 
 std::cerr << p->alignment();
 

However you choose to define your custom exceptions, I suggest using them, rather than all the hassle of constructing those what() messages in the middle of code, which clutters things up.

template <class T, class... Args>
inline Handle ObjectPool::emplace(Args... args)

 // Find the pool that fits T
 using Pool = typename PoolCond1<T>::type;

 if (sizeof(T) <= OBJECT_SIZE_64)
 
 return allocateConstruct<T, Pool>(args...);
 
 else
 
 std::stringstream message;
 message << typeid(T).name() << " is too large for ObjectPool. sizeof(" << typeid(T).name() << "): " << sizeof(T) << ".";

 throw std::length_error(message.str());
 

You're doing perfect forwarding wrong.

First, the arguments have to be forwarding references. So Args&&....

Second, you have to actually forward them. So std::forward<Args>(args)..., not just args....

Once again, these errors can be caught at compile time, so this becomes:

template <class T, class... Args>
Handle ObjectPool::emplace(Args&&... args)

 using Pool = typename PoolCond1<T>::type; // does compile-time check

 return allocateConstruct<T, Pool>(std::forward<Args>(args)...);

And the same concepts apply to this:

template <class T>
inline T *ObjectPool::get(const Handle &handle)

 if (handle.size != sizeof(T))
 
 std::stringstream message;
 message << "Type mismatch. HandleSize: " << handle.size << " != sizeof(T): " << sizeof(T) << ". typeid(T): " << typeid(T).name();

 throw std::invalid_argument(message.str());
 
 else if (sizeof(T) <= OBJECT_SIZE_64)
 
 return getObject<T>(handle);
 
 else
 
 std::stringstream message;
 message << "HandleSize (" << handle.size << ") too large for ObjectPool.";

 throw std::length_error(message.str());
 

which becomes:

template <class T>
T* ObjectPool::get(Handle const& handle) const

 constexpr auto alignment = std::max(sizeof(T), alignof(T));

 static_assert(alignment <= OBJECT_SIZE_64);

 if (handle.size != alignment)
 throw type_mismatch<T>handle;

 return getObject<T>(handle);

and so on for the other functions. As you can see, these functions get so small that it actually becomes feasible to simply define them right in the class. In any case, you don't need the inline specifier because they're templates.

Also get() seems like something that could/should be const.

Skip on down to reorder()... I'm not sure why you use a pointer here:

auto pool = &std::get<Pool>(_pools);

It only adds the hassle of having to use *, and you never reseat it, so why not use a reference?

Now here's where you start getting into trouble with the mutexes and locking. You lock the pool mutex in reorder()... then you call getIndex(), getObject(), and allocateMove(), the latter two of which also lock the pool mutex... and then allocateMove() also locks the hash table mutex while holding the pool mutex.

The danger is in allocateMove(). A deadlock happens, basically, when one function locks mutexes A then B while another locks mutexes B then A. So any time you want to lock two mutexes, you should be paranoid. You may think that all it takes is programming discipline to make sure you always lock A then B in order... but that's naïve because the compiler is free to reorder instructions.

Any time you lock more than one mutex, you should always use std::lock() or std::scoped_lock and lock them all together. As of C++17, you can probably use std::scoped_lock pretty much universally - forget all about std::lock_guard.

The first issue you need to avoid is recursive locking. That's just a matter of programming discipline. For example, you could work with the rule to only lock in public methods, and never call one public method from another. Or you could name all functions that require locks <something something>_locked_() (like getIndex_locked_()), and insist that once a function acquires a lock it can only call *_locked_() functions, and *_locked_() functions can only call other *_locked_() functions. Since you're dealing with multiple locks (unwise!), you may have to use more complex function names to keep track of what's locked and what's not.

A lot of this complexity could be alleviated if you used proper types for your pools, rather than tuples.

So let's get practical. Here's the sitch with regards to reorder(): You know reorder() needs to mess with both the pool and the handle hashtable. You have two options:

1. take all the locks at once


2. lock things only when needed, making sure to never try to lock something while something else is already locked

Both are valid strategies.

Option 1 is simpler.

template <class T>
inline Handle ObjectPool::reorder(const Handle &handle)

 using Pool = typename PoolCond1<T>::type;

 if (handle.size != sizeof(T))
 throw type_mismatch<T>handle;

 auto& pool = std::get<Pool>(_pools);

 // All locking is done here.
 auto lock = std::scoped_lockstd::get<2>(pool), _hashMapMutex;

 // Do everything with the pool and hashtable here. None of the
 // private functions called are allowed to do any locking.

Option 2 is more efficient... but that only helps if it doesn't introduce errors. Is it safe to first allocate the object then release the pool lock, the lock and update the hash map separately? Probably not, because you're checking for a space and then filling it - if you give up the lock, someone else may fill it instead.

But it's not that big a deal here, because you bail early if you're not going to be doing any reordering. It's a bummer to lock the hashtable unnecessarily in that case, but it least it passes quickly.

Before I show what that might look like, there's one more thing about reorder() that I think needs improvement. After you get the lock, you check the index, and return immediately if there's no need to reorder. That's good. But then you immediately create a T object. Why? You don't need it yet. In fact, you may not need it at all! You won't need it until getObject(). On top of that, you call getObject() twice, unnecessarily.

So the whole bottom half of reorder() needs to be restructured to not create any objects unless necessary.

Put altogether:

template <class T>
Handle ObjectPool::reorder(Handle const& handle)

 using Pool = typename PoolCond1<T>::type;

 if (handle.size != sizeof(T))
 throw type_mismatch<T>handle;

 auto& pool = std::get<Pool>(_pools);

 auto lock = std::scoped_lockstd::get<2>(pool), _hashMapMutex;

 // Notice I've flipped the condition
 if (getIndex<Pool>(std::get<0>(pool)) > handle.index)
 
 if (auto p = getObject<T>(handle); p)
 
 auto temp = std::move(*p);
 erase<T, Pool>(handle);
 return allocateMove<T, Pool>(std::move(temp));
 
 

 return ;

Next is capacity().... This could probably be greatly simplified and generalized with std::apply() and/or fold expressions.

Same for shrink().

On to the private methods....

template <class T, class Pool, class... Args>
inline Handle ObjectPool::allocateConstruct(Args... args)

template <class T, class Pool>
inline Handle ObjectPool::allocateMove(T &&object)

For allocateConstruct() see above for the comments on emplace() about perfect forwarding.

But the bigger problem here is that you've defined allocateConstruct() in terms of allocateMove(). That just seems bass-ackwards. This way you're not really doing a real emplace, and you can't support non-movable/non-copyable types. The right thing to do would be to do the emplace properly... and then allocateMove() becomes superfluous.

The other problem is that you do the move with std::memcpy... which is a terrible idea. It will only work for trivially copyable types, and you don't limit the pool to those types. If you tried to put a std::string in the pool, what would happen is that the memcpy would copy the guts of object into the new location... then object would be destroyed when it goes out of scope, which would free the string's memory... so what's left in the pool is now a zombie string pointing to deallocated memory.

What you want to be doing here is placement new.

With all that, allocateConstruct() would now look something like this:

template <class T, class Pool, class... Args>
Handle ObjectPool::allocateConstruct(Args&&... args)

 auto& pool = std::get<Pool>(_pools);

 // No locking! That should be done at a higher level.

 // This code would be a lot more clear if one could write
 // pool.list
 // rather than
 // std::get<1>(pool)
 // and so on.
 auto totalPositions = std::get<1>(pool).size() * OBJECT_POOL_PAGE_LENGTH;
 if (totalPositions == 0 

And allocateMove() can simply forward to allocateConstruct() or be removed completely.

I think that's about enough for the review, because from this point on it's pretty much repeating the same points. The one additional thing to note is that in erase():

if (std::is_destructible<T>::value && !std::is_trivially_destructible<T>::value)
 reinterpret_cast<T *>(ptrToRemove)->~T();

std::memset(ptrToRemove, 0, std::get<0>(*pool)->size);

can pretty much reduce to:

std::destroy_at(reinterpret_cast<T*>(ptrToRemove));

This will boil down to a no-op for trivially destructible types, but if you're really paranoid you can make it:

// No need to check std::is_destructible<T>, at least not *here*.
// T *HAS* to be destructible... because you're destroying it!
if constexpr (!std::is_trivially_destructible_v<T>)
 std::destroy_at(reinterpret_cast<T*>(ptrToRemove));

The memset seems unnecessary.

pragma once

#pragma once is supported on most C++17 compilers...

Include guards are supported on all C++17 compilers, even on the strictest conformance settings.

... I don't intend to have multiple copies of the same file.

That's like saying "I don't need to worry about smart pointers, because I don't intend to leak memory".

Whatever you may intend, it is not hard to get into the situation where you have multiple copies of a header. You make a shared library of useful utility functions, then you make an app that makes use of that library... you want to use your object pool in the app, but uh oh, the library was using it too, and now you have two copies of the header - one in the library code base, one in the app code base.

Even if you are the most perfect programmer who ever lived, and would never create a situation where the same header is in two different places, it could still very easily happen. Some other programmer might have found the header useful and included in their code. And even if you're the only programmer who will ever use the file, and you're perfect, you could still find yourself with multiple copies of the file, because of something your build system does, or maybe your revision control system, package manager, or snap or whatever else.

So whatever your intentions, it's a possibility. And if it happens, #pragma once is not guaranteed to work. I see no benefit in using a non-standard extension that doesn't work instead of a standard construct that does. Just use include guards; it's the standard, portable solution.

Alignment problems

Your test case appeared to work because you didn't actually test the problem. Bar is supposed to be 64-bit aligned. You never actually tested whether it was. And it isn't. See here: http://cpp.sh/8w5ti (Note, the function push() there should really be called emplace().)

Normally a type's size is >= its alignment. But it is possible to create types whose alignment is >= their size. If you try to put those objects in one of your pools, it will "work", but you're in undefined behaviour territory. Who knows what will happen next: an access violation or corrupted memory are both likely possibilities.

Using sizeof(T) will work for most objects. But to be really safe, you should probably use max(sizeof(T), alignof(T)). I have edited the notes above to take into account every edge case I can think of.

Forward ref problems

I am just guessing, but what I suspect you're doing is calling Foo f; pool.push<Foo>(f); or something like that. When you make the call like that, you are preventing template argument deduction and losing the benefit of reference collapsing. Just do Foo f; pool.push(f);. That will work in every case.

See for yourself: http://cpp.sh/8pdxy

As you can see, it works for every type of value I throw at it - lvalues, lvalue refs, rvalue refs, even const rvalue refs (which are weird). And I deliberately made it a member template, just like your push() functions. (I probably should have named it push() instead of bar(). Oh well.)

Being able to call pool.push(t); rather than pool.push<T>(t); is the whole point of having a push() function. If you're only ever going to call push<T>(t);, then you might as well call emplace<T>(t); and delete the push() functions completely.

(You still need to call emplace() like pool.emplace<Foo>(arg1, arg2, etc);, because emplace() can't deduce T from the arguments like push() can.)