Add allocator for node based containers #22702

Tested that IsMultiple() can be removed (Debian Clang 13)

0test/node_allocator_tests.cpp:245:16: warning: unused function 'isMultiple' [-Wunused-function]
1constexpr bool isMultiple(size_t a, size_t b)
2               ^
31 warning generated.

Perhaps rule of 5 (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-five, https://www.stroustrup.com/C++11FAQ.html#default)

0     /**
1-     * Copying a memory resource is not allowed, it is an immobile object.
2+     * Copying/moving a memory resource is not allowed; it is an immobile object.
3      */
4     MemoryResource(const MemoryResource&) = delete;
5     MemoryResource& operator=(const MemoryResource&) = delete;
6+    MemoryResource(MemoryResource&&) = delete;
7+    MemoryResource& operator=(MemoryResource&&) = delete;

As far as I can tell, this can remain 1 << 10 (1024).

In both cases, invoking the test with src/test/test_bitcoin -t validation_flush_tests -l test_suite prints CoinsTip usage percentage: 0.997634

0            chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10), // 1024

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

m_untouched_memory_end is always m_allocated_blocks.back() + alloc_size. Seems like it would be safer and less confusing to drop this unnecessary variable. Safer to avoid possibility of bugs where object gets into an inconsistent state. Less confusing to need one less variable to understand this.

If you prefer to keep this variable, I think for clarity it should have a comment like

// m_untouched_memory_end member variable is redundant, and is always equal to (m_allocated_pools.back().get() + POOL_SIZE_BYTES) whenever it is accessed, but m_untouched_memory_end caches this for clarity and efficiency.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

Why are individual allocations sometimes called “chunks” and sometimes called “nodes”? Can we drop “chunk” terminology and just refer to individual allocations consistently as nodes?

Personally I would love to drop the custom chunk/node/block terms entirely, just call the allocator a fixed size allocator, call the allocations allocations, and call the blocks pools. Just IMO, though.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

Strictly speaking, keeping ChunkNode structs aligned shouldn’t be required, because we could memcpy them to and from the empty spaces instead of static_casting them. But I guess not taking the max alignment is unlikely to save any space, if we already need to use the max size anyway?

In any case, it would be good to have sentence here to say why this is using ChunkNode alignment, since it seems like ChunkNode alignment shouldn’t need to matter in principle.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

It seems like it would be more ideal for chunk_size_bytes to be a compile time constant (template parameter) instead of a runtime constant (class member), since in all cases can be known at compile time, and the fact that it isn’t mean that other things like size of a block can’t be constant either. Not being able to easily compute size of the block (it requires runtime multiplication and division) I think indirectly led to m_untouched_memory_end being added unnecessarily. Not having a compile-time block size also appears to be a reason m_allocated_blocks is using void* and raw new/deletes instead of safer types like std::unique_ptr and std::array

Making this const would also have more minor benefit of improving clarity by being able to declare useful constants up front and not have things like / m_chunk_size_bytes * m_chunk_size_bytes various places in the code.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

This could go away using std::unique_ptr

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

Exposing this method seems pretty dubious. Maybe do friend class MemoryResourceTester; instead and move it to test code. My concerns here are verbosity of this header, and temptation for someone to call this inappropriately in some performance counter or log statement. If this should be exposed publicly, maybe it should have a name like CountFreeChunks so it doesn’t look like an innocuous accessor method.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

Suggestion not for here, but I wouldn’t mind some other PR adding simple CEIL_DIV, ROUND_UP util macros and using them places like this.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

This should be removed since it’s true by definition that sizeof returns char sized units and sizeof(char) is 1. Size of a char can vary (it is CHAR_BIT bits which is usually 8), but I don’t think any code here depends on a particular char size.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

I think this should be named MaxBlockSizeBytes instead of BlockSizeBytes since actual block size may be slightly less depending on alignment. Also convention is to use snake case for variable/constant names and camel case for type/function names, so maybe would be good MAX_BLOCK_SIZE_BYTES to also make it more obvious this is a compile-time constant.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

This doesn’t doesn’t really have anything to do with MemoryResource, and I’d think it should be part of the Allocator paragraph not the MemoryResource one. I’d reshuffle this design section a little bit and put MemoryResource paragraph first, Allocator paragraph second, and information about determining the node sizes third.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

Curious, when does rebinding happen? Maybe say something about when/if this would happen in a sentence here.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

