Make Condition to not use ConditionalWeakTable#129083
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors how Monitor.Wait/Pulse are implemented by removing the ConditionalWeakTable<object, Condition> mapping and instead associating a Condition directly with the managed Lock used by a sync block. It also updates diagnostics code (DAC/DBI) to discover monitor waiters via the Lock rather than via Monitor’s static table, and updates ManualResetEventSlim to use Lock for its wait/pulse coordination.
Changes:
- Replace
Monitor’s object→Conditiontable withLock-basedWait/Pulse/PulseAllhelpers. - Update
Lockto store either anAutoResetEventor aConditionin a single field and lazily create theConditionwhen needed. - Update
ManualResetEventSlimto useLockand revise waiter-count/state handling; update DAC monitor-wait enumeration accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Threading/Monitor.cs | Routes Wait/Pulse/PulseAll through the sync-block Lock instead of a global ConditionalWeakTable. |
| src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs | Switches internal wait/pulse lock to Lock and rewrites signaled/waiter state manipulation. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs | Introduces _waitEventOrCondition union field and adds internal Wait/Pulse/PulseAll via Condition. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Condition.cs | Refactors waiter bookkeeping and signaling strategy; adds storage for a Lock’s wait event. |
| src/coreclr/vm/corelib.h | Updates binder field list to remove Monitor.s_conditionTable and add Lock._waitEventOrCondition. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Changes monitor-wait enumeration to locate Condition via the sync-block Lock field. |
| using (mre.m_lock.EnterScope()) | ||
| { | ||
| int oldState = m_combinedState; // cache the old value for testing in CAS | ||
|
|
||
| // Procedure:(1) zero the updateBits. eg oldState = [11111111] flag= [00111000] newState = [11000111] | ||
| // then (2) map in the newBits. eg [11000111] newBits=00101000, newState=[11101111] | ||
| int newState = (oldState & ~updateBitsMask) | newBits; | ||
|
|
||
| if (Interlocked.CompareExchange(ref m_combinedState, newState, oldState) == oldState) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| sw.SpinOnce(sleep1Threshold: -1); | ||
| mre.m_lock.PulseAll(); // awaken all waiters | ||
| } |
There was a problem hiding this comment.
This would not be a new issue for ManualResetEventSlim. The previous implementation would do the same just via ConditionalWeakTable indirection.
On the other hand making disposal of the m_lock more eager has a small potential of breaking some existing code that, for example, disposes an ManualResetEventSlim while some waiters are still waking up.
|
Is there a micro-benchmark that demonstrates the improvement? |
Main motivation is that
ManualResetEventSlimusesMonitor.Waitto implementWaitthat is:Thread.Interruptsense) andMonitor.Waitis a good fit to implement such pattern and should generally perform well enough.ManualResetEventSlimin turn is used inTask.Waitand some scenarios can wait on Tasks relatively frequently.In such scenarios using
ConditionalWaitTablefor Lock->Condition association may have two inconveniences:it may result in quite a few dependent handles being alive and that can have impact on GC.
In particular because dependent handles currently do not age and need to be revisited in every Gen0, even if both objects referred from the handle may be Gen2 objects.
(we should probably address Consider aging dependent handles the same way as the other kinds of handles. #79062, regardless of this PR)
Allocating an entry in
ConditionalWaitTableacquires table-wide lock and on large enough core count may contend.It seems there is a relatively simple way to arrange Lock->Condition link without involving
ConditionalWaitTable, thus why not.The change also enables
Wait/Pulse/PulseAllfunctionality onLock, but only internally.It can be exposed as a public API, but it would be a separate discussion.