Skip to content

Cleanup semaphore usage in utilcode #115685

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

Closed
wants to merge 6 commits into from
Closed

Cleanup semaphore usage in utilcode #115685

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b88092
Use pthread_rwlock_t for utsem
huoyaoyuan May 17, 2025
5c31732
Cleanup Semaphore from PAL
huoyaoyuan May 17, 2025
57b7fa3
Test HOST for dac
huoyaoyuan May 17, 2025
c1fc77d
Revert "Cleanup Semaphore from PAL"
huoyaoyuan May 18, 2025
f894dc6
Trailing space
huoyaoyuan May 18, 2025
a9232ca
Check for failure
huoyaoyuan May 20, 2025
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
12 changes: 10 additions & 2 deletions src/coreclr/inc/utsem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
// INCLUDES
// -------------------------------------------------------------
#include "utilcode.h"
#ifdef HOST_UNIX
#include <pthread.h>
#endif // TARGET_UNIX

/* ----------------------------------------------------------------------------
@class UTSemReadWrite
Expand All @@ -39,15 +42,20 @@ class UTSemReadWrite
void UnlockRead(); // Unlock the object for reading
void UnlockWrite(); // Unlock the object for writing

#ifdef _DEBUG
#if defined(_DEBUG) && defined(HOST_WINDOWS)
BOOL Debug_IsLockedForRead();
BOOL Debug_IsLockedForWrite();
#endif //_DEBUG
#endif // defined(_DEBUG) && defined(HOST_WINDOWS)

private:
#ifdef HOST_WINDOWS
Volatile<ULONG> m_dwFlag; // internal state, see implementation
HANDLE m_hReadWaiterSemaphore; // semaphore for awakening read waiters
HANDLE m_hWriteWaiterEvent; // event for awakening write waiters
#else // HOST_WINDOWS
bool m_initialized;
pthread_rwlock_t m_rwLock;
Copy link
Member

@jkotas jkotas May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does performance of pthread_rwlock_t compare to the current implementation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plugged UTSemReadWrite into corerun and some basic tests. The time cost for 1 million loops of single-thread lock-unlock under WSL:

PAL implementation: Read lock: 12350 us Write lock: 12379 us
pthreads implementation: Read lock: 11678 us Write lock: 15976 us

For reference, under Windows: Read lock: 13450 us Write lock: 12793 us

#endif // HOST_WINDOWS
}; // class UTSemReadWrite

#endif // __UTSEM_H__
2 changes: 2 additions & 0 deletions src/coreclr/md/enc/metamodelrw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7144,8 +7144,10 @@ CMiniMdRW::ValidateVirtualSortAfterAddRecord(
void
CMiniMdRW::Debug_CheckIsLockedForWrite()
{
#ifdef HOST_WINDOWS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an appropriate limitation. There should be nothing Windows specific about this code path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find a way to test whether a pthread_rwlock_t is held.

// If this assert fires, then we are trying to modify MetaData that is not locked for write
_ASSERTE((dbg_m_pLock == NULL) || dbg_m_pLock->Debug_IsLockedForWrite());
#endif // HOST_WINDOWS
}

#endif //_DEBUG
Expand Down
45 changes: 39 additions & 6 deletions src/coreclr/utilcode/utsem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,14 @@ UTSemReadWrite::UTSemReadWrite()
}
#endif //SELF_NO_HOST

#ifdef HOST_WINDOWS
m_dwFlag = 0;
m_hReadWaiterSemaphore = NULL;
m_hWriteWaiterEvent = NULL;
#else // HOST_WINDOWS
m_initialized = false;
memset(&m_rwLock, 0, sizeof(pthread_rwlock_t));
#endif // HOST_WINDOWS
}


Expand All @@ -139,14 +144,19 @@ UTSemReadWrite::~UTSemReadWrite()
GC_NOTRIGGER;
}
CONTRACTL_END;


#ifdef HOST_WINDOWS
_ASSERTE_MSG((m_dwFlag == (ULONG)0), "Destroying a UTSemReadWrite while in use");

if (m_hReadWaiterSemaphore != NULL)
CloseHandle(m_hReadWaiterSemaphore);

if (m_hWriteWaiterEvent != NULL)
CloseHandle(m_hWriteWaiterEvent);
#else // HOST_WINDOWS
if (m_initialized)
pthread_rwlock_destroy(&m_rwLock);
#endif // HOST_WINDOWS
}

//=======================================================================================
Expand All @@ -163,7 +173,8 @@ UTSemReadWrite::Init()
}
CONTRACTL_END;



#ifdef HOST_WINDOWS
_ASSERTE(m_hReadWaiterSemaphore == NULL);
_ASSERTE(m_hWriteWaiterEvent == NULL);

Expand All @@ -172,6 +183,10 @@ UTSemReadWrite::Init()

m_hWriteWaiterEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
IfNullRet(m_hWriteWaiterEvent);
#else // HOST_WINDOWS
pthread_rwlock_init(&m_rwLock, nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle failures?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, but unsure about what to do on failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same way as the current code - return HRESULT.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading through the manpage, it's unclear to me that when a writer is holding the lock, whether a reader will be blocked or returned with EBUSY. Simply returning E_FAIL under any failure?

m_initialized = true;
#endif // HOST_WINDOWS

return S_OK;
} // UTSemReadWrite::Init
Expand All @@ -195,6 +210,7 @@ HRESULT UTSemReadWrite::LockRead()
// holding this lock.
IncCantStopCount();

#ifdef HOST_WINDOWS
// First do some spinning - copied from file:..\VM\crst.cpp#CrstBase::SpinEnter
for (DWORD iter = 0; iter < g_SpinConstants.dwRepetitions; iter++)
{
Expand Down Expand Up @@ -261,6 +277,10 @@ HRESULT UTSemReadWrite::LockRead()
ReadLockAcquired:
_ASSERTE ((m_dwFlag & READERS_MASK) != 0 && "reader count is zero after acquiring read lock");
_ASSERTE ((m_dwFlag & WRITERS_MASK) == 0 && "writer count is nonzero after acquiring write lock");
#else // HOST_WINDOWS
pthread_rwlock_rdlock(&m_rwLock);
#endif // HOST_WINDOWS

EE_LOCK_TAKEN(this);

return S_OK;
Expand All @@ -286,7 +306,8 @@ HRESULT UTSemReadWrite::LockWrite()
// Inform CLR that the debugger shouldn't suspend this thread while
// holding this lock.
IncCantStopCount();


#ifdef HOST_WINDOWS
// First do some spinning - copied from file:..\VM\crst.cpp#CrstBase::SpinEnter
for (DWORD iter = 0; iter < g_SpinConstants.dwRepetitions; iter++)
{
Expand Down Expand Up @@ -350,6 +371,10 @@ HRESULT UTSemReadWrite::LockWrite()
WriteLockAcquired:
_ASSERTE ((m_dwFlag & READERS_MASK) == 0 && "reader count is nonzero after acquiring write lock");
_ASSERTE ((m_dwFlag & WRITERS_MASK) == WRITERS_INCR && "writer count is not 1 after acquiring write lock");
#else // HOST_WINDOWS
pthread_rwlock_wrlock(&m_rwLock);
#endif // HOST_WINDOWS

EE_LOCK_TAKEN(this);

return S_OK;
Expand All @@ -371,6 +396,7 @@ void UTSemReadWrite::UnlockRead()
}
CONTRACTL_END;

#ifdef HOST_WINDOWS
ULONG dwFlag;


Expand Down Expand Up @@ -416,6 +442,9 @@ void UTSemReadWrite::UnlockRead()
}
}
}
#else // HOST_WINDOWS
pthread_rwlock_unlock(&m_rwLock);
#endif // HOST_WINDOWS

DecCantStopCount();
EE_LOCK_RELEASED(this);
Expand All @@ -435,7 +464,8 @@ void UTSemReadWrite::UnlockWrite()
GC_NOTRIGGER;
}
CONTRACTL_END;


#ifdef HOST_WINDOWS
ULONG dwFlag;
ULONG count;

Expand Down Expand Up @@ -480,12 +510,15 @@ void UTSemReadWrite::UnlockWrite()
}
}
}
#else // HOST_WINDOWS
pthread_rwlock_unlock(&m_rwLock);
#endif // HOST_WINDOWS

DecCantStopCount();
EE_LOCK_RELEASED(this);
} // UTSemReadWrite::UnlockWrite

#ifdef _DEBUG
#if defined(_DEBUG) && defined(HOST_WINDOWS)

//=======================================================================================
BOOL
Expand All @@ -501,5 +534,5 @@ UTSemReadWrite::Debug_IsLockedForWrite()
return ((m_dwFlag & WRITERS_MASK) != 0);
}

#endif //_DEBUG
#endif // defined(_DEBUG) && defined(HOST_WINDOWS)

Loading