From 02741a7706aa53f56bfcb0def7c504b373dbe9ac Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 22 Nov 2023 11:12:00 +0000 Subject: [PATCH] Merge bitcoin/bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment d5b4c0b69e543de51bb37d602d488ee0949ba185 pool: change memusage_test to use int64_t, add allocation check (Martin Leitner-Ankerl) ce881bf9fcb7c30bb1fafd6ce38844f4f829452a pool: make sure PoolAllocator uses the correct alignment (Martin Leitner-Ankerl) Pull request description: The class `CTxOut` has a member `CAmount` which is an int64_t, and on ARM 32bit int64_t are 8 byte aligned, which is larger than the pointer alignment of 4 bytes. So for `CCoinsMap` to be able to use the pool, we need to use the alignment of the member instead of just `alignof(void*)`. This fixes #28906 (first noted in https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1807197107) and #28440. ACKs for top commit: pinheadmz: ACK d5b4c0b69e543de51bb37d602d488ee0949ba185 hebasto: re-ACK d5b4c0b69e543de51bb37d602d488ee0949ba185, the only change since my recent [review](https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1739334189) is an updated test. theStack: Tested ACK d5b4c0b69e543de51bb37d602d488ee0949ba185 Tree-SHA512: 4446793fad6d56f0fe22e09ac9ade051e86de11ac039cd61c0f6b7f79874242878a6a46a2c76ac3b8f1d53464872620d39139f54b1471daccad26d6bb1ae8ca1 --- src/bench/pool.cpp | 3 +-- src/coins.h | 3 +-- src/support/allocators/pool.h | 2 +- src/test/pool_tests.cpp | 22 +++++++++++++--------- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/bench/pool.cpp b/src/bench/pool.cpp index 0bf2b18514..0ce3b952f8 100644 --- a/src/bench/pool.cpp +++ b/src/bench/pool.cpp @@ -37,8 +37,7 @@ static void PoolAllocator_StdUnorderedMapWithPoolResource(benchmark::Bench& benc std::hash, std::equal_to, PoolAllocator, - sizeof(std::pair) + 4 * sizeof(void*), - alignof(void*)>>; + sizeof(std::pair) + 4 * sizeof(void*)>>; // make sure the resource supports large enough pools to hold the node. We do this by adding the size of a few pointers to it. auto pool_resource = Map::allocator_type::ResourceType(); diff --git a/src/coins.h b/src/coins.h index fde5f688cc..369a645e49 100644 --- a/src/coins.h +++ b/src/coins.h @@ -145,8 +145,7 @@ using CCoinsMap = std::unordered_map, PoolAllocator, - sizeof(std::pair) + sizeof(void*) * 4, - alignof(void*)>>; + sizeof(std::pair) + sizeof(void*) * 4>>; using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType; diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h index c8e70ebacf..873e260b65 100644 --- a/src/support/allocators/pool.h +++ b/src/support/allocators/pool.h @@ -272,7 +272,7 @@ public: /** * Forwards all allocations/deallocations to the PoolResource. */ -template +template class PoolAllocator { PoolResource* m_resource; diff --git a/src/test/pool_tests.cpp b/src/test/pool_tests.cpp index 3f8ed3636d..e83b41e5ba 100644 --- a/src/test/pool_tests.cpp +++ b/src/test/pool_tests.cpp @@ -155,21 +155,20 @@ BOOST_AUTO_TEST_CASE(random_allocations) BOOST_AUTO_TEST_CASE(memusage_test) { - auto std_map = std::unordered_map{}; + auto std_map = std::unordered_map{}; - using Map = std::unordered_map, - std::equal_to, - PoolAllocator, - sizeof(std::pair) + sizeof(void*) * 4, - alignof(void*)>>; + using Map = std::unordered_map, + std::equal_to, + PoolAllocator, + sizeof(std::pair) + sizeof(void*) * 4>>; auto resource = Map::allocator_type::ResourceType(1024); PoolResourceTester::CheckAllDataAccountedFor(resource); { - auto resource_map = Map{0, std::hash{}, std::equal_to{}, &resource}; + auto resource_map = Map{0, std::hash{}, std::equal_to{}, &resource}; // can't have the same resource usage BOOST_TEST(memusage::DynamicUsage(std_map) != memusage::DynamicUsage(resource_map)); @@ -181,6 +180,11 @@ BOOST_AUTO_TEST_CASE(memusage_test) // Eventually the resource_map should have a much lower memory usage because it has less malloc overhead BOOST_TEST(memusage::DynamicUsage(resource_map) <= memusage::DynamicUsage(std_map) * 90 / 100); + + // Make sure the pool is actually used by the nodes + auto max_nodes_per_chunk = resource.ChunkSizeBytes() / sizeof(Map::value_type); + auto min_num_allocated_chunks = resource_map.size() / max_nodes_per_chunk + 1; + BOOST_TEST(resource.NumAllocatedChunks() >= min_num_allocated_chunks); } PoolResourceTester::CheckAllDataAccountedFor(resource);