Cleanup semaphore usage in utilcode

[huoyaoyuan]

CreateSemaphore is only used in one place, replaced with pthread_rwlock_t instead.

Mutex and Event are used in a bit more places, thus not included in the same PR.

[jkotas]

I do not think that this is going to work. These Windows-simulating CoreCLR PAL APIs are used to implement managed System.Threading.Semaphore and friends on non-Windows. The main pre-requisite for cleaning up these Windows-simulating PAL APIs is to switch managed System.Threading.Semaphore and friends to managed WaitSubSystem. It requires factoring out support for named semaphores from the CoreCLR PAL. I know @jkoritzinsky has been thinking about what it would take.

[jkoritzinsky]

Yeah this PR isn't going to work as-is because the Semaphore's underlying handle needs to be a SafeWaitHandle, which on non-Windows is a PAL wait handle. We can't have two separate implementations here. The way forward is to use the managed wait subsystem, but we need to determine how to handle features that aren't supported there (named semaphores, named mutexes, anything cross-process) before we can move to that.

The changes in utilcode to use pthreads can be done, but we can't remove the pal implementation yet.

[huoyaoyuan]

These Windows-simulating CoreCLR PAL APIs are used to implement managed System.Threading.Semaphore and friends on non-Windows.

I do find such usage for Mutex, but not Semaphore.

I can see "can't find QCall" in the test failures now.

OK, I see the Interop.Semaphore.cs is included unconditionally in coreclr.

[huoyaoyuan]

but we need to determine how to handle features that aren't supported there (named semaphores, named mutexes, anything cross-process) before we can move to that.

Apparently named semaphore isn't supported in PAL implementation either:

runtime/src/coreclr/pal/src/synchobj/semaphore.cpp

Lines 179 to 184 in ab105b5

	if (lpName != nullptr)
	{
	ASSERT("lpName: Cross-process named objects are not supported in PAL");
	palError = ERROR_NOT_SUPPORTED;
	goto InternalCreateSemaphoreExit;
	}

runtime/src/coreclr/pal/src/synchobj/semaphore.cpp

Lines 468 to 478 in ab105b5

	/* validate parameters */
	if (lpName == nullptr)
	{
	ERROR("lpName is NULL\n");
	palError = ERROR_INVALID_PARAMETER;
	}
	else
	{
	ASSERT("lpName: Cross-process named objects are not supported in PAL");
	palError = ERROR_NOT_SUPPORTED;
	}

Am I missing anything?

[jkotas]

It requires factoring out support for named semaphores from the CoreCLR PAL.

I meant to say named mutexes. Named mutexes are supported by CoreCLR PAL.

All PAL synchronization primitives (semaphores, mutexes, events) return opaque HANDLEs that can be used interchangeably and passed to PAL methods like WaitForMultipleObjects. It means it is not possible to switch these synchronization primitives to use the managed WaitSubSystem one at a time.

[huoyaoyuan]

All PAL synchronization primitives (semaphores, mutexes, events) return opaque HANDLEs that can be used interchangeably and passed to PAL methods like WaitForMultipleObjects.

Yes, I realized this after seeing the QCalls.

Then I think it's better to do a minor refactor first and investigate other synchronization primitives.

[jkotas]

Do we need to handle failures?

[huoyaoyuan]

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

[jkotas]

Same way as the current code - return HRESULT.

[huoyaoyuan]

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?

[jkotas]

How does performance of pthread_rwlock_t compare to the current implementation?

[huoyaoyuan]

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

[AaronRobinsonMSFT]

I don't think this is a good use of time. This is merely sprinkling compile time branchs in more places and not improving the actual situation. We should be focusing on simpler abstractions in src/native/minipal or removing native code and elevating it to managed. I don't see a lot of value in this current PR.

[AaronRobinsonMSFT]

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

[huoyaoyuan]

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

[AaronRobinsonMSFT]

If this needed, we should add a new abstraction in the minipal, but given the current changes in this PR it doesn't seem appropriate without a much larger change.

[huoyaoyuan]

We should be focusing on simpler abstractions in src/native/minipal or removing native code and elevating it to managed. I don't see a lot of value in this current PR.

Yes, the initial attempt was to remove the pal implementation of semaphore, but as #115685 (comment), all synchronization primitives have to be switched at the same time.

The UTSemReadWrite lock is only used in R/W metadata model, which should also be deprecated if we adopt DNMD as new metadata model.

[huoyaoyuan]

Closing since it would be unnecessary if we switch to new implementation of metadata. Performance is recorded at #115685 (comment)