From 494f20977ee59fe27263b9133f5104786f373df1 Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 27 Apr 2015 17:39:23 +0200 Subject: [PATCH] Move CriticalSection into rtc_base_approved. This class is being used from both libjingle and webrtc but we recently had a regression when we added dependency on libjingle's Thread class. This cleans that up and moves the implementation of CriticalSection and helper classes into the source file. I'm also improving debugging facilities and constness. BUG= R=magjed@webrtc.org Review URL: https://webrtc-codereview.appspot.com/51769004 Cr-Commit-Position: refs/heads/master@{#9089} --- webrtc/base/BUILD.gn | 4 +- webrtc/base/base.gyp | 8 +- webrtc/base/criticalsection.cc | 126 ++++++++++++++++++++++++++++- webrtc/base/criticalsection.h | 144 +++++++++------------------------ webrtc/base/messagequeue.cc | 6 +- 5 files changed, 173 insertions(+), 115 deletions(-) diff --git a/webrtc/base/BUILD.gn b/webrtc/base/BUILD.gn index 363f8c3a4e..35d33ab99a 100644 --- a/webrtc/base/BUILD.gn +++ b/webrtc/base/BUILD.gn @@ -114,6 +114,8 @@ static_library("rtc_base_approved") { "byteorder.h", "checks.cc", "checks.h", + "criticalsection.cc", + "criticalsection.h", "event.cc", "event.h", "event_tracer.cc", @@ -204,8 +206,6 @@ static_library("rtc_base") { "cpumonitor.h", "crc32.cc", "crc32.h", - "criticalsection.cc", - "criticalsection.h", "cryptstring.cc", "cryptstring.h", "diskcache.cc", diff --git a/webrtc/base/base.gyp b/webrtc/base/base.gyp index ce64bd026f..51b72917d6 100644 --- a/webrtc/base/base.gyp +++ b/webrtc/base/base.gyp @@ -29,6 +29,8 @@ 'target_name': 'rtc_base_approved', 'type': 'static_library', 'sources': [ + '../overrides/webrtc/base/basictypes.h', + '../overrides/webrtc/base/constructormagic.h', 'basictypes.h', 'bitbuffer.cc', 'bitbuffer.h', @@ -40,6 +42,8 @@ 'checks.cc', 'checks.h', 'constructormagic.h', + 'criticalsection.cc', + 'criticalsection.h', 'event.cc', 'event.h', 'event_tracer.cc', @@ -66,8 +70,6 @@ 'timeutils.cc', 'timeutils.h', 'trace_event.h', - '../overrides/webrtc/base/basictypes.h', - '../overrides/webrtc/base/constructormagic.h', ], 'conditions': [ ['build_with_chromium==1', { @@ -134,8 +136,6 @@ 'cpumonitor.h', 'crc32.cc', 'crc32.h', - 'criticalsection.cc', - 'criticalsection.h', 'cryptstring.cc', 'cryptstring.h', 'dbus.cc', diff --git a/webrtc/base/criticalsection.cc b/webrtc/base/criticalsection.cc index fcad5c31d6..f4822d96fa 100644 --- a/webrtc/base/criticalsection.cc +++ b/webrtc/base/criticalsection.cc @@ -11,13 +11,135 @@ #include "webrtc/base/criticalsection.h" #include "webrtc/base/checks.h" -#include "webrtc/base/thread.h" namespace rtc { +CriticalSection::CriticalSection() { +#if defined(WEBRTC_WIN) + InitializeCriticalSection(&crit_); +#else + pthread_mutexattr_t mutex_attribute; + pthread_mutexattr_init(&mutex_attribute); + pthread_mutexattr_settype(&mutex_attribute, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&mutex_, &mutex_attribute); + pthread_mutexattr_destroy(&mutex_attribute); + CS_DEBUG_CODE(thread_ = 0); + CS_DEBUG_CODE(recursion_count_ = 0); +#endif +} + +CriticalSection::~CriticalSection() { +#if defined(WEBRTC_WIN) + DeleteCriticalSection(&crit_); +#else + pthread_mutex_destroy(&mutex_); +#endif +} + +void CriticalSection::Enter() EXCLUSIVE_LOCK_FUNCTION() { +#if defined(WEBRTC_WIN) + EnterCriticalSection(&crit_); +#else + pthread_mutex_lock(&mutex_); +#if CS_DEBUG_CHECKS + if (!recursion_count_) { + DCHECK(!thread_); + thread_ = pthread_self(); + } else { + DCHECK(CurrentThreadIsOwner()); + } + ++recursion_count_; +#endif +#endif +} + +bool CriticalSection::TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true) { +#if defined(WEBRTC_WIN) + return TryEnterCriticalSection(&crit_) != FALSE; +#else + if (pthread_mutex_trylock(&mutex_) != 0) + return false; +#if CS_DEBUG_CHECKS + if (!recursion_count_) { + DCHECK(!thread_); + thread_ = pthread_self(); + } else { + DCHECK(CurrentThreadIsOwner()); + } + ++recursion_count_; +#endif + return true; +#endif +} +void CriticalSection::Leave() UNLOCK_FUNCTION() { + DCHECK(CurrentThreadIsOwner()); +#if defined(WEBRTC_WIN) + LeaveCriticalSection(&crit_); +#else +#if CS_DEBUG_CHECKS + --recursion_count_; + DCHECK(recursion_count_ >= 0); + if (!recursion_count_) + thread_ = 0; +#endif + pthread_mutex_unlock(&mutex_); +#endif +} + +bool CriticalSection::CurrentThreadIsOwner() const { +#if defined(WEBRTC_WIN) + return crit_.OwningThread == reinterpret_cast(GetCurrentThreadId()); +#else +#if CS_DEBUG_CHECKS + return pthread_equal(thread_, pthread_self()); +#else + return true; +#endif // CS_DEBUG_CHECKS +#endif +} + +bool CriticalSection::IsLocked() const { +#if defined(WEBRTC_WIN) + return crit_.LockCount != -1; +#else +#if CS_DEBUG_CHECKS + return thread_ != 0; +#else + return true; +#endif +#endif +} + +CritScope::CritScope(CriticalSection* cs) : cs_(cs) { cs_->Enter(); } +CritScope::~CritScope() { cs_->Leave(); } + +TryCritScope::TryCritScope(CriticalSection* cs) + : cs_(cs), locked_(cs->TryEnter()) { + CS_DEBUG_CODE(lock_was_called_ = false); +} + +TryCritScope::~TryCritScope() { + CS_DEBUG_CODE(DCHECK(lock_was_called_)); + if (locked_) + cs_->Leave(); +} + +bool TryCritScope::locked() const { + CS_DEBUG_CODE(lock_was_called_ = true); + return locked_; +} + void GlobalLockPod::Lock() { +#if !defined(WEBRTC_WIN) + const struct timespec ts_null = {0}; +#endif + while (AtomicOps::CompareAndSwap(&lock_acquired, 0, 1)) { - Thread::SleepMs(0); +#if defined(WEBRTC_WIN) + ::Sleep(0); +#else + nanosleep(&ts_null, nullptr); +#endif } } diff --git a/webrtc/base/criticalsection.h b/webrtc/base/criticalsection.h index 1891f83bb9..d4e6bd76eb 100644 --- a/webrtc/base/criticalsection.h +++ b/webrtc/base/criticalsection.h @@ -21,116 +21,56 @@ // exists as two separate projects, webrtc and libjingle. #include #include -#endif +#include // must come after windows headers. +#endif // defined(WEBRTC_WIN) #if defined(WEBRTC_POSIX) #include #endif #if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)) -#define CS_TRACK_OWNER 1 +#define CS_DEBUG_CHECKS 1 #endif -#if CS_TRACK_OWNER -#define TRACK_OWNER(x) x -#else // !CS_TRACK_OWNER -#define TRACK_OWNER(x) -#endif // !CS_TRACK_OWNER +#if CS_DEBUG_CHECKS +#define CS_DEBUG_CODE(x) x +#else // !CS_DEBUG_CHECKS +#define CS_DEBUG_CODE(x) +#endif // !CS_DEBUG_CHECKS namespace rtc { +class LOCKABLE CriticalSection { + public: + CriticalSection(); + ~CriticalSection(); + + void Enter() EXCLUSIVE_LOCK_FUNCTION(); + bool TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true); + void Leave() UNLOCK_FUNCTION(); + + // Use only for DCHECKing. + bool CurrentThreadIsOwner() const; + // Use only for DCHECKing. + bool IsLocked() const; + + private: #if defined(WEBRTC_WIN) -class LOCKABLE CriticalSection { - public: - CriticalSection() { InitializeCriticalSection(&crit_); } - ~CriticalSection() { DeleteCriticalSection(&crit_); } - void Enter() EXCLUSIVE_LOCK_FUNCTION() { - EnterCriticalSection(&crit_); - } - bool TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true) { - return TryEnterCriticalSection(&crit_) != FALSE; - } - void Leave() UNLOCK_FUNCTION() { - LeaveCriticalSection(&crit_); - } - - // Used for debugging. - bool CurrentThreadIsOwner() const { - return crit_.OwningThread == reinterpret_cast(GetCurrentThreadId()); - } - // Use only for DCHECKing. - bool IsLocked() { return crit_.LockCount != -1; } - - private: CRITICAL_SECTION crit_; -}; -#endif // WEBRTC_WIN - -#if defined(WEBRTC_POSIX) -class LOCKABLE CriticalSection { - public: - CriticalSection() { - pthread_mutexattr_t mutex_attribute; - pthread_mutexattr_init(&mutex_attribute); - pthread_mutexattr_settype(&mutex_attribute, PTHREAD_MUTEX_RECURSIVE); - pthread_mutex_init(&mutex_, &mutex_attribute); - pthread_mutexattr_destroy(&mutex_attribute); - TRACK_OWNER(thread_ = 0); - } - ~CriticalSection() { - pthread_mutex_destroy(&mutex_); - } - void Enter() EXCLUSIVE_LOCK_FUNCTION() { - pthread_mutex_lock(&mutex_); - TRACK_OWNER(thread_ = pthread_self()); - } - bool TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true) { - if (pthread_mutex_trylock(&mutex_) == 0) { - TRACK_OWNER(thread_ = pthread_self()); - return true; - } - return false; - } - void Leave() UNLOCK_FUNCTION() { - TRACK_OWNER(thread_ = 0); - pthread_mutex_unlock(&mutex_); - } - - // Used for debugging. - bool CurrentThreadIsOwner() const { -#if CS_TRACK_OWNER - return pthread_equal(thread_, pthread_self()); -#else - return true; -#endif // CS_TRACK_OWNER - } - // Use only for DCHECKing. - bool IsLocked() { -#if CS_TRACK_OWNER - return thread_ != 0; -#else - return true; -#endif - } - - private: +#elif defined(WEBRTC_POSIX) pthread_mutex_t mutex_; - TRACK_OWNER(pthread_t thread_); + CS_DEBUG_CODE(pthread_t thread_); + CS_DEBUG_CODE(int recursion_count_); +#endif }; -#endif // WEBRTC_POSIX // CritScope, for serializing execution through a scope. class SCOPED_LOCKABLE CritScope { public: - explicit CritScope(CriticalSection *pcrit) EXCLUSIVE_LOCK_FUNCTION(pcrit) { - pcrit_ = pcrit; - pcrit_->Enter(); - } - ~CritScope() UNLOCK_FUNCTION() { - pcrit_->Leave(); - } + explicit CritScope(CriticalSection* cs) EXCLUSIVE_LOCK_FUNCTION(cs); + ~CritScope() UNLOCK_FUNCTION(); private: - CriticalSection *pcrit_; + CriticalSection* const cs_; DISALLOW_COPY_AND_ASSIGN(CritScope); }; @@ -143,21 +83,17 @@ class SCOPED_LOCKABLE CritScope { // lock was taken. If you're not calling locked(), you're doing it wrong! class TryCritScope { public: - explicit TryCritScope(CriticalSection *pcrit) { - pcrit_ = pcrit; - locked_ = pcrit_->TryEnter(); - } - ~TryCritScope() { - if (locked_) { - pcrit_->Leave(); - } - } - bool locked() const { - return locked_; - } + explicit TryCritScope(CriticalSection* cs); + ~TryCritScope(); +#if defined(WEBRTC_WIN) + _Check_return_ bool locked() const; +#else + bool locked() const __attribute__((warn_unused_result)); +#endif private: - CriticalSection *pcrit_; - bool locked_; + CriticalSection* const cs_; + const bool locked_; + CS_DEBUG_CODE(mutable bool lock_was_called_); DISALLOW_COPY_AND_ASSIGN(TryCritScope); }; diff --git a/webrtc/base/messagequeue.cc b/webrtc/base/messagequeue.cc index 53e451f632..5cf448e3bd 100644 --- a/webrtc/base/messagequeue.cc +++ b/webrtc/base/messagequeue.cc @@ -59,7 +59,7 @@ void MessageQueueManager::AddInternal(MessageQueue *message_queue) { // MessageQueueManager methods should be non-reentrant, so we // ASSERT that is the case. If any of these ASSERT, please // contact bpm or jbeda. -#if CS_TRACK_OWNER // CurrentThreadIsOwner returns true by default. +#if CS_DEBUG_CHECKS // CurrentThreadIsOwner returns true by default. ASSERT(!crit_.CurrentThreadIsOwner()); #endif CritScope cs(&crit_); @@ -73,7 +73,7 @@ void MessageQueueManager::Remove(MessageQueue *message_queue) { return Instance()->RemoveInternal(message_queue); } void MessageQueueManager::RemoveInternal(MessageQueue *message_queue) { -#if CS_TRACK_OWNER // CurrentThreadIsOwner returns true by default. +#if CS_DEBUG_CHECKS // CurrentThreadIsOwner returns true by default. ASSERT(!crit_.CurrentThreadIsOwner()); // See note above. #endif // If this is the last MessageQueue, destroy the manager as well so that @@ -104,7 +104,7 @@ void MessageQueueManager::Clear(MessageHandler *handler) { return Instance()->ClearInternal(handler); } void MessageQueueManager::ClearInternal(MessageHandler *handler) { -#if CS_TRACK_OWNER // CurrentThreadIsOwner returns true by default. +#if CS_DEBUG_CHECKS // CurrentThreadIsOwner returns true by default. ASSERT(!crit_.CurrentThreadIsOwner()); // See note above. #endif CritScope cs(&crit_);