From db8bd0b9a2e6abf98060307ae46699403187b3a3 Mon Sep 17 00:00:00 2001 From: FritzFlorian Date: Tue, 19 Mar 2019 19:33:15 +0100 Subject: [PATCH] Fix: change thread impelmentation to point to the correct memory region. The local storage per thread is only a pointer, this must be assigned AFTER the local storage object has moved to it's final location (e.g. on the stack of the caller). We might even change this a little further in the future to make the location of the state object more transparent. --- lib/pls/CMakeLists.txt | 7 +++++-- lib/pls/include/pls/internal/base/choose_threading.h | 13 +++++++++++++ lib/pls/include/pls/internal/base/spin_lock.h | 34 ++++++++++++++++++++++++++++++++++ lib/pls/include/pls/internal/base/thread.h | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------- lib/pls/src/internal/base/spin_lock.cpp | 9 +++++++++ lib/pls/src/internal/base/thread.cpp | 15 +++++++++++++++ test/thread_tests.cpp | 13 ++++++++----- 7 files changed, 170 insertions(+), 75 deletions(-) create mode 100644 lib/pls/include/pls/internal/base/choose_threading.h create mode 100644 lib/pls/include/pls/internal/base/spin_lock.h create mode 100644 lib/pls/src/internal/base/spin_lock.cpp diff --git a/lib/pls/CMakeLists.txt b/lib/pls/CMakeLists.txt index c6684e2..3686507 100644 --- a/lib/pls/CMakeLists.txt +++ b/lib/pls/CMakeLists.txt @@ -1,7 +1,10 @@ # List all required files here (cmake best practice to NOT automate this step!) add_library(pls STATIC src/library.cpp include/pls/library.h - src/internal/base/thread.cpp include/pls/internal/base/thread.h) + include/pls/internal/base/choose_threading.h + src/internal/base/spin_lock.cpp include/pls/internal/base/spin_lock.h + src/internal/base/thread.cpp include/pls/internal/base/thread.h + ) # Settings for our project... # ...pthreads or C++ 11 threads @@ -46,4 +49,4 @@ target_compile_options(pls PRIVATE $<$,$,$>: -Wall> $<$: - -W4>) \ No newline at end of file + -W4>) diff --git a/lib/pls/include/pls/internal/base/choose_threading.h b/lib/pls/include/pls/internal/base/choose_threading.h new file mode 100644 index 0000000..05124d7 --- /dev/null +++ b/lib/pls/include/pls/internal/base/choose_threading.h @@ -0,0 +1,13 @@ +// Make sure exactly ONE threading library is active for +// all of our threading primitives +#ifndef PLS_CHOOSE_THREADING_H +#define PLS_CHOOSE_THREADING_H + +#if defined(PLS_USING_PTHREADS) && defined(PLS_USING_CPP_THREADS) + #error "Please activate exactly one threading library (currently both are activated)!" +#endif +#if !defined(PLS_USING_PTHREADS) && !defined(PLS_USING_CPP_THREADS) + #error "Please activate exactly one threading library (currently none are activated)!" +#endif + +#endif //PLS_CHOOSE_THREADING_H diff --git a/lib/pls/include/pls/internal/base/spin_lock.h b/lib/pls/include/pls/internal/base/spin_lock.h new file mode 100644 index 0000000..4105e1c --- /dev/null +++ b/lib/pls/include/pls/internal/base/spin_lock.h @@ -0,0 +1,34 @@ + +#ifndef PLS_SPINLOCK_H +#define PLS_SPINLOCK_H + +#include + +#include "pls/internal/base/thread.h" + +namespace pls { + namespace internal { + namespace base { + class spin_lock { + std::atomic_flag flag_; + int yield_at_tries_; + + + public: + spin_lock(): flag_{ATOMIC_FLAG_INIT}, yield_at_tries_{1024} {}; + + void lock() { + int tries = 0; + while (flag_.test_and_set(std::memory_order_acquire)) { + tries++; + if (tries % yield_at_tries_ == 0) { + this_thread::yield(); + } + } + } + }; + } + } +} + +#endif //PLS_SPINLOCK_H diff --git a/lib/pls/include/pls/internal/base/thread.h b/lib/pls/include/pls/internal/base/thread.h index 3eaa540..b641a3a 100644 --- a/lib/pls/include/pls/internal/base/thread.h +++ b/lib/pls/include/pls/internal/base/thread.h @@ -6,13 +6,16 @@ #ifndef PLS_THREAD_H #define PLS_THREAD_H +#include + // platform specific includes +#include "pls/internal/base/choose_threading.h" #ifdef PLS_USING_PTHREADS #include +#include + #elif PLS_USING_CPP_THREADS #include -#else - #error "Please configure exactly one threading library!" #endif namespace pls { @@ -20,91 +23,110 @@ namespace pls { namespace base { using thread_entrypoint = void(); - // Thread local storage support + class this_thread { + template + friend class thread; #ifdef PLS_USING_PTHREADS - pthread_key_t thr_id_key; - bool thr_id_key_created = false; - // forward declaration - template - void* start_pthread_internal(void*); + static pthread_key_t local_storage_key_; + static bool local_storage_key_initialized_; #endif #ifdef PLS_USING_CPP_THREADS - thread_local void* local_thread; + static thread_local void* local_storage_; #endif - - template - class thread { - private: - // Handle to the native thread used + public: + static void yield() { #ifdef PLS_USING_PTHREADS - friend void* start_pthread_internal(void*); - - pthread_t pthread_thread_; - thread_entrypoint* entry_function_; - - thread(thread_entrypoint entry_function, T local_object): - pthread_thread_(), - entry_function_(entry_function), - local_object_(local_object) { - if (!thr_id_key_created) { - pthread_key_create(&thr_id_key, nullptr); - thr_id_key_created = true; - } - - pthread_create(&pthread_thread_, nullptr, start_pthread_internal, (void *)(this)); - } + pthread_yield(); #endif #ifdef PLS_USING_CPP_THREADS - std::thread std_thread_; + std::this_thread::yield(); +#endif + } - thread(thread_entrypoint entry_function, T local_object): - local_object_(local_object), - std_thread_([=](){ - local_thread = this; - entry_function(); - }) {}; + template + static T* state() { +#ifdef PLS_USING_PTHREADS + return reinterpret_cast(pthread_getspecific(local_storage_key_)); +#endif +#ifdef PLS_USING_CPP_THREADS + reinterpret_cast(local_storage_); #endif - public: - T local_object_; - - /** - * Creates and starts a thread. - * NOT thread safe, best only use from one main thread managing the runtime! - * - * @param entry_function The entry function to run on the thread - * @param T local_object - * - * @return a handle to the newly created thread - */ - static thread start(thread_entrypoint entry_function, T local_object) { - return thread(entry_function, local_object); } - void join() { + template + static void set_state(const T& state) { #ifdef PLS_USING_PTHREADS - pthread_join(pthread_thread_, nullptr); + *reinterpret_cast(pthread_getspecific(local_storage_key_)) = state; #endif #ifdef PLS_USING_CPP_THREADS - std_thread_.join(); + *reinterpret_cast(local_storage_) = state; #endif } + }; - static void yield() { + template + class thread { + friend class this_thread; + // Keep a copy of the function (lambda) in this object to make sure it is valid when called! + Function function_; + // Keep the local state we hold in here + State state_; + + // Keep handle to native implementation #ifdef PLS_USING_PTHREADS - pthread_yield(); + pthread_t pthread_thread_; #endif #ifdef PLS_USING_CPP_THREADS - std::this_thread::yield(); + std::thread std_thread_; #endif + +#ifdef PLS_USING_PTHREADS + static void* start_pthread_internal(void* thread_pointer) { + auto my_thread = reinterpret_cast(thread_pointer); + pthread_setspecific(this_thread::local_storage_key_, (void*)&my_thread->state_); + my_thread->function_(); + pthread_exit(nullptr); } - static thread* get_current() { +#endif + + public: #ifdef PLS_USING_PTHREADS - return (thread*) pthread_getspecific(thr_id_key); + explicit thread(const Function& function, const State& state): + function_{function}, + state_{state}, + pthread_thread_{} {} + + void start() { + if (!this_thread::local_storage_key_initialized_) { + pthread_key_create(&this_thread::local_storage_key_, nullptr); + this_thread::local_storage_key_initialized_ = true; + } + + pthread_create(&pthread_thread_, nullptr, start_pthread_internal, (void *)(this)); + } #endif #ifdef PLS_USING_CPP_THREADS - return (thread*) local_thread; + explicit thread(const Function& function, const State& state): + function_{function}, + state_{state}, + std_thread_{0} {}; + + void start() { + std_thread_ = std::thread([=](){ + local_storage = reinterpret_cast<&this->state_>; + this->function_(); + }); + } +#endif + public: + void join() { +#ifdef PLS_USING_PTHREADS + pthread_join(pthread_thread_, nullptr); +#endif +#ifdef PLS_USING_CPP_THREADS + std_thread_.join(); #endif } @@ -116,15 +138,11 @@ namespace pls { thread& operator=(const thread&) = delete; }; -#ifdef PLS_USING_PTHREADS - template - void* start_pthread_internal(void* thread_pointer) { - auto* my_thread = (thread*)thread_pointer; - pthread_setspecific(thr_id_key, thread_pointer); - my_thread->entry_function_(); - pthread_exit(nullptr); + template + thread create_thread(const Function& function, const State& state) { + return thread(function, state); } -#endif + } } diff --git a/lib/pls/src/internal/base/spin_lock.cpp b/lib/pls/src/internal/base/spin_lock.cpp new file mode 100644 index 0000000..c0b06f7 --- /dev/null +++ b/lib/pls/src/internal/base/spin_lock.cpp @@ -0,0 +1,9 @@ +#include "pls/internal/base/spin_lock.h" + +namespace pls { + namespace internal { + namespace base { + // implementation in header (inlining) + } + } +} diff --git a/lib/pls/src/internal/base/thread.cpp b/lib/pls/src/internal/base/thread.cpp index 2b1f521..4cc07d9 100644 --- a/lib/pls/src/internal/base/thread.cpp +++ b/lib/pls/src/internal/base/thread.cpp @@ -1 +1,16 @@ #include "pls/internal/base/thread.h" + +namespace pls { + namespace internal { + namespace base { +#ifdef PLS_USING_PTHREADS + bool this_thread::local_storage_key_initialized_ = false; + pthread_key_t this_thread::local_storage_key_; +#endif +#ifdef PLS_USING_CPP_THREADS + thread_local void* this_thread::local_storage_; +#endif + // implementation in header (C++ templating) + } + } +} diff --git a/test/thread_tests.cpp b/test/thread_tests.cpp index 51551bb..06fe9db 100644 --- a/test/thread_tests.cpp +++ b/test/thread_tests.cpp @@ -1,7 +1,7 @@ #include #include -#include +#include using namespace pls::internal::base; using namespace std; @@ -10,15 +10,18 @@ static bool visited; TEST_CASE( "thread creation and joining", "[internal/base/thread.h]") { visited = false; - auto t1 = thread::start([]() { visited = true; }, 0); + auto t1 = create_thread([]() { visited = true; }, 0); + t1.start(); t1.join(); REQUIRE(visited); } TEST_CASE( "thread state", "[internal/base/thread.h]") { - auto t1 = thread::start([]() { REQUIRE(thread::get_current()->local_object_ == 1); }, 1); - auto t2 = thread::start([]() { REQUIRE(thread::get_current()->local_object_ == "Hello"); }, "Hello"); + auto t1 = create_thread([]() { REQUIRE(*this_thread::state() == 1); }, 1); + auto t2 = create_thread([]() { REQUIRE(*this_thread::state>() == vector{1, 2}); }, vector{1, 2}); + t1.start(); + t2.start(); t1.join(); t2.join(); -} \ No newline at end of file +} -- libgit2 0.26.0