diff options
| -rw-r--r-- | base/lock.cc | 104 | ||||
| -rw-r--r-- | base/lock.h | 55 |
2 files changed, 46 insertions, 113 deletions
diff --git a/base/lock.cc b/base/lock.cc index 70fa044..1dd4dc1 100644 --- a/base/lock.cc +++ b/base/lock.cc @@ -7,42 +7,31 @@ // The Lock class is used everywhere, and hence any changes // to lock.h tend to require a complete rebuild. To facilitate // profiler development, all the profiling methods are listed -// here. Note that they are only instantiated in a debug -// build, and the header provides all the trivial implementations -// in a production build. +// here. #include "base/lock.h" #include "base/logging.h" +#ifndef NDEBUG Lock::Lock() : lock_(), - recursion_count_shadow_(0) { -#ifndef NDEBUG - recursion_used_ = false; - acquisition_count_ = 0; - contention_count_ = 0; -#endif + recursion_count_shadow_(0), + recursion_used_(false), + acquisition_count_(0), + contention_count_(0) { +} +#else // NDEBUG +Lock::Lock() + : lock_() { } +#endif // NDEBUG Lock::~Lock() { -#ifndef NDEBUG - // There should be no one to contend for the lock, - // ...but we need the memory barrier to get a good value. - lock_.Lock(); - int final_recursion_count = recursion_count_shadow_; - lock_.Unlock(); -#endif - - // Allow unit test exception only at end of method. -#ifndef NDEBUG - DCHECK(0 == final_recursion_count); -#endif } void Lock::Acquire() { #ifdef NDEBUG lock_.Lock(); - recursion_count_shadow_++; #else // NDEBUG if (!lock_.Try()) { // We have contention. @@ -51,82 +40,35 @@ void Lock::Acquire() { } // ONLY access data after locking. recursion_count_shadow_++; - if (1 == recursion_count_shadow_) - acquisition_count_++; - else if (2 == recursion_count_shadow_ && !recursion_used_) - // Usage Note: Set a break point to debug. + acquisition_count_++; + if (2 == recursion_count_shadow_ && !recursion_used_) { recursion_used_ = true; + // TODO(sky): Uncomment this DCHECK after fixing test cases. + // DCHECK(false); // Catch accidental redundant lock acquisition. + } #endif // NDEBUG } void Lock::Release() { - --recursion_count_shadow_; // ONLY access while lock is still held. #ifndef NDEBUG + --recursion_count_shadow_; // ONLY access while lock is still held. DCHECK(0 <= recursion_count_shadow_); -#endif +#endif // NDEBUG lock_.Unlock(); } bool Lock::Try() { if (lock_.Try()) { - recursion_count_shadow_++; #ifndef NDEBUG - if (1 == recursion_count_shadow_) - acquisition_count_++; - else if (2 == recursion_count_shadow_ && !recursion_used_) - // Usage Note: Set a break point to debug. + recursion_count_shadow_++; + acquisition_count_++; + if (2 == recursion_count_shadow_ && !recursion_used_) { recursion_used_ = true; + DCHECK(false); // Catch accidental redundant lock acquisition. + } #endif return true; } else { return false; } } - -// GetCurrentThreadRecursionCount returns the number of nested Acquire() calls -// that have been made by the current thread holding this lock. The calling -// thread is ***REQUIRED*** to be *currently* holding the lock. If that -// calling requirement is violated, the return value is not well defined. -// Return results are guaranteed correct if the caller has acquired this lock. -// The return results might be incorrect otherwise. -// This method is designed to be fast in non-debug mode by co-opting -// synchronization using lock_ (no additional synchronization is used), but in -// debug mode it slowly and carefully validates the requirement (and fires a -// a DCHECK if it was called incorrectly). -int32 Lock::GetCurrentThreadRecursionCount() { -#ifndef NDEBUG - // If this DCHECK fails, then the most probable cause is: - // This method was called by class AutoUnlock during processing of a - // Wait() call made into the ConditonVariable class. That call to - // Wait() was made (incorrectly) without first Aquiring this Lock - // instance. - lock_.Lock(); - int temp = recursion_count_shadow_; - lock_.Unlock(); - // Unit tests catch an exception, so we need to be careful to test - // outside the critical section, since the Leave would be skipped!?! - DCHECK(temp >= 1); // Allow unit test exception only at end of method. -#endif // DEBUG - - // We hold lock, so this *is* correct value. - return recursion_count_shadow_; -} - - -AutoUnlock::AutoUnlock(Lock& lock) : lock_(&lock), release_count_(0) { - // We require our caller have the lock, so we can call for recursion count. - // CRITICALLY: Fetch value before we release the lock. - int32 count = lock_->GetCurrentThreadRecursionCount(); - DCHECK(count > 0); // Make sure we owned the lock. - while (count-- > 0) { - release_count_++; - lock_->Release(); - } -} - -AutoUnlock::~AutoUnlock() { - DCHECK(release_count_ >= 0); - while (release_count_-- > 0) - lock_->Acquire(); -} - diff --git a/base/lock.h b/base/lock.h index 4e736af..b43e6ef 100644 --- a/base/lock.h +++ b/base/lock.h @@ -7,14 +7,16 @@ #include "base/lock_impl.h" -// A convenient wrapper for a critical section. +// A convenient wrapper for an OS specific critical section. // -// NOTE: A thread may acquire the same lock multiple times, but it must call -// Release for each call to Acquire in order to finally release the lock. +// NOTE: Although windows critical sections support recursive locks, we do not +// allow this, and we will commonly fire a DCHECK() if a thread attempts to +// acquire the lock a second time (while already holding it). // // Complication: UnitTest for DeathTests catch DCHECK exceptions, so we need // to write code assuming DCHECK will throw. This means we need to save any // assertable value in a local until we can safely throw. + class Lock { public: Lock(); @@ -22,7 +24,7 @@ class Lock { void Acquire(); void Release(); // If the lock is not held, take it and return true. If the lock is already - // held by something else, immediately return false. + // held by another thread, immediately return false. bool Try(); // Return the underlying lock implementation. @@ -33,20 +35,13 @@ class Lock { private: LockImpl lock_; // User-supplied underlying lock implementation. - // All private data is implicitly protected by spin_lock_. - // Be VERY careful to only access under that lock. - int32 recursion_count_shadow_; - - // Allow access to GetCurrentThreadRecursionCount() - friend class AutoUnlock; - int32 GetCurrentThreadRecursionCount(); - #ifndef NDEBUG - // Even in Debug mode, the expensive tallies won't be calculated by default. - bool recursion_used_; - int32 acquisition_count_; - - int32 contention_count_; + // All private data is implicitly protected by lock_. + // Be VERY careful to only access members under that lock. + int32 recursion_count_shadow_; + bool recursion_used_; // Allow debugging to continued after a DCHECK(). + int32 acquisition_count_; // Number of times lock was acquired. + int32 contention_count_; // Number of times there was contention. #endif // NDEBUG DISALLOW_COPY_AND_ASSIGN(Lock); @@ -68,27 +63,23 @@ class AutoLock { DISALLOW_COPY_AND_ASSIGN(AutoLock); }; -// AutoUnlock is a helper class for ConditionVariable instances -// that is analogous to AutoLock. It provides for nested Releases -// of a lock for the Wait functionality of a ConditionVariable class. -// The destructor automatically does the corresponding Acquire -// calls (to return to the initial nested lock state). - -// Instances of AutoUnlock can ***ONLY*** validly be constructed if the -// caller currently holds the lock provided as the constructor's argument. -// If that ***REQUIREMENT*** is violated in debug mode, a DCHECK will -// be generated in the Lock class. In production (non-debug), -// the results are undefined (and probably bad) if the caller -// is not already holding the indicated lock. +// AutoUnlock is a helper class for ConditionVariable that will Release() the +// lock argument in the constructor, and re-Acquire() it in the destructor. class ConditionVariable; class AutoUnlock { private: // Everything is private, so only our friend can use us. friend class ConditionVariable; // The only user of this class. - explicit AutoUnlock(Lock& lock); - ~AutoUnlock(); + + explicit AutoUnlock(Lock& lock) : lock_(&lock) { + // We require our caller to have the lock. + lock_->Release(); + } + + ~AutoUnlock() { + lock_->Acquire(); + } Lock* lock_; - int release_count_; }; #endif // BASE_LOCK_H_ |