From 54e88e691499eb71fc60cb2ac2dd9a131c6aa5ad Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Fri, 26 Dec 2008 01:50:32 +0100 Subject: [PATCH] Refactoring III: Recursive/Nonrecursive and Waitable as policy classes pass test again --- src/lib/allocationcluster.cpp | 12 +- src/lib/allocationcluster.hpp | 2 +- src/lib/singletonpolicies.hpp | 2 +- src/lib/sync.hpp | 137 +++++++++++----------- src/lib/visitordispatcher.hpp | 4 +- src/proc/assetmanager.cpp | 2 +- src/proc/mobject/session/defsregistry.hpp | 2 +- tests/lib/sync-locking-test.cpp | 2 +- tests/lib/sync-waiting-test.cpp | 2 +- 9 files changed, 82 insertions(+), 83 deletions(-) diff --git a/src/lib/allocationcluster.cpp b/src/lib/allocationcluster.cpp index b5eb022ef..4dafb7d48 100644 --- a/src/lib/allocationcluster.cpp +++ b/src/lib/allocationcluster.cpp @@ -81,7 +81,7 @@ namespace lib { void AllocationCluster::MemoryManager::reset (TypeInfo info) { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); if (0 < mem_.size()) purge(); type_ = info; @@ -96,7 +96,7 @@ namespace lib { void AllocationCluster::MemoryManager::purge() { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); REQUIRE (type_.killIt, "we need a deleter function"); REQUIRE (0 < type_.allocSize, "allocation size unknown"); @@ -120,7 +120,7 @@ namespace lib { inline void* AllocationCluster::MemoryManager::allocate() { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); REQUIRE (0 < type_.allocSize); REQUIRE (top_ <= mem_.size()); @@ -140,7 +140,7 @@ namespace lib { inline void AllocationCluster::MemoryManager::commit (void* pendingAlloc) { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); REQUIRE (pendingAlloc); ASSERT (top_ < mem_.size()); @@ -175,7 +175,7 @@ namespace lib { { try { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); TRACE (memory, "shutting down AllocationCluster"); for (size_t i = typeHandlers_.size(); 0 < i; --i) @@ -214,7 +214,7 @@ namespace lib { ASSERT (0 < slot); { - Sync::ClassLock guard(); /////TODO: decide tradeoff: lock just the instance, or lock the AllocationCluster class? + Sync<>::ClassLock guard(); /////TODO: decide tradeoff: lock just the instance, or lock the AllocationCluster class? if (slot > typeHandlers_.size()) typeHandlers_.resize(slot); diff --git a/src/lib/allocationcluster.hpp b/src/lib/allocationcluster.hpp index 1e63732cc..51200e740 100644 --- a/src/lib/allocationcluster.hpp +++ b/src/lib/allocationcluster.hpp @@ -222,7 +222,7 @@ namespace lib { static TypeInfo setup() { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); if (!id_) id_= ++maxTypeIDs; diff --git a/src/lib/singletonpolicies.hpp b/src/lib/singletonpolicies.hpp index d0bade0e1..83c76f5ca 100644 --- a/src/lib/singletonpolicies.hpp +++ b/src/lib/singletonpolicies.hpp @@ -139,7 +139,7 @@ namespace lumiera struct Multithreaded { typedef volatile S VolatileType; - typedef lib::Sync::ClassLock Lock; + typedef lib::Sync<>::ClassLock Lock; }; diff --git a/src/lib/sync.hpp b/src/lib/sync.hpp index 4e295909a..b0a9df32a 100644 --- a/src/lib/sync.hpp +++ b/src/lib/sync.hpp @@ -121,6 +121,12 @@ namespace lib { class Mutex : protected MTX { + protected: + using MTX::mutex; + using MTX::__may_block; + using MTX::__enter; + using MTX::__leave; + public: void acquire() @@ -151,6 +157,10 @@ namespace lib { class Condition : public Mutex { + protected: + using CDX::cond; + using CDX::mutex; + public: void signal (bool wakeAll=false) @@ -162,9 +172,9 @@ namespace lib { } - template + template bool - wait (BF& predicate, MTX& mtx, Timeout& waitEndTime) + wait (BF& predicate, Timeout& waitEndTime) { int err=0; while (!predicate() && !err) @@ -210,32 +220,8 @@ namespace lib { operator bool() { return 0 != tv_sec; } // allows if (timeout_).... }; - typedef volatile bool& Flag; - class Monitor - : Condition /////////TODO: to be refactored. This is just one example, using the non-recursive condition - { - Timeout timeout_; - - //////TODO my intention is to make two variants of the monitor, where the simple one leaves out the condition part - - public: - Monitor() {} - ~Monitor() {} - - void acquireLock() { acquire(); } - void releaseLock() { release(); } - - void signal(bool a){ signal(a); } - - inline bool wait (Flag, ulong); - inline void setTimeout(ulong); - inline bool isTimedWait(); - }; - - - struct BoolFlagPredicate { Flag flag_; @@ -244,25 +230,48 @@ namespace lib { bool operator() () { return flag_; } }; + + template + class Monitor + : IMPL + { + Timeout timeout_; + + public: + Monitor() {} + ~Monitor() {} + + void acquireLock() { IMPL::acquire(); } + void releaseLock() { IMPL::release(); } + + void signal(bool a){ IMPL::signal(a); } + + bool wait (Flag flag, ulong timedwait=0) + { + BoolFlagPredicate checkFlag(flag); + return IMPL::wait(checkFlag, timeout_.setOffset(timedwait)); + } + + void setTimeout(ulong relative) {timeout_.setOffset(relative);} + bool isTimedWait() {return (timeout_);} + }; - bool - Monitor::wait (Flag flag, ulong timedwait) - { - BoolFlagPredicate checkFlag(flag); - return cond_.wait(checkFlag, mtx_, timeout_.setOffset(timedwait)); - } - - void - Monitor::setTimeout (ulong relative) {timeout_.setOffset(relative);} - - bool - Monitor::isTimedWait () {return (timeout_);} + typedef Mutex NonrecursiveLock_NoWait; + typedef Mutex RecursiveLock_NoWait; + typedef Condition NonrecursiveLock_Waitable; + typedef Condition RecursiveLock_Waitable; } // namespace sync (helpers and building blocks) + using sync::NonrecursiveLock_NoWait; + using sync::NonrecursiveLock_Waitable; + using sync::RecursiveLock_NoWait; + using sync::RecursiveLock_Waitable; + + /** * Facility for monitor object based locking. * To be attached either on a per class base or per object base. @@ -274,18 +283,33 @@ namespace lib { * * @todo actually implement this facility using the Lumiera backend. */ + template struct Sync { - typedef sync::Monitor Monitor; - + typedef sync::Monitor Monitor; Monitor objectMonitor_; - /////////////////////////////////////////////////////////////////////////TODO: factor out the recursive/non-recursive mutex case as policy... + static Monitor& + getMonitor(Sync* forThis) + { + REQUIRE (forThis); + return forThis->objectMonitor_; + } template - static inline Monitor& getMonitor(); - - static inline Monitor& getMonitor(Sync* forThis); + static Monitor& + getMonitor() + { + //TODO: a rather obscure race condition is hidden here: + //TODO: depending on the build order, the dtor of this static variable may be called, while another thread is still holding an ClassLock. + //TODO: An possible solution would be to use an shared_ptr to the Monitor in case of a ClassLock and to protect access with another Mutex. + //TODO. But I am really questioning if we can't ignore this case and state: "don't hold a ClassLock when your code maybe still running in shutdown phase!" + //TODO: probably best Idea is to detect this situation in DEBUG or ALPHA mode + + static scoped_ptr classMonitor_ (0); + if (!classMonitor_) classMonitor_.reset (new Monitor ()); + return *classMonitor_; + } class Lock @@ -316,31 +340,6 @@ namespace lib { }; - - - - - Sync::Monitor& - Sync::getMonitor(Sync* forThis) - { - REQUIRE (forThis); - return forThis->objectMonitor_; - } - - template - Sync::Monitor& - Sync::getMonitor() - { - //TODO: a rather obscure race condition is hidden here: - //TODO: depending on the build order, the dtor of this static variable may be called, while another thread is still holding an ClassLock. - //TODO: An possible solution would be to use an shared_ptr to the Monitor in case of a ClassLock and to protect access with another Mutex. - //TODO. But I am really questioning if we can't ignore this case and state: "don't hold a ClassLock when your code maybe still running in shutdown phase!" - //TODO: probably best Idea is to detect this situation in DEBUG or ALPHA mode - - static scoped_ptr classMonitor_ (0); - if (!classMonitor_) classMonitor_.reset (new Monitor ()); - return *classMonitor_; - } } // namespace lumiera diff --git a/src/lib/visitordispatcher.hpp b/src/lib/visitordispatcher.hpp index 7977d7003..4c9145716 100644 --- a/src/lib/visitordispatcher.hpp +++ b/src/lib/visitordispatcher.hpp @@ -62,7 +62,7 @@ namespace lumiera { static void generateID (size_t& id) { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); if (!id) id = ++lastRegisteredID; } @@ -138,7 +138,7 @@ namespace lumiera { void accomodate (size_t index) { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); if (index > table_.size()) table_.resize (index); // performance bottleneck?? TODO: measure the real impact! } diff --git a/src/proc/assetmanager.cpp b/src/proc/assetmanager.cpp index 960699189..939162103 100644 --- a/src/proc/assetmanager.cpp +++ b/src/proc/assetmanager.cpp @@ -116,7 +116,7 @@ namespace asset TODO ("check validity of Ident Category"); ID asset_id (getID (idi)); - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); TODO ("handle duplicate Registrations"); P smart_ptr (obj, &destroy); diff --git a/src/proc/mobject/session/defsregistry.hpp b/src/proc/mobject/session/defsregistry.hpp index ad4721b04..f377c3fe6 100644 --- a/src/proc/mobject/session/defsregistry.hpp +++ b/src/proc/mobject/session/defsregistry.hpp @@ -162,7 +162,7 @@ namespace mobject { static void createSlot (Table& table) { - Sync::ClassLock guard(); + Sync<>::ClassLock guard(); if (!index) index = ++maxSlots; if (index > table.size()) diff --git a/tests/lib/sync-locking-test.cpp b/tests/lib/sync-locking-test.cpp index 884b030e7..e246a8083 100644 --- a/tests/lib/sync-locking-test.cpp +++ b/tests/lib/sync-locking-test.cpp @@ -48,7 +48,7 @@ namespace lib { class Victim - : public Sync + : public Sync { volatile long cnt_[NUM_COUNTERS]; volatile uint step_; ///< @note stored as instance variable diff --git a/tests/lib/sync-waiting-test.cpp b/tests/lib/sync-waiting-test.cpp index 3bc087258..1cc8a9398 100644 --- a/tests/lib/sync-waiting-test.cpp +++ b/tests/lib/sync-waiting-test.cpp @@ -66,7 +66,7 @@ namespace lib { class SyncOnBool : public Token, - public Sync + public Sync { bool got_new_data_;