From c81e9c38d4d047cd65803f65a662b2d2c7d18048 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 25 Apr 2023 16:02:28 +0100 Subject: [PATCH 1/3] Made pool reuse a queue So allocator churn will cause remote queues to be visited. --- src/snmalloc/mem/pool.h | 83 +++++++++++++++++++++++++++++++------- src/snmalloc/mem/pooled.h | 4 +- src/test/func/pool/pool.cc | 7 +++- 3 files changed, 75 insertions(+), 19 deletions(-) diff --git a/src/snmalloc/mem/pool.h b/src/snmalloc/mem/pool.h index 7caddd2a3..c541b82b3 100644 --- a/src/snmalloc/mem/pool.h +++ b/src/snmalloc/mem/pool.h @@ -27,7 +27,11 @@ namespace snmalloc friend class Pool; private: - MPMCStack stack; + // Queue of elements in not currently in use + // Must hold lock to modify + capptr::Alloc front{nullptr}; + capptr::Alloc back{nullptr}; + FlagWord lock{}; capptr::Alloc list{nullptr}; @@ -121,12 +125,20 @@ namespace snmalloc static T* acquire(Args&&... args) { PoolState& pool = get_state(); - auto p = capptr::Alloc::unsafe_from(pool.stack.pop()); - - if (p != nullptr) { - p->set_in_use(); - return p.unsafe_ptr(); + FlagLock f(pool.lock); + if (pool.front != nullptr) + { + auto p = pool.front; + auto next = p->next; + if (next == nullptr) + { + pool.back = nullptr; + } + pool.front = next; + p->set_in_use(); + return p.unsafe_ptr(); + } } auto raw = @@ -137,7 +149,7 @@ namespace snmalloc Config::Pal::error("Failed to initialise thread local allocator."); } - p = capptr::Alloc::unsafe_from(new (raw.unsafe_ptr()) + auto p = capptr::Alloc::unsafe_from(new (raw.unsafe_ptr()) T(std::forward(args)...)); FlagLock f(pool.lock); @@ -159,16 +171,23 @@ namespace snmalloc // is returned without the constructor being run, so the object is reused // without re-initialisation. p->reset_in_use(); - get_state().stack.push(p); + restore(p, p); } static T* extract(T* p = nullptr) { + PoolState& pool = get_state(); // Returns a linked list of all objects in the stack, emptying the stack. if (p == nullptr) - return get_state().stack.pop_all(); + { + FlagLock f(pool.lock); + auto result = pool.front; + pool.front = nullptr; + pool.back = nullptr; + return result.unsafe_ptr(); + } - return p->next; + return p->next.unsafe_ptr(); } /** @@ -178,9 +197,45 @@ namespace snmalloc */ static void restore(T* first, T* last) { - // Pushes a linked list of objects onto the stack. Use to put a linked + PoolState& pool = get_state(); + last->next = nullptr; + FlagLock f(pool.lock); + // Pushes a linked list of objects onto the stack. Used to put a linked // list returned by extract back onto the stack. - get_state().stack.push(first, last); + if (pool.front == nullptr) + { + pool.front = capptr::Alloc::unsafe_from(first); + } + else + { + pool.back->next = capptr::Alloc::unsafe_from(first); + } + + pool.back = capptr::Alloc::unsafe_from(last); + } + + /** + * Return to the pool a list of object previously retrieved by `extract` + * + * Do not return objects from `acquire`. + */ + static void restore_front(T* first, T* last) + { + PoolState& pool = get_state(); + last->next = nullptr; + FlagLock f(pool.lock); + // Pushes a linked list of objects onto the stack. Used to put a linked + // list returned by extract back onto the stack. + if (pool.front == nullptr) + { + pool.back = capptr::Alloc::unsafe_from(last); + } + else + { + last->next = pool.front; + pool.back->next = capptr::Alloc::unsafe_from(first); + } + pool.front = capptr::Alloc::unsafe_from(first); } static T* iterate(T* p = nullptr) @@ -200,7 +255,7 @@ namespace snmalloc static void sort() { // Marker is used to signify free elements. - auto marker = reinterpret_cast(1); + auto marker = capptr::Alloc::unsafe_from(reinterpret_cast(1)); // Extract all the elements and mark them as free. T* curr = extract(); @@ -222,7 +277,7 @@ namespace snmalloc { if (curr->next == marker) { - get_state().stack.push(curr); + restore_front(curr, curr); } curr = iterate(curr); } diff --git a/src/snmalloc/mem/pooled.h b/src/snmalloc/mem/pooled.h index 086cd226b..a812bc924 100644 --- a/src/snmalloc/mem/pooled.h +++ b/src/snmalloc/mem/pooled.h @@ -17,11 +17,9 @@ namespace snmalloc SNMALLOC_CONCEPT(IsConfig) Config, PoolState& get_state()> friend class Pool; - template - friend class MPMCStack; /// Used by the pool for chaining together entries when not in use. - std::atomic next{nullptr}; + capptr::Alloc next{nullptr}; /// Used by the pool to keep the list of all entries ever created. capptr::Alloc list_next; std::atomic in_use{false}; diff --git a/src/test/func/pool/pool.cc b/src/test/func/pool/pool.cc index c721ab2e5..7eeff8743 100644 --- a/src/test/func/pool/pool.cc +++ b/src/test/func/pool/pool.cc @@ -110,6 +110,9 @@ void test_double_alloc() SNMALLOC_CHECK(ptr1 != ptr2); PoolA::release(ptr2); auto ptr3 = PoolA::acquire(); + // The following check assumes a stack discipline for acquire/release. + // Placing it first in the list of tests means, there is a single element + // and thus it works for both stack and queue. SNMALLOC_CHECK(ptr2 == ptr3); PoolA::release(ptr1); PoolA::release(ptr3); @@ -219,14 +222,14 @@ int main(int argc, char** argv) UNUSED(argc, argv); #endif + test_double_alloc(); + std::cout << "test_double_alloc passed" << std::endl; test_alloc(); std::cout << "test_alloc passed" << std::endl; test_constructor(); std::cout << "test_constructor passed" << std::endl; test_alloc_many(); std::cout << "test_alloc_many passed" << std::endl; - test_double_alloc(); - std::cout << "test_double_alloc passed" << std::endl; test_different_alloc(); std::cout << "test_different_alloc passed" << std::endl; test_iterator(); From 2f1b9c746bd3d59b02aecbc2aebe6f689f0b42b8 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 25 Apr 2023 20:41:03 +0100 Subject: [PATCH 2/3] Remove redundant comments --- src/snmalloc/mem/pool.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/snmalloc/mem/pool.h b/src/snmalloc/mem/pool.h index c541b82b3..7ccbdd2a1 100644 --- a/src/snmalloc/mem/pool.h +++ b/src/snmalloc/mem/pool.h @@ -200,8 +200,7 @@ namespace snmalloc PoolState& pool = get_state(); last->next = nullptr; FlagLock f(pool.lock); - // Pushes a linked list of objects onto the stack. Used to put a linked - // list returned by extract back onto the stack. + if (pool.front == nullptr) { pool.front = capptr::Alloc::unsafe_from(first); @@ -224,8 +223,7 @@ namespace snmalloc PoolState& pool = get_state(); last->next = nullptr; FlagLock f(pool.lock); - // Pushes a linked list of objects onto the stack. Used to put a linked - // list returned by extract back onto the stack. + if (pool.front == nullptr) { pool.back = capptr::Alloc::unsafe_from(last); From 54c9633105ff22eb770e2b000e65778dc8384976 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 26 Apr 2023 14:28:49 +0100 Subject: [PATCH 3/3] Clangformat --- src/snmalloc/mem/pool.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/snmalloc/mem/pool.h b/src/snmalloc/mem/pool.h index 7ccbdd2a1..36737207d 100644 --- a/src/snmalloc/mem/pool.h +++ b/src/snmalloc/mem/pool.h @@ -150,7 +150,7 @@ namespace snmalloc } auto p = capptr::Alloc::unsafe_from(new (raw.unsafe_ptr()) - T(std::forward(args)...)); + T(std::forward(args)...)); FlagLock f(pool.lock); p->list_next = pool.list; @@ -207,7 +207,7 @@ namespace snmalloc } else { - pool.back->next = capptr::Alloc::unsafe_from(first); + pool.back->next = capptr::Alloc::unsafe_from(first); } pool.back = capptr::Alloc::unsafe_from(last); @@ -230,8 +230,8 @@ namespace snmalloc } else { - last->next = pool.front; - pool.back->next = capptr::Alloc::unsafe_from(first); + last->next = pool.front; + pool.back->next = capptr::Alloc::unsafe_from(first); } pool.front = capptr::Alloc::unsafe_from(first); }