Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove thread pointer field from AwareLock #111515

Closed
wants to merge 5 commits into from
Closed

Remove thread pointer field from AwareLock #111515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff22d24
Add changes in syncblk.inl
eduardo-vp Nov 28, 2024
8951e1d
Use m_HoldingThread in AwareLock::LeaveHelper
eduardo-vp Nov 29, 2024
ab7ee48
Use thread id to compare in AwareLock::LeaveHelper
eduardo-vp Dec 2, 2024
e33e071
Remove m_HoldingThread field. Some refactor still missing.
eduardo-vp Dec 5, 2024
a1742e4
Add GetHoldingThread
eduardo-vp Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 10 additions & 16 deletions src/coreclr/vm/syncblk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1815,23 +1815,20 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition
SyncBlock* psb = g_pSyncTable[(int)index].m_SyncBlock;

_ASSERTE(psb->GetMonitor() != NULL);
Thread* pThread = psb->GetMonitor()->GetHoldingThread();
// If the lock is orphaned during sync block creation, pThread would be assigned -1.
// Otherwise pThread would point to the owning thread if there was one or NULL if there wasn't.
if (pThread == NULL || pThread == (Thread*) -1)
DWORD holdingThreadId = psb->GetMonitor()->GetHoldingThreadId();
// If the lock is orphaned during sync block creation, holdingThreadId would be assigned -1.
// Otherwise it would be the id of the holding thread if there is one or 0 if there isn't.
if (holdingThreadId == 0 || holdingThreadId == -1)
{
*pThreadId = 0;
*pAcquisitionCount = 0;
return FALSE;
}
else
{
// However, the lock might get orphaned after the sync block is created.
// Therefore accessing pThread is not safe and pThread->GetThreadId() shouldn't be used.
// The thread id can be obtained from the monitor, which would be the id of the thread that owned the lock.
// Notice this id now could have been reused for a different thread,
// but this way we avoid crashing the process and orphaned locks shouldn't be expected to work correctly anyway.
*pThreadId = psb->GetMonitor()->GetHoldingThreadId();
// Notice this id now could have been reused for a different thread (in case the lock was orphaned),
// but orphaned locks shouldn't be expected to work correctly anyway.
*pThreadId = holdingThreadId;
*pAcquisitionCount = psb->GetMonitor()->GetRecursionLevel();
return TRUE;
}
Expand Down Expand Up @@ -2371,12 +2368,11 @@ void AwareLock::Enter()

