From a0c4f951b0a423ccc91f2c24cc6adf0cbcbd2191 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 6 Oct 2017 01:09:31 -0700 Subject: [PATCH] Address feedback, miscellaneous cleanup --- src/debug/daccess/request.cpp | 6 +-- src/inc/clrconfigvalues.h | 2 +- src/vm/interpreter.cpp | 2 +- src/vm/jithelpers.cpp | 2 +- src/vm/syncblk.cpp | 7 ++-- src/vm/syncblk.h | 78 +++++++++++++++++++++++++---------- src/vm/syncblk.inl | 8 ++-- src/vm/threads.cpp | 16 +++---- 8 files changed, 79 insertions(+), 42 deletions(-) diff --git a/src/debug/daccess/request.cpp b/src/debug/daccess/request.cpp index d1b2affa773d..a08aaef1718d 100644 --- a/src/debug/daccess/request.cpp +++ b/src/debug/daccess/request.cpp @@ -3726,9 +3726,9 @@ ClrDataAccess::GetSyncBlockData(unsigned int SBNumber, struct DacpSyncBlockData } #endif // FEATURE_COMINTEROP - pSyncBlockData->MonitorHeld = pBlock->m_Monitor.m_lockState.VolatileLoad().GetMonitorHeldState(); - pSyncBlockData->Recursion = pBlock->m_Monitor.m_Recursion; - pSyncBlockData->HoldingThread = HOST_CDADDR(pBlock->m_Monitor.m_HoldingThread); + pSyncBlockData->MonitorHeld = pBlock->m_Monitor.GetMonitorHeldStateVolatile(); + pSyncBlockData->Recursion = pBlock->m_Monitor.GetRecursionLevel(); + pSyncBlockData->HoldingThread = HOST_CDADDR(pBlock->m_Monitor.GetHoldingThread()); if (pBlock->GetAppDomainIndex().m_dwIndex) { diff --git a/src/inc/clrconfigvalues.h b/src/inc/clrconfigvalues.h index 3d195f1fe51b..7b096b438f7b 100644 --- a/src/inc/clrconfigvalues.h +++ b/src/inc/clrconfigvalues.h @@ -675,7 +675,7 @@ RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_SpinBackoffFactor, W("SpinBackoffFactor"), RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_SpinLimitProcCap, W("SpinLimitProcCap"), 0xFFFFFFFF, "Hex value specifying the largest value of NumProcs to use when calculating the maximum spin duration", EEConfig_default) RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_SpinLimitProcFactor, W("SpinLimitProcFactor"), 0x4E20, "Hex value specifying the multiplier on NumProcs to use when calculating the maximum spin duration", EEConfig_default) RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_SpinLimitConstant, W("SpinLimitConstant"), 0x0, "Hex value specifying the constant to add when calculating the maximum spin duration", EEConfig_default) -RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_SpinRetryCount, W("SpinRetryCount"), 0x0, "Hex value specifying the number of times the entire spin process is repeated (when applicable)", EEConfig_default) +RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_SpinRetryCount, W("SpinRetryCount"), 0xA, "Hex value specifying the number of times the entire spin process is repeated (when applicable)", EEConfig_default) // // Native Binder diff --git a/src/vm/interpreter.cpp b/src/vm/interpreter.cpp index d18eede1f1a2..b4b18cb13db0 100644 --- a/src/vm/interpreter.cpp +++ b/src/vm/interpreter.cpp @@ -1756,7 +1756,7 @@ static void MonitorEnter(Object* obj, BYTE* pbLockTaken) GET_THREAD()->PulseGCMode(); } objRef->EnterObjMonitor(); - _ASSERTE ((objRef->GetSyncBlock()->GetMonitor()->m_Recursion == 1 && pThread->m_dwLockCount == lockCount + 1) || + _ASSERTE ((objRef->GetSyncBlock()->GetMonitor()->GetRecursionLevel() == 1 && pThread->m_dwLockCount == lockCount + 1) || pThread->m_dwLockCount == lockCount); if (pbLockTaken != 0) *pbLockTaken = 1; diff --git a/src/vm/jithelpers.cpp b/src/vm/jithelpers.cpp index 3c11fbf65ae7..9c4d5ab0feac 100644 --- a/src/vm/jithelpers.cpp +++ b/src/vm/jithelpers.cpp @@ -4416,7 +4416,7 @@ NOINLINE static void JIT_MonEnter_Helper(Object* obj, BYTE* pbLockTaken, LPVOID GET_THREAD()->PulseGCMode(); } objRef->EnterObjMonitor(); - _ASSERTE ((objRef->GetSyncBlock()->GetMonitor()->m_Recursion == 1 && pThread->m_dwLockCount == lockCount + 1) || + _ASSERTE ((objRef->GetSyncBlock()->GetMonitor()->GetRecursionLevel() == 1 && pThread->m_dwLockCount == lockCount + 1) || pThread->m_dwLockCount == lockCount); if (pbLockTaken != 0) *pbLockTaken = 1; diff --git a/src/vm/syncblk.cpp b/src/vm/syncblk.cpp index eb414aa91edf..7b5545a93e04 100644 --- a/src/vm/syncblk.cpp +++ b/src/vm/syncblk.cpp @@ -2157,7 +2157,7 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition SyncBlock* psb = g_pSyncTable[(int)index].m_SyncBlock; _ASSERTE(psb->GetMonitor() != NULL); - Thread* pThread = psb->GetMonitor()->m_HoldingThread; + Thread* pThread = psb->GetMonitor()->GetHoldingThread(); if(pThread == NULL) { *pThreadId = 0; @@ -2167,7 +2167,7 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition else { *pThreadId = pThread->GetThreadId(); - *pAcquisitionCount = psb->GetMonitor()->m_Recursion; + *pAcquisitionCount = psb->GetMonitor()->GetRecursionLevel(); return TRUE; } } @@ -2774,8 +2774,7 @@ SyncBlock *ObjHeader::GetSyncBlock() // The lock is orphaned. pThread = (Thread*) -1; } - syncBlock->InitState(); - syncBlock->SetAwareLock(pThread, recursionLevel + 1); + syncBlock->InitState(recursionLevel + 1, pThread); } } else if ((bits & BIT_SBLK_IS_HASHCODE) != 0) diff --git a/src/vm/syncblk.h b/src/vm/syncblk.h index 1fcaf3ccb8dc..07566a1df47d 100644 --- a/src/vm/syncblk.h +++ b/src/vm/syncblk.h @@ -198,7 +198,7 @@ class AwareLock LeaveHelperAction_Error, }; -public: +private: class LockState { private: @@ -221,6 +221,12 @@ class AwareLock } public: + UINT32 GetState() const + { + LIMITED_METHOD_CONTRACT; + return m_state; + } + UINT32 GetMonitorHeldState() const { LIMITED_METHOD_CONTRACT; @@ -241,7 +247,7 @@ class AwareLock return !(m_state & (IsLockedMask + WaiterCountMask)); } - void SetIsLockedWithNoWaiters() + void InitializeToLockedWithNoWaiters() { LIMITED_METHOD_CONTRACT; _ASSERTE(!m_state); @@ -333,7 +339,7 @@ class AwareLock private: bool NeedToSignalWaiter() const { - LIMITED_METHOD_CONTRACT; + WRAPPER_NO_CONTRACT; return HasAnyWaiters() && !(m_state & (SpinnerCountMask + IsWaiterSignaledToWakeMask)); } @@ -355,7 +361,7 @@ class AwareLock public: LockState VolatileLoad() const { - LIMITED_METHOD_CONTRACT; + WRAPPER_NO_CONTRACT; return ::VolatileLoad(&m_state); } @@ -385,12 +391,11 @@ class AwareLock bool InterlockedObserveWakeSignal_Try_LockAndUnregisterWaiter(); }; -public: +private: LockState m_lockState; ULONG m_Recursion; PTR_Thread m_HoldingThread; - -private: + LONG m_TransientPrecious; @@ -437,6 +442,47 @@ class AwareLock } #endif // defined(ENABLE_CONTRACTS_IMPL) +public: + UINT32 GetLockState() const + { + WRAPPER_NO_CONTRACT; + return m_lockState.GetState(); + } + + bool IsUnlockedWithNoWaiters() const + { + WRAPPER_NO_CONTRACT; + return m_lockState.IsUnlockedWithNoWaiters(); + } + + UINT32 GetMonitorHeldStateVolatile() const + { + WRAPPER_NO_CONTRACT; + return m_lockState.VolatileLoad().GetMonitorHeldState(); + } + + ULONG GetRecursionLevel() const + { + LIMITED_METHOD_CONTRACT; + return m_Recursion; + } + + PTR_Thread GetHoldingThread() const + { + LIMITED_METHOD_CONTRACT; + return m_HoldingThread; + } + +private: // friend access is required for this unsafe function + void InitializeToLockedWithNoWaiters(ULONG recursionLevel, PTR_Thread holdingThread) + { + WRAPPER_NO_CONTRACT; + + m_lockState.InitializeToLockedWithNoWaiters(); + m_Recursion = recursionLevel; + m_HoldingThread = holdingThread; + } + public: static void SpinWait(DWORD spinCount); @@ -819,7 +865,7 @@ class SyncBlock { WRAPPER_NO_CONTRACT; return (!IsPrecious() && - m_Monitor.m_lockState.IsUnlockedWithNoWaiters() && + m_Monitor.IsUnlockedWithNoWaiters() && m_Monitor.m_TransientPrecious == 0); } @@ -904,16 +950,6 @@ class SyncBlock m_dwAppDomainIndex = dwAppDomainIndex; } - void SetAwareLock(Thread *holdingThread, DWORD recursionLevel) - { - LIMITED_METHOD_CONTRACT; - // - // DO NOT SET m_lockState HERE! THIS IS NOT PROTECTED BY ANY LOCK!! - // - m_Monitor.m_HoldingThread = PTR_Thread(holdingThread); - m_Monitor.m_Recursion = recursionLevel; - } - DWORD GetHashCode() { LIMITED_METHOD_CONTRACT; @@ -1058,10 +1094,10 @@ class SyncBlock // This should ONLY be called when initializing a SyncBlock (i.e. ONLY from // ObjHeader::GetSyncBlock()), otherwise we'll have a race condition. // - void InitState() + void InitState(ULONG recursionLevel, PTR_Thread holdingThread) { - LIMITED_METHOD_CONTRACT; - m_Monitor.m_lockState.SetIsLockedWithNoWaiters(); + WRAPPER_NO_CONTRACT; + m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThread); } #if defined(ENABLE_CONTRACTS_IMPL) diff --git a/src/vm/syncblk.inl b/src/vm/syncblk.inl index 40fb0dc95d9e..e60b44b55ff1 100644 --- a/src/vm/syncblk.inl +++ b/src/vm/syncblk.inl @@ -148,7 +148,8 @@ FORCEINLINE bool AwareLock::LockState::InterlockedUnregisterSpinner_TryLock() return false; } - LockState state = stateBeforeUpdate - SpinnerCountIncrement; + LockState state = stateBeforeUpdate; + state.DecrementSpinnerCount(); _ASSERTE(!state.IsLocked()); do { @@ -218,8 +219,8 @@ FORCEINLINE bool AwareLock::LockState::InterlockedTry_LockAndUnregisterWaiterAnd LockState newState = state; newState.InvertIsLocked(); - newState.DecrementWaiterCount(); newState.InvertIsWaiterSignaledToWake(); + newState.DecrementWaiterCount(); LockState stateBeforeUpdate = CompareExchange(newState, state); if (stateBeforeUpdate == state) @@ -247,7 +248,8 @@ FORCEINLINE bool AwareLock::LockState::InterlockedObserveWakeSignal_Try_LockAndU return false; } - LockState state = stateBeforeUpdate - IsWaiterSignaledToWakeMask; + LockState state = stateBeforeUpdate; + state.InvertIsWaiterSignaledToWake(); _ASSERTE(!state.IsLocked()); do { diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 0b0fc7333d05..2218c944a285 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -1579,11 +1579,11 @@ void Dbg_TrackSyncStack::EnterSync(UINT_PTR caller, void *pAwareLock) { LIMITED_METHOD_CONTRACT; - STRESS_LOG4(LF_SYNC, LL_INFO100, "Dbg_TrackSyncStack::EnterSync, IP=%p, Recursion=%d, MonitorHeld=%d, HoldingThread=%p.\n", + STRESS_LOG4(LF_SYNC, LL_INFO100, "Dbg_TrackSyncStack::EnterSync, IP=%p, Recursion=%u, LockState=%x, HoldingThread=%p.\n", caller, - ((AwareLock*)pAwareLock)->m_Recursion, - (int)((AwareLock*)pAwareLock)->m_lockState, - ((AwareLock*)pAwareLock)->m_HoldingThread ); + ((AwareLock*)pAwareLock)->GetRecursionLevel(), + ((AwareLock*)pAwareLock)->GetState(), + ((AwareLock*)pAwareLock)->GetHoldingThread()); if (m_Active) { @@ -1605,11 +1605,11 @@ void Dbg_TrackSyncStack::LeaveSync(UINT_PTR caller, void *pAwareLock) { WRAPPER_NO_CONTRACT; - STRESS_LOG4(LF_SYNC, LL_INFO100, "Dbg_TrackSyncStack::LeaveSync, IP=%p, Recursion=%d, MonitorHeld=%d, HoldingThread=%p.\n", + STRESS_LOG4(LF_SYNC, LL_INFO100, "Dbg_TrackSyncStack::LeaveSync, IP=%p, Recursion=%u, LockState=%x, HoldingThread=%p.\n", caller, - ((AwareLock*)pAwareLock)->m_Recursion, - (int)((AwareLock*)pAwareLock)->m_lockState, - ((AwareLock*)pAwareLock)->m_HoldingThread ); + ((AwareLock*)pAwareLock)->GetRecursionLevel(), + ((AwareLock*)pAwareLock)->GetState(), + ((AwareLock*)pAwareLock)->GetHoldingThread()); if (m_Active) {