Remove helper method frames from Monitors #113242

davidwrighton · 2025-03-07T01:22:23Z

Convert to the FCALL fast path/QCALL slow path approach
Make EnterHelperResult/LeaveHelperAction into enum class so that they can safely be silently marshaled between native and managed
Move the lockTaken flag handling to managed code so we can share more helpers
Benchmarking indicates that this makes uncontended locks somewhere around 10% faster. I believe this is primarily due to removing a small amount of handling around error checking and lock flag usage which isn't always needed.

- Convert to the FCALL fast path/QCALL slow path approach - Make EnterHelperResult/LeaveHelperAction into enum class so that they can safely be silently marshaled between native and managed - Move the lockTaken flag handling to managed code so we can share more helpers

dotnet-policy-service · 2025-03-07T01:23:01Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

davidwrighton · 2025-03-07T17:33:42Z

@EgorBot -amd -arm -windows_intel

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Numerics;

BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

public class Tests
{
    private obj _lockObj = new object();
    private int _field = 0;

    [Benchmark(Baseline = true)]
    public string Lock_OnLockObj()
    {
        _field++;
        lock(_lockObj)
        {
            _field++;
        }
    }

    [Benchmark]
    public bool LockOnThis_ThisKnownToBeNonNull()
    {
        _field++;
        lock(this)
        {
            _field++;
        }
    }

    [Benchmark]
    public bool LockOnThis_ThisKnownToBeNonNull()
    {
        lock(this)
        {
            _field++;
        }
    }
}

Copilot

PR Overview

This PR refactors the Monitor implementation to remove helper method frames by introducing a fast path/slow path approach for lock acquisition and release. Key changes include:

Introducing new fast path methods (TryEnter_FastPath and TryEnter_FastPath_WithTimeout) and enumerated types (EnterHelperResult, LeaveHelperAction) for better handling of lock acquisition.
Replacing the ReliableEnter/Exit methods with new logic for handling both immediate and timeout-based lock acquisition.
Migrating lockTaken flag handling to managed code and updating related methods (Enter, TryEnter, and Exit) accordingly.

Reviewed Changes

File	Description
src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs	Refactors lock acquisition and release by adding fast path/slow path methods and replacing old helper methods, while updating argument validation and exception handling

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Copilot · 2025-03-07T18:20:27Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs

-            TryEnter(obj, millisecondsTimeout, ref lockTaken);
-            return lockTaken;
+            ArgumentNullException.ThrowIfNull(obj);
+


The TryEnter(object, int) method does not explicitly validate that millisecondsTimeout is not less than -1. To maintain the documented contract, consider adding a check to throw ArgumentException for invalid timeout values.

Suggested change

if (millisecondsTimeout < -1)

{

throw new ArgumentException("Timeout must be greater than or equal to -1.", nameof(millisecondsTimeout));

}

Copilot · 2025-03-07T18:20:27Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs

+        }
+
+        private static void TryEnter_Timeout_WithLockTaken(object obj, int timeout, ref bool lockTaken)
+        {


The TryEnter_Timeout_WithLockTaken method does not handle cases where timeout is less than -1. It is recommended to add an explicit check and throw an ArgumentException when timeout is invalid.

Suggested change

{

{

if (timeout < -1)

{

throw new ArgumentException("Timeout must be greater than or equal to -1.", nameof(timeout));

}

davidwrighton · 2025-03-08T00:43:35Z

Sigh. Looks like something is busted. I'd like to finish looking into that before anyone reviews this.

IDisposable · 2025-03-12T21:04:58Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs

+                    return;
+                }
+
+                Exit_Slowpath(obj, exitBehavior);


Can Exit_FastPath do anything when returning exitBehavior != LeaveHelperAction.None (note NOT EQUAL) that would break the subsequent fall-through to Exit_Slowpath (e.g. we did the fast-path exit, but it needs some other LeaveHelperAction... but we've done the Exit already)?

(same question on line 154)

IDisposable · 2025-03-12T21:06:09Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs

+            if (exitBehavior == LeaveHelperAction.None)
+                return;
+
+            Exit_Slowpath(obj, exitBehavior);


Can Exit_FastPath do anything when returning exitBehavior != LeaveHelperAction.None (note NOT EQUAL) that would break the subsequent fall-through to Exit_Slowpath (e.g. we did the fast-path exit, but it needs some other LeaveHelperAction... but we've done the Exit already)?

(same question on line 174)

IDisposable · 2025-03-12T21:08:54Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs

+                }
+            }
+
+            if (TryEnter_Slowpath(obj, timeout))


Will this result in up to two timeout periods elapsing if the fast-path tried and returned anything other than Entered or Contention?

No, the timeout input to the fast path is only used to check against 0 (which has special meaning).

IDisposable · 2025-03-12T21:13:49Z

src/coreclr/classlibnative/bcltype/objectnative.cpp

+            return AwareLock::EnterHelperResult::Contention;
+        }
+
+        result = obj->EnterObjMonitorHelperSpin(pCurThread);


Should a timeOut of non-zero be handled, should it be passed down to/honored by the EnterObjMonitorHelperSpin?

No, this is the fast set of spinning that waits for fractions of a millisecond or so, if you're willing to wait at all, we run the same amount of spinning for all possible timeouts.

…ectly. Also hard-code the exact values for the Enter/Leave results on the native side so that the values are well defined instead of implicit.

AaronRobinsonMSFT · 2025-03-18T23:19:35Z

src/coreclr/classlibnative/bcltype/objectnative.cpp

+{
+    QCALL_CONTRACT;
+
+    GCX_COOP();


QCalls need BEGIN_QCALL and END_QCALL if exceptions will be thrown.

AaronRobinsonMSFT · 2025-03-18T23:35:40Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs

+        private enum LeaveHelperAction {
+            None = 0,
+            Signal = 1,
+            Yield = 2,
+            Contention = 3,
+            Error = 4,
+        };


Suggested change

private enum LeaveHelperAction {

None = 0,

Signal = 1,

Yield = 2,

Contention = 3,

Error = 4,

};

private enum LeaveHelperAction

{

None = 0,

Signal = 1,

Yield = 2,

Contention = 3,

Error = 4,

}

dotnet-issue-labeler bot added the area-VM-coreclr label Mar 7, 2025

dotnet-policy-service bot assigned davidwrighton Mar 7, 2025

Refactoring oops

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

Loading
Loading status checks…

5ba5cf7

EgorBot mentioned this pull request Mar 7, 2025

EgorBot for davidwrighton in #113242 EgorBot/runtime-utils#311

Open

davidwrighton mentioned this pull request Feb 10, 2025

Convert all FCalls with HELPER_METHOD_FRAME to QCalls #95695

Open

31 tasks

davidwrighton marked this pull request as ready for review March 7, 2025 18:19

Copilot bot review requested due to automatic review settings March 7, 2025 18:19

Copilot AI reviewed Mar 7, 2025

View reviewed changes

davidwrighton marked this pull request as draft March 8, 2025 00:43

IDisposable reviewed Mar 12, 2025

View reviewed changes

davidwrighton added 2 commits March 17, 2025 17:10

Merge branch 'main' into hmf_monitor_removal

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

Loading
Loading status checks…

66fa763

AaronRobinsonMSFT reviewed Mar 18, 2025

View reviewed changes

Address code review feedback

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

Loading
Loading status checks…

5a31471

AaronRobinsonMSFT reviewed Mar 18, 2025

View reviewed changes

build-analysis bot mentioned this pull request Mar 19, 2025

Linux x64 machine running out of space #113652

Closed

davidwrighton added 3 commits March 19, 2025 11:47

Fix missing ThrowIfNull

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

Loading
Loading status checks…

d42f4ec

Handle TryEnter contention scenarios correctly

Loading
Loading status checks…

e1199d5

Fix build break

Loading
Loading status checks…

426b8ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove helper method frames from Monitors #113242

Remove helper method frames from Monitors #113242

davidwrighton commented Mar 7, 2025 •

edited

Loading

dotnet-policy-service bot commented Mar 7, 2025

davidwrighton commented Mar 7, 2025

Copilot AI left a comment

Copilot AI Mar 7, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Mar 7, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

davidwrighton commented Mar 8, 2025

IDisposable Mar 12, 2025

davidwrighton Mar 18, 2025

IDisposable Mar 12, 2025

davidwrighton Mar 18, 2025

IDisposable Mar 12, 2025

davidwrighton Mar 18, 2025

IDisposable Mar 12, 2025

davidwrighton Mar 18, 2025

AaronRobinsonMSFT Mar 18, 2025

AaronRobinsonMSFT Mar 18, 2025

+            if (millisecondsTimeout < -1)
+            {
+                throw new ArgumentException("Timeout must be greater than or equal to -1.", nameof(millisecondsTimeout));
+            }

-        {
+        {
+            if (timeout < -1)
+            {
+                throw new ArgumentException("Timeout must be greater than or equal to -1.", nameof(timeout));
+            }

Remove helper method frames from Monitors #113242

Are you sure you want to change the base?

Remove helper method frames from Monitors #113242

Conversation

davidwrighton commented Mar 7, 2025 • edited Loading

dotnet-policy-service bot commented Mar 7, 2025

davidwrighton commented Mar 7, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Copilot AI Mar 7, 2025

Choose a reason for hiding this comment

Copilot AI Mar 7, 2025

Choose a reason for hiding this comment

davidwrighton commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwrighton commented Mar 7, 2025 •

edited

Loading