These construct methods seem sketchy and prone to misuse. Can we drop these? The one place these are called is in ReallocateCache and it should be possible to eliminate them there by keeping the existing placement new:

0::new (&cacheCoinsMemoryResource) MemoryResource{CCoinsMapFactory::CreateMemoryResource()};
1::new (&cacheCoins) CCoinsMap{CCoinsMapFactory::CreateContainer(&cacheCoinsMemoryResource)};

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

I think it makes sense to add ReallocateCache cache (though I would be curious if someone knows a reason it wasn’t added here initially).

But if we are going to add this here I think we should remove the other ReallocateCache() call in validation.h which is now redundant, and make the ReallocateCache method private so it is not uselessly confusing.

ReallocateCache is now necessary here, because Flush is triggered when memory is full, and with the node_allocator clear() does not free any memory the memory usage would stay constant. It was not necessary before because clear() already deallocated all of the nodes and only kept the memory for the unordered_map’s internal indexing array.

Right! CoinsTip().ReallocateCache(); in validation.cpp is now unnecessary. Good catch.

In commit “Add allocator for node based containers” (952c37a31c39b7acdb5ab2634a354a1c548708a2)

Just so this information is easily accessible, can you add a comment about how this is tested for accuracy or where the tests can be found?

To confirm, replacing unordered_node<X> with std::pair<X, void*> is just a simplification for consistent style, not a change in behavior?

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Filling this fake transaction just to throw it all away and call tx.GetHash seems unnecessarily complicated. Could just construct the outpoint with any uint256 instead of a real transaction hash

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Would be good to replace std::pair<const Key, Value> with ContainerType::value_type to make this more obvious.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Could be shortened to NodeSize<ContainerType>

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Would s/destroyed by the Allocator/freed/ to be clear this happens independently of the allocator, and whether or not it is used.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Should drop “which is given in the constructor”, since it’s not true anymore and is not a really relevant detail describing the high level behavior here.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

It is unexpected to mention std::unordered_map here when everything above is talking about containers generally. Also unclear what “all cases” is referring to. Would drop last two sentences and just say something like:

Using node_allocator with a standard container types like std::list, std::unordered_map requires knowing node sizes of those containers which are non-standard implementation details. The \ref memusage::NodeSize trait and \ref node_allocator::Factory class can be used to help with this.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

It seems like it would be better to make this a static_assert than an if constexpr. I also don’t see a reason for this method to take an n argument. Switching to static_assert and dropping n argument here would have the following advantages:

• It would simplify this method and remove footguns where calling it incorrectly could result in bad runtime behavior instead of a compile time errors.
• Moving the size and n checks to Allocator::allocate instead of MemoryResource::Allocate would allow more flexiblility like falling back to a nested allocator like std::allocator instead of global ::operator new (In theory this would even allow chaining different sized node_allocator::Allocator types together so an allocator could transparently direct diffferent sized allocations to different sized pools, and fall back to std::allocator if no pool fits)

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Would expect FreeList* instead of void*. void* could make sense if these were ever pointers to something besides FreeLists, but if next is ever non-null it is always pointing to a FreeList.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Reading through the code again I now think that I actually have undefined behavior: I’m basically reinterpret_casting T* to FreeList*, and both types are completely unrelated, so according to the type aliasing rules accessing this is undefined behavior:

Performing a class member access that designates a non-static data member or a non-static member function on a glvalue that does not actually designate an object of the appropriate type - such as one obtained through a reinterpret_cast - results in undefined behavior

See https://en.cppreference.com/w/cpp/language/reinterpret_cast

This can be fixed by using memcpy, and then I can also relax the alignment.

Again seems like type should be FreeList* not void*

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Having this special case to handle shared memory resources seems clunky, especially because it adds a circular dependency between memusage and node_allocator types, instead of letting node_allocator have a one-way dependency on memusage.

Also it seems like awkward accounting to have dynamic usage of containers not actually include the memory they are using, and for all the cost of all memory be attributed to the memory resource with no distinction between allocated and unallocated memory.

Another way of doing the accounting would be to keep the containers attributed memory usage unchanged, and just attribute size of unallocated memory to MemoryResource, instead of size of allocated+unallocated memory.

Specifically the suggestion would be to:

• Drop the if constexpr special case code here and circular dependency of memusage on node_allocator
• Change definition of MemoryResource::DynamicMemoryUsage to only include unallocated space not total space by:
  • Subtracting POOL_SIZE_BYTES * m_allocated_pools.size() from current value
  • Adding m_untouched_memory_end - m_untouched_memory_iterator + (length of m_free_allocations list) * ALLOCATION_SIZE_BYTES
  • Maybe adding a counter variable to get length of m_free_allocations list efficiently

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

