From 515cc2cba870d486e661ae5c635c03e6cc3cef59 Mon Sep 17 00:00:00 2001 From: "Steven R. Brandt" Date: Wed, 23 Nov 2016 09:55:36 -0600 Subject: [PATCH 1/3] Improve guards: 1) Add docs to header 2) Fix cleanup bug 3) Fewer async calls. --- hpx/lcos/local/composable_guard.hpp | 76 +++++++++++++++++++++++++++-- src/lcos/local/composable_guard.cpp | 53 ++++++++++++++------ 2 files changed, 111 insertions(+), 18 deletions(-) diff --git a/hpx/lcos/local/composable_guard.hpp b/hpx/lcos/local/composable_guard.hpp index 39d44aa289a7..0ab72ab2fec6 100644 --- a/hpx/lcos/local/composable_guard.hpp +++ b/hpx/lcos/local/composable_guard.hpp @@ -2,6 +2,70 @@ // // Distributed under the Boost Software License, Version 1.0. (See accompanying // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) +// +// How do guards work: +// Pythonesque pseudocode +// +// class guard: +// task # an atomic pointer to a guard_task +// +// class guard_task: +// run # a function pointer of some kind +// next # an atomic pointer to another guard task +// +// def run_guarded(g,func): +// n = new guard_task +// n.run = func +// t = g.task.exchange(n) +// if t == nullptr: +// run_task(n) +// else: +// zero = nullptr +// if t.next.compare_exchange_strong(zero,n): +// pass +// else: +// run_task(n) +// delete t +// +// def run_task(t): +// t.run() // call the task +// zero = nullptr +// if t.next.compare_exchange_strong(zero,t): +// pass +// else: +// run_task(zero) +// delete t +// +// Consider cases. Thread A, B, and C on guard g. +// Case 1: +// Thread A runs on guard g, gets t == nullptr and runs to completion. +// Thread B starts, gets t != null, compare_exchange_strong fails, it runs to completion and deletes t +// Thread C starts, gets t != null, compare_exchange_strong fails, it runs to completion and deletes t +// +// Case 2: +// Thread A runs on guard g, gets t == nullptr, but before it completes, thread B starts. +// Thread B runs on guard g, gets t != nullptr, compare_exchange_strong succeeds. It does nothing further. +// Thread A resumes and finishes, compare_exchange_strong fails, it runs B's task to completion. +// Thread C starts, gets t != null, compare_exchange_strong fails, it runs to completion and deletes t +// +// Case 3: +// Thread A runs on guard g, gets t == nullptr, but before it completes, thread B starts. +// Thread B runs on guard g, gets t != nullptr, compare_exchange_strong succeeds, It does nothing further. +// Thread C runs on guard g, gets t != nullptr, compare_exchange_strong succeeds, It does nothing further. +// Thread A resumes and finishes, compare_exchange_strong fails, it runs B's task to completion. +// Thread B does compare_exchange_strong fails, it runs C's task to completion. +// +// def destructor guard: +// t = g.load() +// if t == nullptr: +// pass +// else: +// zero = nullptr +// if t.next.compare_exchange_strong(zero,empty): +// pass +// else: +// delete t + #ifndef HPX_LCOS_LOCAL_COMPOSABLE_GUARD_HPP #define HPX_LCOS_LOCAL_COMPOSABLE_GUARD_HPP @@ -10,6 +74,7 @@ #include #include #include +#include #include @@ -62,9 +127,7 @@ namespace hpx { namespace lcos { namespace local detail::guard_atomic task; guard() : task(nullptr) {} - ~guard() { - detail::free(task.load()); - } + HPX_API_EXPORT ~guard(); }; class guard_set : public detail::debug_object @@ -78,15 +141,20 @@ namespace hpx { namespace lcos { namespace local public: guard_set() : guards(), sorted(true) {} - ~guard_set() {} + ~guard_set() {} std::shared_ptr get(std::size_t i) { return guards[i]; } void add(std::shared_ptr const& guard_ptr) { + HPX_ASSERT(guard_ptr.get() != nullptr); guards.push_back(guard_ptr); sorted = false; } + std::size_t size() { + return guards.size(); + } + friend HPX_API_EXPORT void run_guarded( guard_set& guards, detail::guard_function task); }; diff --git a/src/lcos/local/composable_guard.cpp b/src/lcos/local/composable_guard.cpp index b208d82d9e7e..43f511ace499 100644 --- a/src/lcos/local/composable_guard.cpp +++ b/src/lcos/local/composable_guard.cpp @@ -3,14 +3,14 @@ // Distributed under the Boost Software License, Version 1.0. (See accompanying // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) -#include - #include #include #include #include #include +#include + #include #include @@ -24,6 +24,8 @@ namespace hpx { namespace lcos { namespace local static void run_composable(detail::guard_task* task); static void run_async(detail::guard_task* task); + static void nothing() {} + namespace detail { // A link in the list of tasks attached @@ -35,9 +37,9 @@ namespace hpx { namespace lcos { namespace local bool const single_guard; guard_task() - : next(nullptr), run(nullptr), single_guard(true) {} + : next(nullptr), run(nothing), single_guard(true) {} guard_task(bool sg) - : next(nullptr), run(nullptr), single_guard(sg) {} + : next(nullptr), run(nothing), single_guard(sg) {} }; void free(guard_task* task) @@ -62,20 +64,25 @@ namespace hpx { namespace lcos { namespace local { guard_set gs; detail::guard_function task; + std::size_t n; detail::guard_task** stages; stage_data(detail::guard_function task_, std::vector >& guards) - : task(std::move(task_)) - , stages(new detail::guard_task*[guards.size()]) + : gs() + , task(std::move(task_)) + , n(guards.size()) + , stages(new detail::guard_task*[n]) { - std::size_t const n = guards.size(); for (std::size_t i=0; icheck(); detail::guard_task* zero = nullptr; if (!prev->next.compare_exchange_strong(zero, task)) { - run_async(task); + run_composable(task); free(prev); } } else { @@ -116,7 +123,7 @@ namespace hpx { namespace lcos { namespace local zero = nullptr; if (!lt->next.compare_exchange_strong(zero, lt)) { HPX_ASSERT(zero != lt); - run_async(zero); + run_composable(zero); free(lt); } } @@ -190,22 +197,40 @@ namespace hpx { namespace lcos { namespace local HPX_ASSERT(task != nullptr); task->check(); if (!task->next.compare_exchange_strong(zero, task)) { - HPX_ASSERT(task->next.load()!=nullptr); - run_async(zero); + HPX_ASSERT(zero != nullptr); + run_composable(zero); free(task); } } }; + using hpx::lcos::local::detail::guard_task; + guard_task *empty = new guard_task; + static void run_composable(detail::guard_task* task) { + if(task == empty) + return; HPX_ASSERT(task != nullptr); task->check(); if (task->single_guard) { - run_composable_cleanup rcc(task); - task->run(); + run_composable_cleanup rcc(task); + task->run(); } else { - task->run(); + task->run(); + // Note that by this point in the execution + // the task data structure has probably + // been deleted. + } + } + + guard::~guard() { + guard_task *zero = nullptr; + guard_task *current = task.load(); + if(current == nullptr) + return; + if(!current->next.compare_exchange_strong(zero,empty)) { + free(zero); } } }}} From f271edac1ae34f5def99a66cfb2f6c24eac7809f Mon Sep 17 00:00:00 2001 From: "Steven R. Brandt" Date: Wed, 23 Nov 2016 10:43:50 -0600 Subject: [PATCH 2/3] Fix indents --- hpx/lcos/local/composable_guard.hpp | 2 +- src/lcos/local/composable_guard.cpp | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/hpx/lcos/local/composable_guard.hpp b/hpx/lcos/local/composable_guard.hpp index 0ab72ab2fec6..7bc0d8fd0f5d 100644 --- a/hpx/lcos/local/composable_guard.hpp +++ b/hpx/lcos/local/composable_guard.hpp @@ -152,7 +152,7 @@ namespace hpx { namespace lcos { namespace local } std::size_t size() { - return guards.size(); + return guards.size(); } friend HPX_API_EXPORT void run_guarded( diff --git a/src/lcos/local/composable_guard.cpp b/src/lcos/local/composable_guard.cpp index 43f511ace499..2b640f6f05df 100644 --- a/src/lcos/local/composable_guard.cpp +++ b/src/lcos/local/composable_guard.cpp @@ -81,7 +81,7 @@ namespace hpx { namespace lcos { namespace local ~stage_data() { if(stages == nullptr) - abort(); + abort(); HPX_ASSERT(n == gs.size()); delete[] stages; stages = nullptr; @@ -210,17 +210,17 @@ namespace hpx { namespace lcos { namespace local static void run_composable(detail::guard_task* task) { if(task == empty) - return; + return; HPX_ASSERT(task != nullptr); task->check(); if (task->single_guard) { - run_composable_cleanup rcc(task); - task->run(); + run_composable_cleanup rcc(task); + task->run(); } else { - task->run(); - // Note that by this point in the execution - // the task data structure has probably - // been deleted. + task->run(); + // Note that by this point in the execution + // the task data structure has probably + // been deleted. } } @@ -228,7 +228,7 @@ namespace hpx { namespace lcos { namespace local guard_task *zero = nullptr; guard_task *current = task.load(); if(current == nullptr) - return; + return; if(!current->next.compare_exchange_strong(zero,empty)) { free(zero); } From a575b3cf8dc7c8dcc0c507e0a4660bc4fb3ee3f4 Mon Sep 17 00:00:00 2001 From: "Steven R. Brandt" Date: Mon, 28 Nov 2016 11:19:22 -0600 Subject: [PATCH 3/3] Fix whitespace and line length issues --- hpx/lcos/local/composable_guard.hpp | 41 ++++++++++++++++++----------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/hpx/lcos/local/composable_guard.hpp b/hpx/lcos/local/composable_guard.hpp index 7bc0d8fd0f5d..5b8c7edf91a7 100644 --- a/hpx/lcos/local/composable_guard.hpp +++ b/hpx/lcos/local/composable_guard.hpp @@ -2,7 +2,7 @@ // // Distributed under the Boost Software License, Version 1.0. (See accompanying // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) -// +// // How do guards work: // Pythonesque pseudocode // @@ -26,35 +26,46 @@ // else: // run_task(n) // delete t -// +// // def run_task(t): // t.run() // call the task // zero = nullptr // if t.next.compare_exchange_strong(zero,t): // pass -// else: +// else: // run_task(zero) // delete t // // Consider cases. Thread A, B, and C on guard g. // Case 1: // Thread A runs on guard g, gets t == nullptr and runs to completion. -// Thread B starts, gets t != null, compare_exchange_strong fails, it runs to completion and deletes t -// Thread C starts, gets t != null, compare_exchange_strong fails, it runs to completion and deletes t +// Thread B starts, gets t != null, compare_exchange_strong fails, +// it runs to completion and deletes t +// Thread C starts, gets t != null, compare_exchange_strong fails, +// it runs to completion and deletes t // // Case 2: -// Thread A runs on guard g, gets t == nullptr, but before it completes, thread B starts. -// Thread B runs on guard g, gets t != nullptr, compare_exchange_strong succeeds. It does nothing further. -// Thread A resumes and finishes, compare_exchange_strong fails, it runs B's task to completion. -// Thread C starts, gets t != null, compare_exchange_strong fails, it runs to completion and deletes t +// Thread A runs on guard g, gets t == nullptr, +// but before it completes, thread B starts. +// Thread B runs on guard g, gets t != nullptr, +// compare_exchange_strong succeeds. It does nothing further. +// Thread A resumes and finishes, compare_exchange_strong fails, +// it runs B's task to completion. +// Thread C starts, gets t != null, compare_exchange_strong fails, +// it runs to completion and deletes t // // Case 3: -// Thread A runs on guard g, gets t == nullptr, but before it completes, thread B starts. -// Thread B runs on guard g, gets t != nullptr, compare_exchange_strong succeeds, It does nothing further. -// Thread C runs on guard g, gets t != nullptr, compare_exchange_strong succeeds, It does nothing further. -// Thread A resumes and finishes, compare_exchange_strong fails, it runs B's task to completion. -// Thread B does compare_exchange_strong fails, it runs C's task to completion. -// +// Thread A runs on guard g, gets t == nullptr, +// but before it completes, thread B starts. +// Thread B runs on guard g, gets t != nullptr, +// compare_exchange_strong succeeds, It does nothing further. +// Thread C runs on guard g, gets t != nullptr, +// compare_exchange_strong succeeds, It does nothing further. +// Thread A resumes and finishes, compare_exchange_strong fails, +// it runs B's task to completion. +// Thread B does compare_exchange_strong fails, +// it runs C's task to completion. +// // def destructor guard: // t = g.load() // if t == nullptr: