From 2f25f89067d190ba201548bcb4caeefcb5bade8e Mon Sep 17 00:00:00 2001 From: Zygo Blaxell Date: Tue, 29 Nov 2022 22:06:54 -0500 Subject: [PATCH] task: get rid of separate Exclusion and ExclusionState Exclusion was generating a new Task every time a lock was contended. That results in thousands of empty Task objects which contain a single Task item. Get rid of ExclusionState. Exclusion is now a simple weak_ptr to a Task. If the weak_ptr is expired, the Exclusion is unlocked. If the weak_ptr is not expired, it points to the Task which owns the Exclusion. try_lock now appends the Task attempting to lock the Exclusion directly to the owning Task, eliminating the need for Exclusion to have one. This also removes the need to call insert_task separately, though insert_task remains for other use cases. With no ExclusionState there is no need for a string argument to Exclusion's constructor, so get rid of that too. Signed-off-by: Zygo Blaxell --- include/crucible/task.h | 53 +++++++++------------- lib/task.cc | 98 +++++++++-------------------------------- test/task.cc | 7 ++- 3 files changed, 45 insertions(+), 113 deletions(-) diff --git a/include/crucible/task.h b/include/crucible/task.h index 63feaa3..00ce6d3 100644 --- a/include/crucible/task.h +++ b/include/crucible/task.h @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -121,50 +122,40 @@ namespace crucible { void release(); }; - // Exclusion provides exclusive access to a ExclusionLock. - // One Task will be able to obtain the ExclusionLock; other Tasks - // may schedule themselves for re-execution after the ExclusionLock - // is released. - - class ExclusionState; - class Exclusion; - class ExclusionLock { - shared_ptr m_exclusion_state; - ExclusionLock(shared_ptr pes); - ExclusionLock() = default; + shared_ptr m_owner; + ExclusionLock(shared_ptr owner); friend class Exclusion; public: - // Calls release() - ~ExclusionLock(); + /// Explicit default constructor because we have other kinds + ExclusionLock() = default; - // Release this Lock immediately and permanently + /// Release this Lock immediately and permanently void release(); - // Test for locked state + /// Test for locked state operator bool() const; }; class Exclusion { - shared_ptr m_exclusion_state; + mutex m_mutex; + weak_ptr m_owner; - Exclusion(shared_ptr pes); public: - Exclusion(const string &title); + /// Attempt to obtain a Lock. If successful, current Task + /// owns the Lock until the ExclusionLock is released + /// (it is the ExclusionLock that owns the lock, so it can + /// be passed to other Tasks or threads, but this is not + /// recommended practice). + /// If not successful, current Task is appended to the + /// task that currently holds the lock. Current task is + /// expected to release any other ExclusionLock + /// objects it holds, and exit its Task function. + ExclusionLock try_lock(const Task &task); - // Attempt to obtain a Lock. If successful, current Task - // owns the Lock until the ExclusionLock is released - // (it is the ExclusionLock that owns the lock, so it can - // be passed to other Tasks or threads, but this is not - // recommended practice). - // If not successful, current Task is expected to call - // insert_task(current_task()), release any ExclusionLock - // objects it holds, and exit its Task function. - ExclusionLock try_lock(); - - // Execute Task when Exclusion is unlocked (possibly - // immediately). - void insert_task(Task t = Task::current_task()); + /// Execute Task when Exclusion is unlocked (possibly + /// immediately). + void insert_task(const Task &t); }; diff --git a/lib/task.cc b/lib/task.cc index 0a971f1..0c47d3f 100644 --- a/lib/task.cc +++ b/lib/task.cc @@ -827,110 +827,52 @@ namespace crucible { m_barrier_state.reset(); } - class ExclusionState { - mutex m_mutex; - bool m_locked = false; - Task m_task; - - public: - ExclusionState(const string &title); - ~ExclusionState(); - void release(); - bool try_lock(); - void insert_task(Task t); - }; - - Exclusion::Exclusion(shared_ptr pbs) : - m_exclusion_state(pbs) - { - } - - Exclusion::Exclusion(const string &title) : - m_exclusion_state(make_shared(title)) - { - } - - ExclusionState::ExclusionState(const string &title) : - m_task(title, [](){}) - { - } - - void - ExclusionState::release() - { - unique_lock lock(m_mutex); - m_locked = false; - m_task.run(); - } - - ExclusionState::~ExclusionState() - { - release(); - } - - ExclusionLock::ExclusionLock(shared_ptr pbs) : - m_exclusion_state(pbs) + ExclusionLock::ExclusionLock(shared_ptr owner) : + m_owner(owner) { } void ExclusionLock::release() { - if (m_exclusion_state) { - m_exclusion_state->release(); - m_exclusion_state.reset(); - } - } - - ExclusionLock::~ExclusionLock() - { - release(); + m_owner.reset(); } void - ExclusionState::insert_task(Task task) + Exclusion::insert_task(const Task &task) { unique_lock lock(m_mutex); - if (m_locked) { + const auto sp = m_owner.lock(); + lock.unlock(); + if (sp) { // If Exclusion is locked then queue task for release; - m_task.append(task); + sp->append(task); } else { // otherwise, run the inserted task immediately task.run(); } } - bool - ExclusionState::try_lock() + ExclusionLock + Exclusion::try_lock(const Task &task) { unique_lock lock(m_mutex); - if (m_locked) { - return false; + const auto sp = m_owner.lock(); + if (sp) { + if (task) { + sp->append(task); + } + return ExclusionLock(); } else { - m_locked = true; - return true; + const auto rv = make_shared(task); + m_owner = rv; + return ExclusionLock(rv); } } - void - Exclusion::insert_task(Task t) - { - m_exclusion_state->insert_task(t); - } - ExclusionLock::operator bool() const { - return !!m_exclusion_state; + return !!m_owner; } - ExclusionLock - Exclusion::try_lock() - { - THROW_CHECK0(runtime_error, m_exclusion_state); - if (m_exclusion_state->try_lock()) { - return ExclusionLock(m_exclusion_state); - } else { - return ExclusionLock(); - } - } } diff --git a/test/task.cc b/test/task.cc index ddc8db4..25c513a 100644 --- a/test/task.cc +++ b/test/task.cc @@ -157,7 +157,7 @@ void test_exclusion(size_t count) { mutex only_one; - auto excl = make_shared("test_excl"); + auto excl = make_shared(); mutex mtx; condition_variable cv; @@ -178,9 +178,8 @@ test_exclusion(size_t count) [c, &only_one, excl, &lock_success_count, &lock_failure_count, &pings, &tasks_running, &cv, &mtx]() mutable { // cerr << "Task #" << c << endl; (void)c; - auto lock = excl->try_lock(); + auto lock = excl->try_lock(Task::current_task()); if (!lock) { - excl->insert_task(Task::current_task()); ++lock_failure_count; return; } @@ -200,7 +199,7 @@ test_exclusion(size_t count) t.run(); } - // excl.reset(); + excl.reset(); unique_lock lock(mtx); while (tasks_running) {