From e63a683574417bbea69f07fd21226e37efacf89a Mon Sep 17 00:00:00 2001 From: Kevin Huck Date: Fri, 16 Mar 2018 16:08:44 -0700 Subject: [PATCH 1/6] Refactoring the APEX/HPX integration. Major changes: When an HPX thread is created, an apex_wrapper() call is made to create an apex::task_wrapper. That object can be updated later in the case of an annotated_function. The thread maintains a pointer to this wrapper, and passes it to the apex_wrapper calls to start/stop timers. Memory is managed by APEX, and the task_wrapper object is destroyed after apex::stop. --- hpx/runtime/threads/coroutines/coroutine.hpp | 6 +- .../coroutines/detail/context_base.hpp | 13 ++- .../coroutines/detail/coroutine_self.hpp | 7 +- .../threads/detail/scheduling_loop.hpp | 4 +- hpx/runtime/threads/thread_data.hpp | 9 +- hpx/runtime/threads/thread_data_fwd.hpp | 3 +- hpx/util/annotated_function.hpp | 18 ++-- hpx/util/apex.hpp | 84 ++++++++++++------- src/runtime/threads/thread_data.cpp | 9 +- 9 files changed, 103 insertions(+), 50 deletions(-) diff --git a/hpx/runtime/threads/coroutines/coroutine.hpp b/hpx/runtime/threads/coroutines/coroutine.hpp index 56a780c37624..27a21c404fdc 100644 --- a/hpx/runtime/threads/coroutines/coroutine.hpp +++ b/hpx/runtime/threads/coroutines/coroutine.hpp @@ -97,10 +97,14 @@ namespace hpx { namespace threads { namespace coroutines } #if defined(HPX_HAVE_APEX) - void** get_apex_data() const + void* get_apex_data() const { return impl_.get_apex_data(); } + void set_apex_data(void * data) + { + return impl_.set_apex_data(data); + } #endif void rebind(functor_type&& f, thread_id_type id) diff --git a/hpx/runtime/threads/coroutines/detail/context_base.hpp b/hpx/runtime/threads/coroutines/detail/context_base.hpp index 06d244a2b729..ce48ab8883ad 100644 --- a/hpx/runtime/threads/coroutines/detail/context_base.hpp +++ b/hpx/runtime/threads/coroutines/detail/context_base.hpp @@ -48,6 +48,9 @@ #include #include #include +#if defined(HPX_HAVE_APEX) +#include +#endif #include #include @@ -250,7 +253,7 @@ namespace hpx { namespace threads { namespace coroutines { namespace detail } #if defined(HPX_HAVE_APEX) - void** get_apex_data() const + void* get_apex_data() const { // APEX wants the ADDRESS of a location to store // data. This storage could be updated asynchronously, @@ -258,7 +261,11 @@ namespace hpx { namespace threads { namespace coroutines { namespace detail // to remember state for the HPX thread in-betweeen // calls to apex::start/stop/yield/resume(). // APEX will change the value pointed to by the address. - return const_cast(&m_apex_data); + return m_apex_data; + } + void set_apex_data(void * data) + { + m_apex_data = data; } #endif @@ -338,7 +345,7 @@ namespace hpx { namespace threads { namespace coroutines { namespace detail #endif HPX_ASSERT(m_thread_data == 0); #if defined(HPX_HAVE_APEX) - HPX_ASSERT(m_apex_data == 0ull); + m_apex_data = 0ull; #endif m_type_info = std::exception_ptr(); } diff --git a/hpx/runtime/threads/coroutines/detail/coroutine_self.hpp b/hpx/runtime/threads/coroutines/detail/coroutine_self.hpp index b5bb776088cc..541a29932e58 100644 --- a/hpx/runtime/threads/coroutines/detail/coroutine_self.hpp +++ b/hpx/runtime/threads/coroutines/detail/coroutine_self.hpp @@ -197,11 +197,16 @@ namespace hpx { namespace threads { namespace coroutines { namespace detail static HPX_EXPORT void reset_self(); #if defined(HPX_HAVE_APEX) - void** get_apex_data() const + void* get_apex_data(void) const { HPX_ASSERT(m_pimpl); return m_pimpl->get_apex_data(); } + void set_apex_data(void * data) + { + HPX_ASSERT(m_pimpl); + m_pimpl->set_apex_data(data); + } #endif private: diff --git a/hpx/runtime/threads/detail/scheduling_loop.hpp b/hpx/runtime/threads/detail/scheduling_loop.hpp index 5490d71d6552..4eb61c8fc559 100644 --- a/hpx/runtime/threads/detail/scheduling_loop.hpp +++ b/hpx/runtime/threads/detail/scheduling_loop.hpp @@ -354,7 +354,7 @@ namespace hpx { namespace threads { namespace detail if (HPX_LIKELY(thrd_stat.is_valid() && thrd_stat.get_previous() == pending)) { -#if defined(HPX_HAVE_APEX) +#if defined(HPX_HAVE_APEX_disabled) // get the APEX data pointer, in case we are resuming the // thread and have to restore any leaf timers from // direct actions, etc. @@ -362,7 +362,6 @@ namespace hpx { namespace threads { namespace detail // the address of tmp_data is getting stored // by APEX during this call util::apex_wrapper apex_profiler( - background_thread->get_description(), background_thread->get_apex_data()); thrd_stat = (*background_thread)(); @@ -526,7 +525,6 @@ namespace hpx { namespace threads { namespace detail // the address of tmp_data is getting stored // by APEX during this call util::apex_wrapper apex_profiler( - thrd->get_description(), thrd->get_apex_data()); thrd_stat = (*thrd)(); diff --git a/hpx/runtime/threads/thread_data.hpp b/hpx/runtime/threads/thread_data.hpp index cde62b384dfa..6e2a89633f6f 100644 --- a/hpx/runtime/threads/thread_data.hpp +++ b/hpx/runtime/threads/thread_data.hpp @@ -526,10 +526,14 @@ namespace hpx { namespace threads } #if defined(HPX_HAVE_APEX) - void** get_apex_data() const + void* get_apex_data() const { return coroutine_.get_apex_data(); } + void set_apex_data(void * data) + { + return coroutine_.set_apex_data(data); + } #endif void rebind(thread_init_data& init_data, @@ -598,6 +602,9 @@ namespace hpx { namespace threads } if (0 == parent_locality_id_) parent_locality_id_ = get_locality_id(); +#endif +#if defined(HPX_HAVE_APEX) + set_apex_data(apex_new_task(get_description())); #endif HPX_ASSERT(init_data.stacksize != 0); HPX_ASSERT(coroutine_.is_ready()); diff --git a/hpx/runtime/threads/thread_data_fwd.hpp b/hpx/runtime/threads/thread_data_fwd.hpp index a64b05c62eec..9981314dfacd 100644 --- a/hpx/runtime/threads/thread_data_fwd.hpp +++ b/hpx/runtime/threads/thread_data_fwd.hpp @@ -159,7 +159,8 @@ namespace hpx { namespace threads thread_state_enum state = unknown); #if defined(HPX_HAVE_APEX) - HPX_API_EXPORT void** get_self_apex_data(); + HPX_API_EXPORT void* get_self_apex_data(void); + HPX_API_EXPORT void set_self_apex_data(void * data); #endif }} diff --git a/hpx/util/annotated_function.hpp b/hpx/util/annotated_function.hpp index b61dba429839..75f9df57acf4 100644 --- a/hpx/util/annotated_function.hpp +++ b/hpx/util/annotated_function.hpp @@ -73,16 +73,18 @@ namespace hpx { namespace util HPX_NON_COPYABLE(annotate_function); explicit annotate_function(char const* name) - : apex_profiler_(name, threads::get_self_apex_data()) - {} + { + threads::set_self_apex_data( + apex_update_task(threads::get_self_apex_data(), + name)); + } template explicit annotate_function(F && f) - : apex_profiler_(hpx::util::thread_description(f), - threads::get_self_apex_data()) - {} - - private: - hpx::util::apex_wrapper apex_profiler_; + { + threads::set_self_apex_data( + apex_update_task(threads::get_self_apex_data(), + hpx::util::thread_description(f))); + } }; #else struct annotate_function diff --git a/hpx/util/apex.hpp b/hpx/util/apex.hpp index 27cafe484f1f..f411123ea19c 100644 --- a/hpx/util/apex.hpp +++ b/hpx/util/apex.hpp @@ -35,36 +35,49 @@ namespace hpx { namespace util apex::finalize(); } - struct apex_wrapper + inline void * apex_new_task( + thread_description const& description) { - apex_wrapper(thread_description const& name) - : name_(name), stopped(false) - { - if (name_.kind() == thread_description::data_type_description) - { - profiler_ = apex::start(name_.get_description()); - } - else - { - profiler_ = apex::start( - apex_function_address(name_.get_address())); - } + if (description.kind() == + thread_description::data_type_description) { + return (void*)apex::new_task(description.get_description()); + } else { + return (void*)apex::new_task(description.get_address()); } - apex_wrapper(thread_description const& name, void** const data_ptr) - : name_(name), stopped(false) + } + + inline void * apex_new_task(char const* name) + { + return (void*)apex::new_task(name); + } + + inline void * apex_update_task(void * wrapper, + thread_description const& description) + { + if (description.kind() == thread_description::data_type_description) { + return (void*)apex::update_task((apex::task_wrapper*)wrapper, + description.get_description()); + } else { + return (void*)apex::update_task((apex::task_wrapper*)wrapper, + description.get_address()); + } + } + + inline void * apex_update_task(void * wrapper, char const* name) + { + return (void*)apex::update_task((apex::task_wrapper*)wrapper, name); + } + + struct apex_wrapper + { + apex_wrapper(void* const data_ptr) : stopped(false), data_(nullptr) { - if (name_.kind() == thread_description::data_type_description) - { - // not a mistake... we need to pass in the address of this - // pointer, so that we can assign data to it in apex... - profiler_ = apex::start(name_.get_description(), data_ptr); - } - else - { - // not a mistake... we need to pass in the address of this - // pointer, so that we can assign data to it in apex... - profiler_ = apex::start( - apex_function_address(name_.get_address()), data_ptr); + /* APEX internal actions are not timed. Otherwise, we would + * end up with recursive timers. So it's possible to have + * a null task wrapper pointer here. */ + if (data_ptr != nullptr) { + data_ = (apex::task_wrapper*)data_ptr; + apex::start(data_); } } ~apex_wrapper() @@ -75,20 +88,29 @@ namespace hpx { namespace util void stop() { if(!stopped) { stopped = true; - apex::stop(profiler_); + /* APEX internal actions are not timed. Otherwise, we would + * end up with recursive timers. So it's possible to have + * a null task wrapper pointer here. */ + if (data_ != nullptr) { + apex::stop(data_); + } } } void yield() { if(!stopped) { stopped = true; - apex::yield(profiler_); + /* APEX internal actions are not timed. Otherwise, we would + * end up with recursive timers. So it's possible to have + * a null task wrapper pointer here. */ + if (data_ != nullptr) { + apex::yield(data_); + } } } - thread_description name_; bool stopped; - apex::profiler * profiler_; + apex::task_wrapper * data_; }; struct apex_wrapper_init diff --git a/src/runtime/threads/thread_data.cpp b/src/runtime/threads/thread_data.cpp index 21c2ab8d84f9..f8d0e9de0d9f 100644 --- a/src/runtime/threads/thread_data.cpp +++ b/src/runtime/threads/thread_data.cpp @@ -214,12 +214,19 @@ namespace hpx { namespace threads } #if defined(HPX_HAVE_APEX) - void** get_self_apex_data() + void* get_self_apex_data() { thread_self* self = get_self_ptr(); if (nullptr == self) return nullptr; return self->get_apex_data(); } + void set_self_apex_data(void* data) + { + thread_self* self = get_self_ptr(); + if (nullptr == self) + return; + self->set_apex_data(data); + } #endif }} From e45f5ba3c04714216f43f5abdf8af0041af99066 Mon Sep 17 00:00:00 2001 From: Kevin Huck Date: Fri, 16 Mar 2018 16:12:38 -0700 Subject: [PATCH 2/6] Removing the background_thread timer Removing the background thread timer because that work should be handled in the regular scheduling loop. --- .../threads/detail/scheduling_loop.hpp | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/hpx/runtime/threads/detail/scheduling_loop.hpp b/hpx/runtime/threads/detail/scheduling_loop.hpp index 4eb61c8fc559..c83f67b79c2a 100644 --- a/hpx/runtime/threads/detail/scheduling_loop.hpp +++ b/hpx/runtime/threads/detail/scheduling_loop.hpp @@ -354,29 +354,7 @@ namespace hpx { namespace threads { namespace detail if (HPX_LIKELY(thrd_stat.is_valid() && thrd_stat.get_previous() == pending)) { -#if defined(HPX_HAVE_APEX_disabled) - // get the APEX data pointer, in case we are resuming the - // thread and have to restore any leaf timers from - // direct actions, etc. - - // the address of tmp_data is getting stored - // by APEX during this call - util::apex_wrapper apex_profiler( - background_thread->get_apex_data()); - thrd_stat = (*background_thread)(); - - if (thrd_stat.get_previous() == terminated) - { - apex_profiler.stop(); - } - else - { - apex_profiler.yield(); - } -#else - thrd_stat = (*background_thread)(); -#endif thread_data* next = thrd_stat.get_next_thread(); if (next != nullptr && next != background_thread.get()) { From 81ade4645b98ccbc188b8a23330c5295614c34a4 Mon Sep 17 00:00:00 2001 From: Kevin Huck Date: Fri, 16 Mar 2018 16:15:07 -0700 Subject: [PATCH 3/6] Set the apex_data_ptr to nullptr after timers are stopped. --- hpx/runtime/threads/detail/scheduling_loop.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hpx/runtime/threads/detail/scheduling_loop.hpp b/hpx/runtime/threads/detail/scheduling_loop.hpp index c83f67b79c2a..f645bfd763ce 100644 --- a/hpx/runtime/threads/detail/scheduling_loop.hpp +++ b/hpx/runtime/threads/detail/scheduling_loop.hpp @@ -510,6 +510,8 @@ namespace hpx { namespace threads { namespace detail if (thrd_stat.get_previous() == terminated) { apex_profiler.stop(); + // just in case, clean up the now dead pointer. + thrd->set_apex_data(nullptr); } else { From c173edde4c0d296734858ca1ee92916c033d0993 Mon Sep 17 00:00:00 2001 From: Kevin Huck Date: Fri, 16 Mar 2018 16:18:16 -0700 Subject: [PATCH 4/6] Point to the refactored APEX repo branch. This should be updated to master before next release. --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 704a7eecc886..f1be37b260b6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1539,7 +1539,7 @@ if(HPX_WITH_APEX) include(GitExternal) git_external(apex https://github.com/khuck/xpress-apex.git - master + generate_guids ${_hpx_apex_no_update} VERBOSE) From 47a1fe2a947953e8e787cfbf2df0fa0703c3d2bb Mon Sep 17 00:00:00 2001 From: Kevin Huck Date: Sun, 18 Mar 2018 18:33:51 -0700 Subject: [PATCH 5/6] Updating APEX to v2.0. --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f1be37b260b6..a66d4d691040 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1539,7 +1539,7 @@ if(HPX_WITH_APEX) include(GitExternal) git_external(apex https://github.com/khuck/xpress-apex.git - generate_guids + v2.0 ${_hpx_apex_no_update} VERBOSE) From 1a30539cec4347ae28ace78a9a64287a444ba2e1 Mon Sep 17 00:00:00 2001 From: Kevin Huck Date: Tue, 20 Mar 2018 10:41:45 -0700 Subject: [PATCH 6/6] Fixing HPX tool complaints. --- hpx/util/annotated_function.hpp | 4 ++-- hpx/util/apex.hpp | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/hpx/util/annotated_function.hpp b/hpx/util/annotated_function.hpp index 75f9df57acf4..151de303da06 100644 --- a/hpx/util/annotated_function.hpp +++ b/hpx/util/annotated_function.hpp @@ -75,14 +75,14 @@ namespace hpx { namespace util explicit annotate_function(char const* name) { threads::set_self_apex_data( - apex_update_task(threads::get_self_apex_data(), + apex_update_task(threads::get_self_apex_data(), name)); } template explicit annotate_function(F && f) { threads::set_self_apex_data( - apex_update_task(threads::get_self_apex_data(), + apex_update_task(threads::get_self_apex_data(), hpx::util::thread_description(f))); } }; diff --git a/hpx/util/apex.hpp b/hpx/util/apex.hpp index f411123ea19c..4ccdd6f72f89 100644 --- a/hpx/util/apex.hpp +++ b/hpx/util/apex.hpp @@ -38,7 +38,7 @@ namespace hpx { namespace util inline void * apex_new_task( thread_description const& description) { - if (description.kind() == + if (description.kind() == thread_description::data_type_description) { return (void*)apex::new_task(description.get_description()); } else { @@ -51,14 +51,14 @@ namespace hpx { namespace util return (void*)apex::new_task(name); } - inline void * apex_update_task(void * wrapper, + inline void * apex_update_task(void * wrapper, thread_description const& description) { if (description.kind() == thread_description::data_type_description) { - return (void*)apex::update_task((apex::task_wrapper*)wrapper, + return (void*)apex::update_task((apex::task_wrapper*)wrapper, description.get_description()); } else { - return (void*)apex::update_task((apex::task_wrapper*)wrapper, + return (void*)apex::update_task((apex::task_wrapper*)wrapper, description.get_address()); } }