From b8a5d7fca96fd442eb74df8db59666bd1704696c Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Tue, 2 Jul 2019 16:07:56 +0100 Subject: [PATCH 01/11] Lazily initialise TLS on slow paths. Copying an idea from mimalloc, initialise the TLS variable to a global allocator that doesn't own any memory and then lazily check when we hit a slow path (which we always do when using the global allocator, because it doesn't own any memory) if we are the global allocator and replace it. There is a slight complication compared to mimalloc's version of this idea. Snmalloc collects outgoing messages and it's possible for the first operation in a thread to be a free of memory allocated by a different thread. We address this by initialising the queues with a size value indicating that they are full and then do the lazy check when about to insert a message that would make a queue full. This will then trigger lazy creation of an allocator. Global initialisation doesn't work for the fake allocator, so skip most of its constructor. --- src/mem/alloc.h | 96 ++++++++++++++--- src/mem/globalalloc.h | 21 +++- src/mem/threadalloc.h | 139 +++++++++++++++---------- src/test/perf/contention/contention.cc | 2 +- 4 files changed, 184 insertions(+), 74 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 7e5e569..67dd05e 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -234,6 +234,11 @@ namespace snmalloc FastFreeLists() : small_fast_free_lists() {} }; + ALWAYSINLINE inline void* no_replacement(void*) + { + return nullptr; + } + /** * Allocator. This class is parameterised on three template parameters. The * `MemoryProvider` defines the source of memory for this allocator. @@ -245,18 +250,27 @@ namespace snmalloc * to associate metadata with large (16MiB, by default) regions, allowing an * allocator to find the allocator responsible for that region. * - * The final template parameter, `IsQueueInline`, defines whether the + * The next template parameter, `IsQueueInline`, defines whether the * message queue for this allocator should be stored as a field of the * allocator (`true`) or provided externally, allowing it to be anywhere else * in the address space (`false`). + * + * The final template parameter provides a hook to allow the allocator in use + * to be dynamically modified. This is used to implement a trick from + * mimalloc that avoids a conditional branch on the fast path. We initialise + * the thread-local allocator pointer with the address of a global allocator, + * which never owns any memory. When we try to allocate memory, we call the + * replacement function. */ template< class MemoryProvider = GlobalVirtual, class PageMap = SNMALLOC_DEFAULT_PAGEMAP, - bool IsQueueInline = true> + bool IsQueueInline = true, + void* (*Replacement)(void*) = no_replacement> class Allocator : public FastFreeLists, - public Pooled> + public Pooled< + Allocator> { LargeAlloc large_allocator; PageMap page_map; @@ -637,7 +651,14 @@ namespace snmalloc struct RemoteCache { - size_t size = 0; + /** + * The total amount of memory stored awaiting dispatch to other + * allocators. This is initialised to the maximum size that we use + * before caching so that, when we hit the slow path and need to dispatch + * everything, we can check if we are a real allocator and lazily provide + * a real allocator. + */ + size_t size = REMOTE_CACHE; RemoteList list[REMOTE_SLOTS]; /// Used to find the index into the array of queues for remote @@ -645,17 +666,18 @@ namespace snmalloc /// r is used for which round of sending this is. inline size_t get_slot(size_t id, size_t r) { - constexpr size_t allocator_size = - sizeof(Allocator); + constexpr size_t allocator_size = sizeof( + Allocator); constexpr size_t initial_shift = bits::next_pow2_bits_const(allocator_size); + assert((initial_shift - (r * REMOTE_SLOT_BITS)) < 64); return (id >> (initial_shift + (r * REMOTE_SLOT_BITS))) & REMOTE_MASK; } SNMALLOC_FAST_PATH void - dealloc(alloc_id_t target_id, void* p, sizeclass_t sizeclass) + dealloc_sized(alloc_id_t target_id, void* p, size_t objectsize) { - this->size += sizeclass_to_size(sizeclass); + this->size += objectsize; Remote* r = static_cast(p); r->set_target_id(target_id); @@ -666,6 +688,12 @@ namespace snmalloc l->last = r; } + SNMALLOC_FAST_PATH void + dealloc(alloc_id_t target_id, void* p, sizeclass_t sizeclass) + { + dealloc_sized(target_id, p, sizeclass_to_size(sizeclass)); + } + void post(alloc_id_t id) { // When the cache gets big, post lists to their target allocators. @@ -780,7 +808,10 @@ namespace snmalloc public: Allocator( - MemoryProvider& m, PageMap&& p = PageMap(), RemoteAllocator* r = nullptr) + MemoryProvider& m, + PageMap&& p = PageMap(), + RemoteAllocator* r = nullptr, + bool isFake = false) : large_allocator(m), page_map(p) { if constexpr (IsQueueInline) @@ -796,6 +827,11 @@ namespace snmalloc if (id() >= static_cast(-1)) error("Id should not be -1"); + // If this is fake, don't do any of the bits of initialisation that may + // allocate memory. + if (isFake) + return; + init_message_queue(); message_queue().invariant(); @@ -1055,6 +1091,11 @@ namespace snmalloc template SNMALLOC_SLOW_PATH void* small_alloc_slow(sizeclass_t sizeclass) { + if (void* replacement = Replacement(this)) + { + return reinterpret_cast(replacement) + ->template small_alloc_slow(sizeclass); + } handle_message_queue(); size_t rsize = sizeclass_to_size(sizeclass); auto& sl = small_classes[sizeclass]; @@ -1205,6 +1246,12 @@ namespace snmalloc } else { + if (void* replacement = Replacement(this)) + { + return reinterpret_cast(replacement) + ->template medium_alloc( + sizeclass, rsize, size); + } slab = reinterpret_cast( large_allocator.template alloc( 0, SUPERSLAB_SIZE)); @@ -1277,6 +1324,12 @@ namespace snmalloc zero_mem == YesZero ? "zeromem" : "nozeromem", allow_reserve == NoReserve ? "noreserve" : "reserve")); + if (void* replacement = Replacement(this)) + { + return reinterpret_cast(replacement) + ->template large_alloc(size); + } + size_t size_bits = bits::next_pow2_bits(size); size_t large_class = size_bits - SUPERSLAB_BITS; assert(large_class < NUM_LARGE_CLASSES); @@ -1317,17 +1370,36 @@ namespace snmalloc remote_dealloc(RemoteAllocator* target, void* p, sizeclass_t sizeclass) { MEASURE_TIME(remote_dealloc, 4, 16); + assert(target->id() != id()); handle_message_queue(); void* offseted = apply_cache_friendly_offset(p, sizeclass); + // Check whether this will overflow the cache first. If we are a fake + // allocator, then our cache will always be full and so we will never hit + // this path. + size_t sz = sizeclass_to_size(sizeclass); + if ((remote.size + sz) < REMOTE_CACHE) + { + stats().remote_free(sizeclass); + remote.dealloc_sized(target->id(), offseted, sz); + return; + } + // Now that we've established that we're in the slow path (if we're a + // real allocator, we will have to empty our cache now), check if we are + // a real allocator and construct one if we aren't. + if (void* replacement = Replacement(this)) + { + // We have to do a dealloc, not a remote_dealloc here because this may + // have been allocated with the allocator that we've just had returned. + reinterpret_cast(replacement)->dealloc(p); + return; + } + stats().remote_free(sizeclass); remote.dealloc(target->id(), offseted, sizeclass); - if (remote.size < REMOTE_CACHE) - return; - stats().remote_post(); remote.post(id()); } diff --git a/src/mem/globalalloc.h b/src/mem/globalalloc.h index 552c313..bda5c35 100644 --- a/src/mem/globalalloc.h +++ b/src/mem/globalalloc.h @@ -6,11 +6,25 @@ namespace snmalloc { + inline void* lazy_replacement(void*); + using Alloc = + Allocator; + template - class AllocPool : Pool, MemoryProvider> + class AllocPool : Pool< + Allocator< + MemoryProvider, + SNMALLOC_DEFAULT_PAGEMAP, + true, + lazy_replacement>, + MemoryProvider> { - using Alloc = Allocator; - using Parent = Pool, MemoryProvider>; + using Alloc = Allocator< + MemoryProvider, + SNMALLOC_DEFAULT_PAGEMAP, + true, + lazy_replacement>; + using Parent = Pool; public: static AllocPool* make(MemoryProvider& mp) @@ -175,5 +189,4 @@ namespace snmalloc return AllocPool::make(mp); } - using Alloc = Allocator; } // namespace snmalloc diff --git a/src/mem/threadalloc.h b/src/mem/threadalloc.h index d19cd16..461172e 100644 --- a/src/mem/threadalloc.h +++ b/src/mem/threadalloc.h @@ -15,6 +15,16 @@ namespace snmalloc { extern "C" void _malloc_thread_cleanup(void); + /** + * A global fake allocator object. This never allocates memory and, as a + * result, never owns any slabs. On the slow paths, where it would fetch + * slabs to allocate from, it will discover that it is the placeholder and + * replace itself with the thread-local allocator, allocating one if + * required. This avoids a branch on the fast path. + */ + HEADER_GLOBAL Alloc GlobalPlaceHolder( + default_memory_provider, SNMALLOC_DEFAULT_PAGEMAP(), nullptr, true); + #ifdef SNMALLOC_EXTERNAL_THREAD_ALLOC /** * Version of the `ThreadAlloc` interface that does no management of thread @@ -32,6 +42,7 @@ namespace snmalloc { return (Alloc*&)ThreadAllocUntyped::get(); } + static void register_cleanup() {} }; #endif @@ -59,7 +70,8 @@ namespace snmalloc */ static inline void exit() { - if (auto* per_thread = get(false)) + auto* per_thread = get(); + if ((per_thread != &GlobalPlaceHolder) && (per_thread != nullptr)) { current_alloc_pool()->release(per_thread); per_thread = nullptr; @@ -76,15 +88,12 @@ namespace snmalloc * The non-create case exists so that the `per_thread` variable can be a * local static and not a global, allowing ODR to deduplicate it. */ - static SNMALLOC_FAST_PATH Alloc*& get(bool create = true) + static SNMALLOC_FAST_PATH Alloc*& get() { - static thread_local Alloc* per_thread; - if (!per_thread && create) - { - per_thread = current_alloc_pool()->acquire(); - } + static thread_local Alloc* per_thread = &GlobalPlaceHolder; return per_thread; } + static void register_cleanup() {} }; /** * Version of the `ThreadAlloc` interface that uses C++ `thread_local` @@ -109,7 +118,7 @@ namespace snmalloc * Constructor. Acquires a new allocator and associates it with this * object. There should be only one instance of this class per thread. */ - ThreadAllocThreadDestructor() : alloc(current_alloc_pool()->acquire()) {} + ThreadAllocThreadDestructor() : alloc(&GlobalPlaceHolder) {} /** * Destructor. Releases the allocator owned by this thread. @@ -129,6 +138,7 @@ namespace snmalloc static thread_local ThreadAllocThreadDestructor per_thread; return per_thread.alloc; } + static void register_cleanup() {} }; // When targeting the FreeBSD kernel, the pthread header exists, but the // pthread symbols do not, so don't compile this because it will fail to @@ -157,9 +167,13 @@ namespace snmalloc # endif thread_alloc_release(void* p) { - Alloc** pp = static_cast(p); - current_alloc_pool()->release(*pp); + // Keep pthreads happy + void** pp = reinterpret_cast(p); *pp = nullptr; + // Actually destroy the allocator and reset TLS + Alloc*& alloc = ThreadAllocExplicitTLSCleanup::get(); + current_alloc_pool()->release(alloc); + alloc = &GlobalPlaceHolder; } # ifdef _WIN32 @@ -185,9 +199,9 @@ namespace snmalloc * * This must not be called until after `tls_key_create` has returned. */ - static inline void tls_set_value(tls_key_t key, Alloc** value) + static inline void tls_set_value(tls_key_t key, Alloc* value) { - FlsSetValue(key, static_cast(value)); + FlsSetValue(key, value); } # else /** @@ -214,9 +228,9 @@ namespace snmalloc * * This must not be called until after `tls_key_create` has returned. */ - static inline void tls_set_value(tls_key_t key, Alloc** value) + static inline void tls_set_value(tls_key_t key, Alloc* value) { - pthread_setspecific(key, static_cast(value)); + pthread_setspecific(key, value); } # endif @@ -226,7 +240,7 @@ namespace snmalloc */ static SNMALLOC_FAST_PATH Alloc*& inner_get() { - static thread_local Alloc* per_thread; + static thread_local Alloc* per_thread = &GlobalPlaceHolder; return per_thread; } @@ -239,42 +253,6 @@ namespace snmalloc } # endif - /** - * Private initialiser for the per thread allocator - */ - static SNMALLOC_SLOW_PATH Alloc*& inner_init() - { - Alloc*& per_thread = inner_get(); - - // If we don't have an allocator, construct one. - if (!per_thread) - { - // Construct the allocator and assign it to `per_thread` *before* doing - // anything else. This is important because `tls_key_create` may - // allocate memory and if we are providing the `malloc` implementation - // then this function must be re-entrant within a single thread. In - // this case, the second call to this function will simply return the - // allocator. - per_thread = current_alloc_pool()->acquire(); - - bool first = false; - tls_key_t key = Singleton::get(&first); - // Associate the new allocator with the destructor. - tls_set_value(key, &per_thread); - -# ifdef USE_SNMALLOC_STATS - // Allocator is up and running now, safe to call atexit. - if (first) - { - atexit(print_stats); - } -# else - UNUSED(first); -# endif - } - return per_thread; - } - public: /** * Public interface, returns the allocator for the current thread, @@ -282,13 +260,25 @@ namespace snmalloc */ static SNMALLOC_FAST_PATH Alloc*& get() { - Alloc*& per_thread = inner_get(); + return inner_get(); + } + static void register_cleanup() + { + // Register the allocator destructor. + bool first = false; + tls_key_t key = Singleton::get(&first); + // Associate the new allocator with the destructor. + tls_set_value(key, &GlobalPlaceHolder); - if (likely(per_thread != nullptr)) - return per_thread; - - // Slow path that performs initialization - return inner_init(); +# ifdef USE_SNMALLOC_STATS + // Allocator is up and running now, safe to call atexit. + if (first) + { + atexit(print_stats); + } +# else + UNUSED(first); +# endif } }; #endif @@ -310,4 +300,39 @@ namespace snmalloc #else using ThreadAlloc = ThreadAllocExplicitTLSCleanup; #endif + + /** + * Slow path for the placeholder replacement. The simple check that this is + * the global placeholder is inlined, the rest of it is only hit in a very + * unusual case and so should go off the fast path. + */ + SNMALLOC_SLOW_PATH inline void* lazy_replacement_slow() + { + auto*& local_alloc = ThreadAlloc::get(); + if ((local_alloc != nullptr) && (local_alloc != &GlobalPlaceHolder)) + { + return local_alloc; + } + local_alloc = current_alloc_pool()->acquire(); + ThreadAlloc::register_cleanup(); + return local_alloc; + } + + /** + * Function passed as a template parameter to `Allocator` to allow lazy + * replacement. This is called on all of the slow paths in `Allocator`. If + * the caller is the global placeholder allocator then this function will + * check if we've already allocated a per-thread allocator, returning it if + * so. If we have not allocated a per-thread allocator yet, then this + * function will allocate one. + */ + ALWAYSINLINE inline void* lazy_replacement(void* existing) + { + if (existing != &GlobalPlaceHolder) + { + return nullptr; + } + return lazy_replacement_slow(); + } + } // namespace snmalloc diff --git a/src/test/perf/contention/contention.cc b/src/test/perf/contention/contention.cc index d799baf..3c36634 100644 --- a/src/test/perf/contention/contention.cc +++ b/src/test/perf/contention/contention.cc @@ -75,7 +75,7 @@ size_t swapcount; void test_tasks_f(size_t id) { - Alloc* a = ThreadAlloc::get(); + Alloc*& a = ThreadAlloc::get(); xoroshiro::p128r32 r(id + 5000); for (size_t n = 0; n < swapcount; n++) From 829d8e856bed660238a14d9ddaa08c0bcc00a6aa Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 4 Jul 2019 15:06:51 +0100 Subject: [PATCH 02/11] Tweak some tests to avoid the slowest path. By caching the result of the first call to ThreadAlloc::get(), we were always hitting a code path that should be hit once per thread in normal operation. --- src/test/perf/external_pointer/externalpointer.cc | 3 +++ src/test/perf/singlethread/singlethread.cc | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/perf/external_pointer/externalpointer.cc b/src/test/perf/external_pointer/externalpointer.cc index 20c9676..4576be3 100644 --- a/src/test/perf/external_pointer/externalpointer.cc +++ b/src/test/perf/external_pointer/externalpointer.cc @@ -69,6 +69,9 @@ namespace test int main(int, char**) { xoroshiro::p128r64 r; + // Force a per-thread allocator to actually exist. + void* p = ThreadAlloc::get()->alloc(16); + ThreadAlloc::get()->dealloc(p); #if NDEBUG size_t nn = 30; #else diff --git a/src/test/perf/singlethread/singlethread.cc b/src/test/perf/singlethread/singlethread.cc index 3b3700c..aef53f2 100644 --- a/src/test/perf/singlethread/singlethread.cc +++ b/src/test/perf/singlethread/singlethread.cc @@ -7,7 +7,7 @@ using namespace snmalloc; template void test_alloc_dealloc(size_t count, size_t size, bool write) { - auto* alloc = ThreadAlloc::get(); + auto*& alloc = ThreadAlloc::get(); DO_TIME( "Count: " << std::setw(6) << count << ", Size: " << std::setw(6) << size From 896b248c8ccea44ba52a99e2f18b6886a1a9c8d4 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 09:16:08 +0100 Subject: [PATCH 03/11] Fix undefined behaviour in FreeBSD PAL. We were passing an argument less than 4K to the MAP_ALIGNED macro, which caused an undefined shift. The compiler helpfully propagated the undef values back to earlier in the code and gave us some exciting nonsense. --- src/pal/pal_freebsd.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/pal/pal_freebsd.h b/src/pal/pal_freebsd.h index 9f3725b..8487f19 100644 --- a/src/pal/pal_freebsd.h +++ b/src/pal/pal_freebsd.h @@ -77,10 +77,7 @@ namespace snmalloc // Alignment must be a power of 2. assert(align == bits::next_pow2(align)); - if (align == 0) - { - align = 1; - } + align = bits::max(4096, align); size_t log2align = bits::next_pow2_bits(align); From 6eeb273cbd88e70dbdc7db61b2a68fa20d8953b3 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 09:38:18 +0100 Subject: [PATCH 04/11] Add -fomit-frame-pointer to CMakeLists for optimised builds. --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e77a7d9..4709aa8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -105,7 +105,7 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /Zi") set(CMAKE_EXE_LINKER_FLAGS_RELEASE "${CMAKE_EXE_LINKER_FLAGS_RELEASE} /DEBUG") else() - add_compile_options(-march=native -fno-exceptions -fno-rtti -g -ftls-model=initial-exec) + add_compile_options(-march=native -fno-exceptions -fno-rtti -g -ftls-model=initial-exec -fomit-frame-pointer) endif() macro(subdirlist result curdir) From 1e65aafa06e24b4193f8b7fc6f6c5002ac687808 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 09:38:58 +0100 Subject: [PATCH 05/11] Add missing fast-path annotations. --- src/mem/alloc.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 67dd05e..c7dac3c 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -288,7 +288,7 @@ namespace snmalloc size_t size, ZeroMem zero_mem = NoZero, AllowReserve allow_reserve = YesReserve> - ALLOCATOR void* alloc() + SNMALLOC_FAST_PATH ALLOCATOR void* alloc() { static_assert(size != 0, "Size must not be zero."); #ifdef USE_MALLOC @@ -324,7 +324,7 @@ namespace snmalloc } template - inline ALLOCATOR void* alloc(size_t size) + SNMALLOC_FAST_PATH ALLOCATOR void* alloc(size_t size) { #ifdef USE_MALLOC static_assert( From eefc9e49c52d667bc3cc73fca228e0c2c4d5bcaf Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 11:15:38 +0100 Subject: [PATCH 06/11] Fix some duplicate inline warnings. --- src/mem/alloc.h | 2 +- src/mem/threadalloc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index c7dac3c..f5f3fe3 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -234,7 +234,7 @@ namespace snmalloc FastFreeLists() : small_fast_free_lists() {} }; - ALWAYSINLINE inline void* no_replacement(void*) + SNMALLOC_FAST_PATH void* no_replacement(void*) { return nullptr; } diff --git a/src/mem/threadalloc.h b/src/mem/threadalloc.h index 461172e..ede455d 100644 --- a/src/mem/threadalloc.h +++ b/src/mem/threadalloc.h @@ -326,7 +326,7 @@ namespace snmalloc * so. If we have not allocated a per-thread allocator yet, then this * function will allocate one. */ - ALWAYSINLINE inline void* lazy_replacement(void* existing) + ALWAYSINLINE void* lazy_replacement(void* existing) { if (existing != &GlobalPlaceHolder) { From 50695d07f83272bc25e02deb1b02310d424f4183 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 11:39:14 +0100 Subject: [PATCH 07/11] Disable an always_inline with GCC in debug mode. Most compilers are happy if you say always-inline but they can't. GCC will complain. Here, we have two mutually recursive functions that are marked as always inline. In an optimised build, one is inlined into the other and then becomes a tail-recursive function that should inline the tail call. Inlining the tail call can be done by simply jumping to the start of the function and so everything is fine. In a debug build, the second transform doesn't happen and so we're left with a call to an always-inline function. --- src/mem/alloc.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index f5f3fe3..94d209d 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -1366,8 +1366,15 @@ namespace snmalloc large_allocator.dealloc(slab, large_class); } - SNMALLOC_FAST_PATH void - remote_dealloc(RemoteAllocator* target, void* p, sizeclass_t sizeclass) +#if defined(__GNUC__) && !defined(__clang__) && !defined(__OPTIMIZE__) + // Don't force this to be always inlined in debug builds with GCC, because + // it will fail and then raise an error. + inline +#else + SNMALLOC_FAST_PATH +#endif + void + remote_dealloc(RemoteAllocator* target, void* p, sizeclass_t sizeclass) { MEASURE_TIME(remote_dealloc, 4, 16); assert(target->id() != id()); From 14b5c57b5530dc128fe411181d28c5a4f6c6a7fe Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 13:24:28 +0100 Subject: [PATCH 08/11] Fix all of the tests. --- src/test/func/fixed_region/fixed_region.cc | 2 +- src/test/func/memory/memory.cc | 16 ++++++++-------- src/test/perf/contention/contention.cc | 2 +- .../perf/external_pointer/externalpointer.cc | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/test/func/fixed_region/fixed_region.cc b/src/test/func/fixed_region/fixed_region.cc index fe1ae39..8ac2338 100644 --- a/src/test/func/fixed_region/fixed_region.cc +++ b/src/test/func/fixed_region/fixed_region.cc @@ -37,7 +37,7 @@ int main() oe_end = (uint8_t*)oe_base + size; std::cout << "Allocated region " << oe_base << " - " << oe_end << std::endl; - auto a = ThreadAlloc::get(); + auto& a = ThreadAlloc::get(); for (size_t i = 0; i < 1000; i++) { diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index a9bdec1..cbfa26e 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -7,7 +7,7 @@ using namespace snmalloc; void test_alloc_dealloc_64k() { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); constexpr size_t count = 1 << 12; constexpr size_t outer_count = 12; @@ -39,7 +39,7 @@ void test_alloc_dealloc_64k() void test_random_allocation() { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); std::unordered_set allocated; constexpr size_t count = 10000; @@ -91,7 +91,7 @@ void test_random_allocation() void test_calloc() { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); for (size_t size = 16; size <= (1 << 24); size <<= 1) { @@ -162,7 +162,7 @@ void test_double_alloc() void test_external_pointer() { // Malloc does not have an external pointer querying mechanism. - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); for (uint8_t sc = 0; sc < NUM_SIZECLASSES; sc++) { @@ -208,7 +208,7 @@ void test_external_pointer_large() { xoroshiro::p128r64 r; - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); constexpr size_t count_log = snmalloc::bits::is64() ? 5 : 3; constexpr size_t count = 1 << count_log; @@ -244,7 +244,7 @@ void test_external_pointer_large() void test_external_pointer_dealloc_bug() { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); constexpr size_t count = (SUPERSLAB_SIZE / SLAB_SIZE) * 2; void* allocs[count]; @@ -268,7 +268,7 @@ void test_external_pointer_dealloc_bug() void test_alloc_16M() { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); // sizes >= 16M use large_alloc const size_t size = 16'000'000; @@ -279,7 +279,7 @@ void test_alloc_16M() void test_calloc_16M() { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); // sizes >= 16M use large_alloc const size_t size = 16'000'000; diff --git a/src/test/perf/contention/contention.cc b/src/test/perf/contention/contention.cc index 3c36634..a17999e 100644 --- a/src/test/perf/contention/contention.cc +++ b/src/test/perf/contention/contention.cc @@ -100,7 +100,7 @@ void test_tasks_f(size_t id) void test_tasks(size_t num_tasks, size_t count, size_t size) { - Alloc* a = ThreadAlloc::get(); + Alloc*& a = ThreadAlloc::get(); contention = new std::atomic[size]; xoroshiro::p128r32 r; diff --git a/src/test/perf/external_pointer/externalpointer.cc b/src/test/perf/external_pointer/externalpointer.cc index 4576be3..70d8149 100644 --- a/src/test/perf/external_pointer/externalpointer.cc +++ b/src/test/perf/external_pointer/externalpointer.cc @@ -43,7 +43,7 @@ namespace test void test_external_pointer(xoroshiro::p128r64& r) { - auto* alloc = ThreadAlloc::get(); + auto& alloc = ThreadAlloc::get(); setup(r, alloc); From 2efcddfc3dd69ce02530bc59979d1e17ac2ddcd6 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 14:20:24 +0100 Subject: [PATCH 09/11] Rework free list so that 0 is the placeholder. This is needed because in some configurations the constructor for the global placeholder is not called before the first allocation (i.e. when other globals call the allocator in their constructor) and so we ended up following a null pointer. --- src/ds/bits.h | 2 +- src/mem/alloc.h | 2 +- src/mem/metaslab.h | 10 +++++----- src/mem/slab.h | 10 ++++++---- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/ds/bits.h b/src/ds/bits.h index e6721cd..2503606 100644 --- a/src/ds/bits.h +++ b/src/ds/bits.h @@ -21,7 +21,7 @@ # include # define ALWAYSINLINE __attribute__((always_inline)) # define NOINLINE __attribute__((noinline)) -# define SNMALLOC_SLOW_PATH NOINLINE +# define SNMALLOC_SLOW_PATH NOINLINE __attribute__((section(".text,slow"))) # define SNMALLOC_FAST_PATH inline ALWAYSINLINE # define SNMALLOC_PURE __attribute__((const)) # ifdef __clang__ diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 94d209d..43e6ff7 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -1072,7 +1072,7 @@ namespace snmalloc assert(sizeclass < NUM_SMALL_CLASSES); auto& fl = small_fast_free_lists[sizeclass]; auto head = fl.value; - if (likely((reinterpret_cast(head) & 1) == 0)) + if (likely(head != nullptr)) { void* p = head; // Read the next slot from the memory that's about to be allocated. diff --git a/src/mem/metaslab.h b/src/mem/metaslab.h index a7a60b9..7295324 100644 --- a/src/mem/metaslab.h +++ b/src/mem/metaslab.h @@ -146,10 +146,10 @@ namespace snmalloc { #ifndef NDEBUG size_t length = 0; - void* curr = pointer_offset(slab, head); - void* curr_slow = pointer_offset(slab, head); + void* curr = (head == 1) ? nullptr : pointer_offset(slab, head); + void* curr_slow = (head == 1) ? nullptr : pointer_offset(slab, head); bool both = false; - while ((reinterpret_cast(curr) & 1) == 0) + while (curr != nullptr) { curr = follow_next(curr); if (both) @@ -200,8 +200,8 @@ namespace snmalloc UNUSED(length); // Walk bump-free-list-segment accounting for unused space - void* curr = pointer_offset(slab, head); - while ((address_cast(curr) & 1) == 0) + void* curr = (head == 1) ? nullptr : pointer_offset(slab, head); + while (curr != nullptr) { // Check we are looking at a correctly aligned block void* start = curr; diff --git a/src/mem/slab.h b/src/mem/slab.h index 24c4fe2..0845d7c 100644 --- a/src/mem/slab.h +++ b/src/mem/slab.h @@ -7,7 +7,7 @@ namespace snmalloc struct FreeListHead { // Use a value with bottom bit set for empty list. - void* value = pointer_offset(nullptr, 1); + void* value = nullptr; }; class Slab @@ -98,7 +98,8 @@ namespace snmalloc } else { - Metaslab::store_next(curr, pointer_offset(this, bumpptr)); + Metaslab::store_next( + curr, (bumpptr == 1) ? nullptr : pointer_offset(this, bumpptr)); } curr = pointer_offset(this, bumpptr); bumpptr = newbumpptr; @@ -106,7 +107,7 @@ namespace snmalloc } assert(curr != nullptr); - Metaslab::store_next(curr, pointer_offset(nullptr, 1)); + Metaslab::store_next(curr, nullptr); } } @@ -177,7 +178,8 @@ namespace snmalloc assert(meta.valid_head(is_short())); // Set the next pointer to the previous head. - Metaslab::store_next(p, pointer_offset(this, head)); + Metaslab::store_next( + p, (head == 1) ? nullptr : pointer_offset(this, head)); meta.debug_slab_invariant(is_short(), this); return true; } From 8d216dca3e2c98d1cd6922cdab91883bef74bc78 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 5 Jul 2019 17:01:35 +0100 Subject: [PATCH 10/11] Remove accidentally committed line. --- src/ds/bits.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ds/bits.h b/src/ds/bits.h index 2503606..e6721cd 100644 --- a/src/ds/bits.h +++ b/src/ds/bits.h @@ -21,7 +21,7 @@ # include # define ALWAYSINLINE __attribute__((always_inline)) # define NOINLINE __attribute__((noinline)) -# define SNMALLOC_SLOW_PATH NOINLINE __attribute__((section(".text,slow"))) +# define SNMALLOC_SLOW_PATH NOINLINE # define SNMALLOC_FAST_PATH inline ALWAYSINLINE # define SNMALLOC_PURE __attribute__((const)) # ifdef __clang__ From 3a0cdc05a5b8ccdefad8e8a402403f50f491ed29 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Mon, 8 Jul 2019 19:24:56 +0100 Subject: [PATCH 11/11] Fix on macOS. Fixes an issue where the global placeholder allocator was being released. --- src/mem/threadalloc.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/mem/threadalloc.h b/src/mem/threadalloc.h index ede455d..4ac6708 100644 --- a/src/mem/threadalloc.h +++ b/src/mem/threadalloc.h @@ -125,7 +125,11 @@ namespace snmalloc */ ~ThreadAllocThreadDestructor() { - current_alloc_pool()->release(alloc); + if (alloc != &GlobalPlaceHolder) + { + current_alloc_pool()->release(alloc); + alloc = &GlobalPlaceHolder; + } } public: @@ -172,8 +176,11 @@ namespace snmalloc *pp = nullptr; // Actually destroy the allocator and reset TLS Alloc*& alloc = ThreadAllocExplicitTLSCleanup::get(); - current_alloc_pool()->release(alloc); - alloc = &GlobalPlaceHolder; + if (alloc != &GlobalPlaceHolder) + { + current_alloc_pool()->release(alloc); + alloc = &GlobalPlaceHolder; + } } # ifdef _WIN32 @@ -314,6 +321,7 @@ namespace snmalloc return local_alloc; } local_alloc = current_alloc_pool()->acquire(); + assert(local_alloc != &GlobalPlaceHolder); ThreadAlloc::register_cleanup(); return local_alloc; }