Thread *pCurThread = GetThread();
LockState state = m_lockState.VolatileLoadWithoutBarrier();
if (!state.IsLocked() || m_HoldingThread != pCurThread)
if (!state.IsLocked() || m_HoldingThreadId != pCurThread->GetThreadId())
{
if (m_lockState.InterlockedTryLock_Or_RegisterWaiter(this, state))
{
// We get here if we successfully acquired the mutex.
m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
Expand Down Expand Up @@ -2433,14 +2429,13 @@ BOOL AwareLock::TryEnter(INT32 timeOut)
}

LockState state = m_lockState.VolatileLoadWithoutBarrier();
if (!state.IsLocked() || m_HoldingThread != pCurThread)
if (!state.IsLocked() || m_HoldingThreadId != pCurThread->GetThreadId())
{
if (timeOut == 0
? m_lockState.InterlockedTryLock(state)
: m_lockState.InterlockedTryLock_Or_RegisterWaiter(this, state))
{
// We get here if we successfully acquired the mutex.
m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
Expand Down Expand Up @@ -2720,7 +2715,6 @@ BOOL AwareLock::EnterEpilogHelper(Thread* pCurThread, INT32 timeOut)
return false;
}

m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
Expand Down Expand Up @@ -2783,7 +2777,7 @@ LONG AwareLock::LeaveCompletely()
BOOL AwareLock::OwnedByCurrentThread()
{
WRAPPER_NO_CONTRACT;
return (GetThread() == m_HoldingThread);
return (GetThread()->GetThreadId() == m_HoldingThreadId);
}

// ***************************************************************************
Expand Down
22 changes: 4 additions & 18 deletions src/coreclr/vm/syncblk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,6 @@ class AwareLock
LockState m_lockState;

ULONG m_Recursion;
PTR_Thread m_HoldingThread;
DWORD m_HoldingThreadId;
SIZE_T m_HoldingOSThreadId;

Expand All @@ -456,10 +455,6 @@ class AwareLock
// Only SyncBlocks can create AwareLocks. Hence this private constructor.
AwareLock(DWORD indx)
: m_Recursion(0),
#ifndef DACCESS_COMPILE
// PreFAST has trouble with initializing a NULL PTR_Thread.
m_HoldingThread(NULL),
#endif // DACCESS_COMPILE
m_HoldingThreadId(0),
m_HoldingOSThreadId(0),
m_TransientPrecious(0),
Expand Down Expand Up @@ -522,7 +517,7 @@ class AwareLock
PTR_Thread GetHoldingThread() const
{
LIMITED_METHOD_CONTRACT;
return m_HoldingThread;
return g_pThinLockThreadIdDispenser->IdToThreadWithValidation(m_HoldingThreadId);
}

DWORD GetHoldingThreadId() const
Expand All @@ -537,13 +532,12 @@ class AwareLock
bool ShouldStopPreemptingWaiters() const;

private: // friend access is required for this unsafe function
void InitializeToLockedWithNoWaiters(ULONG recursionLevel, PTR_Thread holdingThread, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
void InitializeToLockedWithNoWaiters(ULONG recursionLevel, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
{
WRAPPER_NO_CONTRACT;

m_lockState.InitializeToLockedWithNoWaiters();
m_Recursion = recursionLevel;
m_HoldingThread = holdingThread;
m_HoldingThreadId = holdingThreadId;
m_HoldingOSThreadId = holdingOSThreadId;
}
Expand Down Expand Up @@ -604,14 +598,6 @@ class AwareLock
// protect it.
inline OBJECTREF GetOwningObject();

// Provide access to the Thread object that owns this awarelock. This is used
// to provide a host to find out owner of a lock.
inline PTR_Thread GetOwningThread()
{
LIMITED_METHOD_CONTRACT;
return m_HoldingThread;
}

static int GetOffsetOfHoldingOSThreadId()
{
LIMITED_METHOD_CONTRACT;
Expand Down Expand Up @@ -1279,10 +1265,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.
// </NOTE>
void InitState(ULONG recursionLevel, PTR_Thread holdingThread, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
void InitState(ULONG recursionLevel, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
{
WRAPPER_NO_CONTRACT;
m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThread, holdingThreadId, holdingOSThreadId);
m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThreadId, holdingOSThreadId);
}

#if defined(ENABLE_CONTRACTS_IMPL)
Expand Down
11 changes: 3 additions & 8 deletions src/coreclr/vm/syncblk.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,14 +478,13 @@ FORCEINLINE bool AwareLock::TryEnterHelper(Thread* pCurThread)

if (m_lockState.InterlockedTryLock())
{
m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
return true;
}

if (GetOwningThread() == pCurThread) /* monitor is held, but it could be a recursive case */
if (GetHoldingThreadId() == pCurThread->GetThreadId()) /* monitor is held, but it could be a recursive case */
{
m_Recursion++;
return true;
Expand All @@ -505,7 +504,7 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterBeforeSpinLoopHelper

// Check the recursive case once before the spin loop. If it's not the recursive case in the beginning, it will not
// be in the future, so the spin loop can avoid checking the recursive case.
if (!state.IsLocked() || GetOwningThread() != pCurThread)
if (!state.IsLocked() || GetHoldingThreadId() != pCurThread->GetThreadId())
{
if (m_lockState.InterlockedTrySetShouldNotPreemptWaitersIfNecessary(this, state))
{
Expand All @@ -525,7 +524,6 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterBeforeSpinLoopHelper
}

// Lock was acquired and the spinner was not registered
m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
Expand Down Expand Up @@ -558,7 +556,6 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterInsideSpinLoopHelper
}

// Lock was acquired and spinner was unregistered
m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
Expand All @@ -582,7 +579,6 @@ FORCEINLINE bool AwareLock::TryEnterAfterSpinLoopHelper(Thread *pCurThread)
}

// Spinner was unregistered and the lock was acquired
m_HoldingThread = pCurThread;
m_HoldingThreadId = pCurThread->GetThreadId();
m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
m_Recursion = 1;
Expand Down Expand Up @@ -687,7 +683,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre
MODE_ANY;
} CONTRACTL_END;

if (m_HoldingThread != pCurThread)
if (m_HoldingThreadId != pCurThread->GetThreadId())
return AwareLock::LeaveHelperAction_Error;

_ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked());
Expand All @@ -702,7 +698,6 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre

if (--m_Recursion == 0)
{
m_HoldingThread = NULL;
m_HoldingThreadId = 0;
m_HoldingOSThreadId = 0;

Expand Down
Loading