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.

src/coreclr/classlibnative/bcltype/objectnative.cpp

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

AaronRobinsonMSFT · 2025-03-24T18:15:12Z

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

+            Yield = 2,
+            Contention = 3,
+            Error = 4,
+        };


Suggested change

};

}

AaronRobinsonMSFT · 2025-03-24T18:16:46Z

src/coreclr/classlibnative/bcltype/objectnative.cpp

+    if (obj->TryEnterObjMonitorSpinHelper())
+    {
+        FC_RETURN_BOOL(TRUE);
+    }
+    else
+    {
+        FC_RETURN_BOOL(FALSE);
+    }


Suggested change

if (obj->TryEnterObjMonitorSpinHelper())

{

FC_RETURN_BOOL(TRUE);

}

else

{

FC_RETURN_BOOL(FALSE);

}

FC_RETURN_BOOL(obj->TryEnterObjMonitorSpinHelper());

AaronRobinsonMSFT · 2025-03-24T18:49:19Z

src/coreclr/classlibnative/bcltype/objectnative.cpp

+    if (timeOut < -1)
+        COMPlusThrow(kArgumentOutOfRangeException);


I think we should assert this here and do the error handling in managed code. The Copilot for the methods also called this out.

AaronRobinsonMSFT · 2025-03-24T18:53:46Z

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

+                if (exitBehavior == LeaveHelperAction.None)
+                {
+                    lockTaken = false;
+                    return;
+                }
+
+                Exit_Slowpath(exitBehavior, obj);
+                lockTaken = false;


Suggested change

if (exitBehavior == LeaveHelperAction.None)

{

lockTaken = false;

return;

}

Exit_Slowpath(exitBehavior, obj);

lockTaken = false;

if (exitBehavior != LeaveHelperAction.None)

{

Exit_Slowpath(exitBehavior, obj);

}

lockTaken = false;

ghost added the area-VM-coreclr label Mar 7, 2025

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

Refactoring oops

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

Closed

25 tasks

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

Copilot AI 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

66fa763

Fix thinko where the slow path of monitor exit was not hooked up corr…

82a4d2e

…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 reviewed Mar 18, 2025

View reviewed changes

src/coreclr/classlibnative/bcltype/objectnative.cpp Show resolved Hide resolved

Address code review feedback

5a31471

AaronRobinsonMSFT reviewed Mar 18, 2025

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs Outdated Show resolved Hide resolved

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

d42f4ec

Handle TryEnter contention scenarios correctly

e1199d5

Fix build break

426b8ff

Produce exactly the same exceptions as before, and fix formatting issue

3d942ff

davidwrighton marked this pull request as ready for review March 24, 2025 18:09

davidwrighton requested a review from AaronRobinsonMSFT March 24, 2025 18:09

AaronRobinsonMSFT reviewed Mar 24, 2025

View reviewed changes

AaronRobinsonMSFT approved these changes Mar 24, 2025

View reviewed changes

davidwrighton merged commit 76750df into dotnet:main Mar 24, 2025
108 checks passed

am11 mentioned this pull request Mar 24, 2025

Benchmarks for #113857 (davidwrighton) EgorBot/runtime-utils#326

Open

LoopedBard3 mentioned this pull request Apr 1, 2025

[Perf] Linux/x64: 8 Regressions on 3/24/2025 9:53:02 PM +00:00 #114112

Closed

This was referenced Apr 1, 2025

[Perf] Linux/x64: 17 Improvements on 3/24/2025 9:53:02 PM +00:00 dotnet/perf-autofiling-issues#52759

Closed

[Perf] Windows/x64: 3 Improvements on 3/24/2025 9:53:02 PM +00:00 dotnet/perf-autofiling-issues#52735

Closed

github-actions bot locked and limited conversation to collaborators Apr 24, 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));
+            }

		if (timeOut < -1)
		COMPlusThrow(kArgumentOutOfRangeException);

Conversation

davidwrighton commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Mar 7, 2025

Uh oh!

davidwrighton commented Mar 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

Copilot AI Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Mar 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

davidwrighton commented Mar 7, 2025 •

edited

Loading