I’ve played with this for a while now, but it doesn’t work that way because in memusage.h I still need to know if the node_allocator is used for a container, because the node sizes are different. For the standard std::unordered_map I need to use memusage::MallocUsage for the nodes, and for node_allocator I have use the node size directly. So I need at least a forward declaration of node_allocator::Allocator. But that is fine I guess, I’ll create a forward declaration then there’s no circular dependency any more.

Also I’ll split up NodeSize calculation into a separate independent file that both memusage and the allocator can use, so there are no circular dependency for that too.

I don’t see a reason to tie this to unordered_map. Would suggest renaming this from UnorderedMapFactory to just Factory, taking a typename Container template argument and calling NodeSize<Container>::Value() below

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

This CreateContainer method seems like a pretty superfluous wrapper around the ContainerType constructor. Maybe we don’t need a factory class at all and can just provide some template typedefs for ccoins to use:

0template<typename BaseContainer>
1using MemoryResourceType = MemoryResource<NodeSize<BaseContainer>::Value()>;
2
3template<typename BaseContainer>
4using AllocatorType = Allocator<BaseContainer::value_type, NodeSize<BaseContainer>::Value()>;

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Putting this comment next to this typedef is a little confusing, because the comment is describing simulating pairs which simulate nonstandard internal data structures, but this pair isn’t really simulating anything, since it’s actually part of standard container definition.

Would suggest either moving comment, or only using std::pair for simulated pairs, or doing both things. By only using std::pair for simulated pairs, I mean something like:

0using ContainerType = std::unordered_map<Key, Value, Hash, Equals, Alloc>;
1using ValueType = typename ContainerType::value_type;

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Can you use #elif above to get rid of this nesting and just make a flat list of cases?

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

