From 975d51d18ad3707a73e47bafd982eeb7ca89b8da Mon Sep 17 00:00:00 2001 From: FritzFlorian Date: Fri, 24 Apr 2020 14:28:00 +0200 Subject: [PATCH] Simplify: use raw pointers instead of optional wrappers on deque. --- lib/pls/include/pls/internal/scheduling/lock_free/external_trading_deque.h | 15 +++++++-------- lib/pls/src/internal/scheduling/lock_free/external_trading_deque.cpp | 32 ++++++++++++++++---------------- lib/pls/src/internal/scheduling/lock_free/task_manager.cpp | 21 ++++++++------------- test/scheduling_lock_free_tests.cpp | 18 +++++++++--------- 4 files changed, 40 insertions(+), 46 deletions(-) diff --git a/lib/pls/include/pls/internal/scheduling/lock_free/external_trading_deque.h b/lib/pls/include/pls/internal/scheduling/lock_free/external_trading_deque.h index 75b95e2..5ac2943 100644 --- a/lib/pls/include/pls/internal/scheduling/lock_free/external_trading_deque.h +++ b/lib/pls/include/pls/internal/scheduling/lock_free/external_trading_deque.h @@ -9,7 +9,6 @@ #include "pls/internal/base/error_handling.h" #include "pls/internal/base/system_details.h" -#include "pls/internal/data_structures/optional.h" #include "pls/internal/data_structures/stamped_integer.h" #include "pls/internal/scheduling/lock_free/task.h" @@ -38,8 +37,8 @@ class external_trading_deque { public: external_trading_deque(unsigned thread_id, size_t num_entries) : thread_id_(thread_id), entries_(num_entries) {} - static optional peek_traded_object(task *target_task); - static optional get_trade_object(task *target_task); + static task *peek_traded_object(task *target_task); + static task *get_trade_object(task *target_task); /** * Pushes a task on the bottom of the deque. @@ -54,12 +53,12 @@ class external_trading_deque { * * @return optional holding the popped task if successful. */ - optional pop_bot(); + task *pop_bot(); struct peek_result { - peek_result(optional top_task, stamped_integer top_pointer) : top_task_{std::move(top_task)}, - top_pointer_{top_pointer} {}; - optional top_task_; + peek_result(task *top_task, stamped_integer top_pointer) : top_task_{std::move(top_task)}, + top_pointer_{top_pointer} {}; + task *top_task_; stamped_integer top_pointer_; }; @@ -83,7 +82,7 @@ class external_trading_deque { * * @return optional holding the popped task if successful. */ - optional pop_top(task *offered_task, peek_result peek_result); + task *pop_top(task *offered_task, peek_result peek_result); private: void reset_bot_and_top(); diff --git a/lib/pls/src/internal/scheduling/lock_free/external_trading_deque.cpp b/lib/pls/src/internal/scheduling/lock_free/external_trading_deque.cpp index c2d322d..e3421c3 100644 --- a/lib/pls/src/internal/scheduling/lock_free/external_trading_deque.cpp +++ b/lib/pls/src/internal/scheduling/lock_free/external_trading_deque.cpp @@ -3,26 +3,26 @@ namespace pls::internal::scheduling::lock_free { -optional external_trading_deque::peek_traded_object(task *target_task) { +task * external_trading_deque::peek_traded_object(task *target_task) { traded_cas_field current_cas = target_task->external_trading_deque_cas_.load(); if (current_cas.is_filled_with_object()) { - return optional{current_cas.get_trade_object()}; + return current_cas.get_trade_object(); } else { - return optional{}; + return nullptr; } } -optional external_trading_deque::get_trade_object(task *target_task) { +task * external_trading_deque::get_trade_object(task *target_task) { traded_cas_field current_cas = target_task->external_trading_deque_cas_.load(); if (current_cas.is_filled_with_object()) { task *result = current_cas.get_trade_object(); traded_cas_field empty_cas; if (target_task->external_trading_deque_cas_.compare_exchange_strong(current_cas, empty_cas)) { - return optional{result}; + return result; } } - return optional{}; + return nullptr; } void external_trading_deque::push_bot(task *published_task) { @@ -58,10 +58,10 @@ void external_trading_deque::decrease_bot() { bot_.store(bot_internal_.value, std::memory_order_relaxed); } -optional external_trading_deque::pop_bot() { +task * external_trading_deque::pop_bot() { if (bot_internal_.value == 0) { reset_bot_and_top(); - return optional{}; + return nullptr; } decrease_bot(); @@ -77,10 +77,10 @@ optional external_trading_deque::pop_bot() { if (popped_task->external_trading_deque_cas_.compare_exchange_strong(expected_sync_cas_field, empty_cas_field, std::memory_order_acq_rel)) { - return optional{popped_task}; + return popped_task; } else { reset_bot_and_top(); - return optional{}; + return nullptr; } } @@ -89,17 +89,17 @@ external_trading_deque::peek_result external_trading_deque::peek_top() { auto local_bot = bot_.load(); if (local_top.value < local_bot) { - return peek_result{optional{entries_[local_top.value].traded_task_}, local_top}; + return peek_result{entries_[local_top.value].traded_task_, local_top}; } else { - return peek_result{optional{}, local_top}; + return peek_result{nullptr, local_top}; } } -optional external_trading_deque::pop_top(task *offered_task, peek_result peek_result) { +task * external_trading_deque::pop_top(task *offered_task, peek_result peek_result) { stamped_integer expected_top = peek_result.top_pointer_; auto local_bot = bot_.load(); if (expected_top.value >= local_bot) { - return data_structures::optional{}; + return nullptr; } auto &target_entry = entries_[expected_top.value]; @@ -119,7 +119,7 @@ optional external_trading_deque::pop_top(task *offered_task, peek_result // We got it, for sure move the top pointer forward. top_.compare_exchange_strong(expected_top, {expected_top.stamp + 1, expected_top.value + 1}); // Return the stolen task - return data_structures::optional{result}; + return result; } else { // We did not get it...help forwarding the top pointer anyway. if (expected_top.stamp == forwarding_stamp) { @@ -130,7 +130,7 @@ optional external_trading_deque::pop_top(task *offered_task, peek_result // This means only updating the tag, as this location can still hold data we need. top_.compare_exchange_strong(expected_top, {forwarding_stamp, expected_top.value}); } - return data_structures::optional{}; + return nullptr; } } diff --git a/lib/pls/src/internal/scheduling/lock_free/task_manager.cpp b/lib/pls/src/internal/scheduling/lock_free/task_manager.cpp index 4844477..b306bca 100644 --- a/lib/pls/src/internal/scheduling/lock_free/task_manager.cpp +++ b/lib/pls/src/internal/scheduling/lock_free/task_manager.cpp @@ -36,12 +36,7 @@ void task_manager::push_local_task(base_task *pushed_task) { } base_task *task_manager::pop_local_task() { - auto result = deque_.pop_bot(); - if (result) { - return *result; - } else { - return nullptr; - } + return deque_.pop_bot(); } std::tuple task_manager::steal_task(thread_state &stealing_state) { @@ -50,27 +45,27 @@ std::tuple task_manager::steal_task(thread_state &stea auto peek = deque_.peek_top(); if (peek.top_task_) { - task *stolen_task = static_cast(*peek.top_task_); + task *stolen_task = peek.top_task_; // get a suitable task to trade in // TODO: opt. add debug marker to traded in tasks that we do not accidentally use them. task *traded_task = static_cast(&scheduler::task_chain_at(stolen_task->depth_, stealing_state)); base_task *chain_after_stolen_task = traded_task->next_; // perform the actual pop operation - auto pop_result_task = deque_.pop_top(traded_task, peek); + task* pop_result_task = deque_.pop_top(traded_task, peek); if (pop_result_task) { PLS_ASSERT(stolen_task->thread_id_ != traded_task->thread_id_, "It is impossible to steal an task we already own!"); - PLS_ASSERT(*pop_result_task == stolen_task, + PLS_ASSERT(pop_result_task == stolen_task, "We must only steal the task that we peeked at!"); // update the resource stack associated with the stolen task stolen_task->push_task_chain(traded_task); - auto optional_exchanged_task = external_trading_deque::get_trade_object(stolen_task); + task* optional_exchanged_task = external_trading_deque::get_trade_object(stolen_task); if (optional_exchanged_task) { // All good, we pushed the task over to the stack, nothing more to do - PLS_ASSERT(*optional_exchanged_task == traded_task, + PLS_ASSERT(optional_exchanged_task == traded_task, "We are currently executing this, no one else can put another task in this field!"); } else { // The last other active thread took it as its spare resource... @@ -93,9 +88,9 @@ base_task *task_manager::pop_clean_task_chain(base_task *base_task) { task *clean_chain = popped_task->pop_task_chain(); if (clean_chain == nullptr) { // double-check if we are really last one or we only have unlucky timing - auto optional_cas_task = external_trading_deque::get_trade_object(popped_task); + task* optional_cas_task = external_trading_deque::get_trade_object(popped_task); if (optional_cas_task) { - clean_chain = *optional_cas_task; + clean_chain = optional_cas_task; } else { clean_chain = popped_task->pop_task_chain(); } diff --git a/test/scheduling_lock_free_tests.cpp b/test/scheduling_lock_free_tests.cpp index 04a9027..987553f 100644 --- a/test/scheduling_lock_free_tests.cpp +++ b/test/scheduling_lock_free_tests.cpp @@ -81,30 +81,30 @@ TEST_CASE("external trading deque", "[internal/scheduling/lock_free/external_tra // Local push/pop deque_1.push_bot(&tasks[0]); - REQUIRE(*deque_1.pop_bot() == &tasks[0]); + REQUIRE(deque_1.pop_bot() == &tasks[0]); REQUIRE(!deque_1.pop_bot()); // Local push, external pop deque_1.push_bot(&tasks[0]); auto peek = deque_1.peek_top(); - REQUIRE(*deque_1.pop_top(&tasks[1], peek) == &tasks[0]); - REQUIRE(*external_trading_deque::get_trade_object(&tasks[0]) == &tasks[1]); + REQUIRE(deque_1.pop_top(&tasks[1], peek) == &tasks[0]); + REQUIRE(external_trading_deque::get_trade_object(&tasks[0]) == &tasks[1]); REQUIRE(!deque_1.pop_top(&tasks[1], peek)); REQUIRE(!deque_1.pop_bot()); // Keeps push/pop order deque_1.push_bot(&tasks[0]); deque_1.push_bot(&tasks[1]); - REQUIRE(*deque_1.pop_bot() == &tasks[1]); - REQUIRE(*deque_1.pop_bot() == &tasks[0]); + REQUIRE(deque_1.pop_bot() == &tasks[1]); + REQUIRE(deque_1.pop_bot() == &tasks[0]); REQUIRE(!deque_1.pop_bot()); deque_1.push_bot(&tasks[0]); deque_1.push_bot(&tasks[1]); auto peek1 = deque_1.peek_top(); - REQUIRE(*deque_1.pop_top(&tasks[2], peek1) == &tasks[0]); + REQUIRE(deque_1.pop_top(&tasks[2], peek1) == &tasks[0]); auto peek2 = deque_1.peek_top(); - REQUIRE(*deque_1.pop_top(&tasks[3], peek2) == &tasks[1]); + REQUIRE(deque_1.pop_top(&tasks[3], peek2) == &tasks[1]); } SECTION("Interwined execution #1") { @@ -112,7 +112,7 @@ TEST_CASE("external trading deque", "[internal/scheduling/lock_free/external_tra deque_1.push_bot(&tasks[0]); auto peek1 = deque_1.peek_top(); auto peek2 = deque_1.peek_top(); - REQUIRE(*deque_1.pop_top(&tasks[1], peek1) == &tasks[0]); + REQUIRE(deque_1.pop_top(&tasks[1], peek1) == &tasks[0]); REQUIRE(!deque_1.pop_top(&tasks[2], peek2)); } @@ -120,7 +120,7 @@ TEST_CASE("external trading deque", "[internal/scheduling/lock_free/external_tra // Top and bottom access deque_1.push_bot(&tasks[0]); auto peek1 = deque_1.peek_top(); - REQUIRE(*deque_1.pop_bot() == &tasks[0]); + REQUIRE(deque_1.pop_bot() == &tasks[0]); REQUIRE(!deque_1.pop_top(&tasks[2], peek1)); } } -- libgit2 0.26.0