In practice, this is probably fine, but logically and in theory, it seems like there is a bug here because this is only using node size to determine allocation size, ignoring node alignment and FreeList size and alignment. It seems like this should be using the RequiredAllocationSizeBytes function to take these things into account, like:

 0diff --git a/src/support/allocators/node_allocator.h b/src/support/allocators/node_allocator.h
 1index 7ff94d39700..5b667ca99b8 100644
 2--- a/src/support/allocators/node_allocator.h
 3+++ b/src/support/allocators/node_allocator.h
 4@@ -385,7 +385,8 @@ template <typename Key, typename Value, typename Hash = std::hash<Key>, typename
 5 class UnorderedMapFactory
 6 {
 7 public:
 8-    static constexpr size_t ALLOCATION_SIZE_BYTES = memusage::NodeSize<std::unordered_map<Key, Value, Hash, Equals>>::Value();
 9+    static constexpr size_t ALLOCATION_SIZE_BYTES = detail::RequiredAllocationSizeBytes<typename
10+                                                    memusage::NodeSize<std::unordered_map<Key, Value, Hash, Equals>>::NodeType>();
11     using MemoryResourceType = MemoryResource<ALLOCATION_SIZE_BYTES>;
12     using AllocatorType = Allocator<std::pair<const Key, Value>, ALLOCATION_SIZE_BYTES>;
13     using ContainerType = std::unordered_map<Key, Value, Hash, Equals, AllocatorType>;
14diff --git a/src/memusage.h b/src/memusage.h
15index fd781f77bf5..af1f33963d6 100644
16--- a/src/memusage.h
17+++ b/src/memusage.h
18@@ -163,35 +163,38 @@ struct NodeSize {
19 // with multiple configurations (alignments, noexcept hash, node sizes).
20 template <typename Key, typename V, typename Hash, typename Equals, typename Allocator>
21 struct NodeSize<std::unordered_map<Key, V, Hash, Equals, Allocator>> {
22-    [[nodiscard]] static constexpr size_t Value()
23-    {
24         // libstdc++, libc++, and MSVC implement the nodes differently. To get the correct size
25         // with the correct alignment, we can simulate that with accordingly nested std::pairs.
26         using ValueType = std::pair<const Key, V>;
27+        using NodeType =
28 
29 #if defined(_MSC_VER)
30         // list node contains 2 pointers and no hash; see
31         // https://github.com/microsoft/STL/blob/main/stl/inc/unordered_map and
32         // https://github.com/microsoft/STL/blob/main/stl/inc/list
33-        return sizeof(std::pair<std::pair<void*, void*>, ValueType>);
34+                      std::pair<std::pair<void*, void*>, ValueType>
35 #else
36 
37 #if defined(_LIBCPP_VERSION) // defined in any C++ header from libc++
38         // libc++ always stores hash and pointer in the node
39         // see https://github.com/llvm/llvm-project/blob/release/13.x/libcxx/include/__hash_table#L92
40-        return sizeof(std::pair<ValueType, std::pair<size_t, void*>>);
41+                      std::pair<ValueType, std::pair<size_t, void*>>
42 #else
43+        std::conditional_t<
44         // libstdc++ doesn't store hash when its operator() is noexcept;
45         // see hashtable_policy.h, struct _Hash_node
46         // https://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a05689.html
47-        if (noexcept(std::declval<Hash>()(std::declval<const Key&>()))) {
48-            return sizeof(std::pair<void*, ValueType>);
49-        } else {
50+            noexcept(std::declval<Hash>()(std::declval<const Key&>())),
51+                          std::pair<void*, ValueType>,
52             // hash is stored along ValueType, and that is wrapped with the pointer.
53-            return sizeof(std::pair<void*, std::pair<ValueType, size_t>>);
54-        }
55+                          std::pair<void*, std::pair<ValueType, size_t>>
56+        >
57 #endif
58 #endif
59+    ;
60+    [[nodiscard]] static constexpr size_t Value()
61+    {
62+        return sizeof(NodeType);
63     }
64 };
65 

Fix above is trying to be minimal but it could be cleaned up more by renaming NodeSize to something like ContainerTraits and dropping the Value() method.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

I’m curious why this is phrased negatively. Is there some downside to swapping allocators when containers are swapped? Swapping everything in the container when the container is swapped seems like the obvious and good thing to do.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

I’ll reword my comment, I meant that the default for propagate_on_container_swap is std::false_type, which is bad because that’s undefined behavior. I’ll change the comment:

0    /**
1     * The default for propagate_on_container_swap is std::false_type. This is bad,because swapping
2     * two containers with unequal allocators but not propagating the allocator is undefined
3     * behavior! Obviously, we want so swap the allocator as well, so we set that to true.
4     *
5     * see https://www.foonathan.net/2015/10/allocatorawarecontainer-propagation-pitfalls/
6     */

Again wondering why it’s phrased like we “have to” do these things. Is there some downside, or some particular way this is not ideal for our use-case?

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

This is interesting. It seems to suggest about the concern that if “the object size gets computed incorrectly by a future libstdc++ change… we’ll silently fall back to old performance” that in theory we could detect at compile time, not runtime, whether a container implementation was calling Allocator::allocate with an unexpected T size for that implementation.

But it doesn’t seem worth implementing this if it requires lots of template trickery or tracking down compile errors on different platforms, especially if NodeSize unit test coverage works OK practically.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Probably should use snake case variable names

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Might suggest calling this something like CHECK_NUMS_FREES_POOLS just so it’s easier to remember what the arguments are to this guy below (and what it is doing).

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Writing this comment feels like reaching new depths of pedantry, but it doesn’t seem guaranteed that void* is bigger than char. It can’t be smaller, because sizeof(char) is always 1, but if they are the same size, then the test isn’t really meaningful. I guess my suggestion would be to replace void* with a type guaranteed to be bigger like std::pair<char, char>.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Could do a stricter check?

0BOOST_CHECK(node_allocator::MemoryResourceTester::NumFreeAllocations(mr) >=
1                    num_free_allocations + 1000);

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Can you add “because its contents were moved to m” to this comment? I was a little confused by what this was doing at first, and the extra hint would have helped.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Why only >50 here, not >=1000 again?

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

It seems like another thing these tests could be checking for aside from number of free allocations and number of pools is amount of slack space the last pool (untouched_end - untouched_iterator)/alloc size.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

I get that it makes sense to have test coverage for std::unordered_map, but I feel like it would be if ideal if unordered_map vagaries with IN_RANGE(1U, 2U) (101U, 102U) soft checks were confined to ~one test, and all other tests used a simpler container like std::list that was easier to think about and had better defined allocation behavior, so the tests would be easier to understand, less fragile, and in some places more meaningful.

Not saying you should rewrite all tests, but this might be something to think about doing selectively or for new tests.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Amount of code duplication between different_memoryresource_* tests is a little eye-watering. This seems like a place where BOOST_FIXTURE_TEST_CASE and a test fixture with unordered_map m_map_a and optional<unordered_map> m_map_b members and some helper methods would cut down duplication and make the checks more human readable.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

Could all of this just be

0template<size_t SIZE, size_t ALIGN>
1struct alignas(ALIGN) A {
2    char data[SIZE];
3};

?

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

IMO would be good to move these A8 and bigger checks out of the preprocessor block to deduplicate and make it obvious these are all the same whether you have have 4 byte or 8 byte pointers.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

deduplicated in e2a5547f12e5da2718ba17ebac5d5aeb5a2e24d5. Also I have added a

0static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ >= std::alignment_of_v<T>, "make sure AllocatePool() aligns correctly");

Into Allocate, because actually the 16 byte alignments are not allowed when the pools are not correctly aligned as well.

Suggest writing /* not noexcept */ to make comment intent more plain.

Also, I have only the vaguest understanding of noexcept but could this be noexcept(false)?

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

The hidden mystery of node_allocator_tests is revealed! This is reason for all the IN_RANGE(101, 102) stuff :arrow_up:. I wish there was a spoiler tag.

(Version: a91b6032f037ab1a26ac4baa761e809fbfeb1bdd)

In commit “Correctly calculate node size for std::unordered_map” (119c3b90d6b7e9b81915a09f5ca5370f2ef1ea57)

Might be good to replace this with a loop and verify there is exactly 1 entry with 123 allocations (and the expected size), instead of at least 1.

In commit “Add allocator for node based containers” (96d8e33acaa079491403d8e8afa6709978dec205)

Would drop “which is given in constructor” (no longer true)

In commit “Add allocator for node based containers” (96d8e33acaa079491403d8e8afa6709978dec205)

Purpose of this test could be a made a little clearer with comment like “Verify that MemoryResource specialized for a larger char pair type will do smaller single char allocations as well”

In commit “Add allocator for node based containers” (96d8e33acaa079491403d8e8afa6709978dec205)

Test is no longer using void*. Probably would just say // freelist is used or drop comment.

In commit “Make use of node_allocator in CCoinsMap” (66f745394ddfb546b9d0d29fe764110944dea158)

Extra punctuation here s/cacheCoins' MemoryResource/cacheCoinsMemoryResource/

In commit “Add allocator for node based containers” (96d8e33acaa079491403d8e8afa6709978dec205)

It doesn’t seem like this static_assert is verifying anything related to our code. It’s not calling any of our own functions, just verifying a relationship between __STDCPP_DEFAULT_NEW_ALIGNMENT__ and std::alignment_of. Also assert message doesn’t seem to make sense. Check might make more sense inside the RequiredAllocationSizeBytes function than outside here.

re: #22702 (review)

I think that check is necessary. In AllocatePool I’m doing new char[POOL_SIZE_BYTES]

Maybe can move check closer to where it’s needed

 0diff --git a/src/support/allocators/node_allocator/memory_resource.h b/src/support/allocators/node_allocator/memory_resource.h
 1index f4ba0138924..cf956da318f 100644
 2--- a/src/support/allocators/node_allocator/memory_resource.h
 3+++ b/src/support/allocators/node_allocator/memory_resource.h
 4@@ -101,7 +101,6 @@ public:
 5     [[nodiscard]] T* Allocate()
 6     {
 7         static_assert(ALLOCATION_SIZE_BYTES == detail::RequiredAllocationSizeBytes<T>());
 8-        static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ >= std::alignment_of_v<T>, "make sure AllocatePool() aligns correctly");
 9 
10         if (m_free_allocations) {
11             // we've already got data in the free list, unlink one element
12@@ -120,7 +119,7 @@ public:
13         // than what has been malloc'ed.
14         if (m_untouched_memory_iterator == m_untouched_memory_end) {
15             // slow path, only happens when a new pool needs to be allocated
16-            AllocatePool();
17+            AllocatePool<T>();
18         }
19 
20         // peel off one allocation from the untouched memory. The next pointer of in-use
21@@ -164,8 +163,10 @@ private:
22     /**
23      * Allocate one full memory pool which is used to carve out allocations.
24      */
25+    template <typename T>
26     void AllocatePool()
27     {
28+        static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ >= std::alignment_of_v<T>, "make sure alignment is compatible with T");
29         m_allocated_pools.emplace_back(new char[POOL_SIZE_BYTES]);
30         m_untouched_memory_iterator = m_allocated_pools.back().get();
31         m_untouched_memory_end = m_untouched_memory_iterator + POOL_SIZE_BYTES;