Skip to content

Remove RequiresPreviewFeatures attribute on Lock, add test using the lock keyword #102222

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

Merged
merged 1 commit into from
May 15, 2024
Merged

Remove RequiresPreviewFeatures attribute on Lock, add test using the lock keyword #102222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions .../nativeaot/System.Private.DisabledReflection/src/System.Private.DisabledReflection.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
<PropertyGroup>
<!-- we access a lot of internals of System.Private.CoreLib, disable compiling against the ref assembly for now so we don't need to update it -->
<CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies>
<!-- CA2252: Opt in to preview features before using them (Lock) -->
<EnablePreviewFeatures>true</EnablePreviewFeatures>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\System.Private.CoreLib\src\System.Private.CoreLib.csproj" />
Expand Down
2 changes: 0 additions & 2 deletions ...iveaot/System.Private.Reflection.Execution/src/System.Private.Reflection.Execution.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
<NativeFormatCommonPath>$(CompilerCommonPath)\Internal\NativeFormat</NativeFormatCommonPath>
<!-- we access a lot of internals of System.Private.CoreLib, disable compiling against the ref assembly for now so we don't need to update it -->
<CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies>
<!-- CA2252: Opt in to preview features before using them (Lock) -->
<EnablePreviewFeatures>true</EnablePreviewFeatures>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(NativeFormatCommonPath)\NativeFormat.cs" />
Expand Down
2 changes: 0 additions & 2 deletions .../nativeaot/System.Private.StackTraceMetadata/src/System.Private.StackTraceMetadata.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
<PropertyGroup>
<!-- we access a lot of internals of System.Private.CoreLib, disable compiling against the ref assembly for now so we don't need to update it -->
<CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies>
<!-- CA2252: Opt in to preview features before using them (Lock) -->
<EnablePreviewFeatures>true</EnablePreviewFeatures>
</PropertyGroup>

<ItemGroup>
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/nativeaot/System.Private.TypeLoader/src/System.Private.TypeLoader.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
<DefineConstants Condition="'$(GENERICS_FORCE_USG)' != ''">GENERICS_FORCE_USG;$(DefineConstants)</DefineConstants>
<!-- we access a lot of internals of System.Private.CoreLib, disable compiling against the ref assembly for now so we don't need to update it -->
<CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies>
<!-- CA2252: Opt in to preview features before using them (Lock) -->
<EnablePreviewFeatures>true</EnablePreviewFeatures>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\System.Private.CoreLib\src\System.Private.CoreLib.csproj" />
Expand Down
2 changes: 0 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
<SharedGUID>c5ed3c1d-b572-46f1-8f96-522a85ce1179</SharedGUID>
<StringResourcesName Condition="'$(StringResourcesName)' == ''">System.Private.CoreLib.Strings</StringResourcesName>
<GenerateResourcesSubstitutions>true</GenerateResourcesSubstitutions>
<!-- CA2252: Opt in to preview features before using them (for System.Threading.Lock) -->
<EnablePreviewFeatures>true</EnablePreviewFeatures>
</PropertyGroup>
<PropertyGroup Label="Configuration">
<Import_RootNamespace />
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ namespace System.Threading
/// that holds a lock may enter the lock repeatedly without exiting it, such as recursively, in which case the thread should
/// eventually exit the lock the same number of times to fully exit the lock and allow other threads to enter the lock.
/// </remarks>
[Runtime.Versioning.RequiresPreviewFeatures]
public sealed partial class Lock
{
private const short DefaultMaxSpinCount = 22;
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15158,7 +15158,6 @@ public enum LazyThreadSafetyMode
PublicationOnly = 1,
ExecutionAndPublication = 2,
}
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute]
Copy link
Member

Choose a reason for hiding this comment

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

Are we ready to start using Lock in runtime? Would the general philosophy be to use it anywhere we're currently using a dedicated lock object and it's only being used for enter/exit and not signaling?

Copy link
Member

Choose a reason for hiding this comment

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

yeah we were planning to initially start using it in Threadpool synchronization logic. But might be good to start using in few other places too, so we can iron out issues (if any) before going broader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea we were planning on using it in a few places relevant to the thread pool to start with. Those locks are used frequently but aren't very contentious. It is already used in NativeAOT, wanted to get some more coverage in CoreCLR as well. It should be ready to be used anywhere Monitor is used currently just as a lock. There are some SOS diagnostics to add, but it should be fine to update existing uses.

Copy link
Member

Choose a reason for hiding this comment

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

Perf wise, how should I think about it? It'll be as-good-or-better in pretty much any scenario, such that I can use it without doing significant perf validation, or it's more nuanced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lock perf was >= perf of Monitor in pretty much any case I tried (with EnterScope or lock keyword). It is a bit nuanced if Enter/Exit are used instead, as the thread-local access is more expensive and there are other code gen issues that can make it a bit slower in some cases. Even with Enter/Exit, it really depends on what is done inside the lock and how good the processor is at prefetching, and in most cases the perf was still equal or better.

public sealed partial class Lock
{
public Lock() { }
Expand Down
39 changes: 39 additions & 0 deletions src/libraries/System.Threading/tests/LockTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,37 @@ public static class LockTests
{
private const int FailTimeoutMilliseconds = 30000;

#pragma warning disable CS9216 // casting Lock to object
[Fact]
public static void LockStatementWithLockVsMonitor()
{
Lock lockObj = new();
lock (lockObj)
{
Assert.True(lockObj.IsHeldByCurrentThread);
Assert.False(Monitor.IsEntered(lockObj));
}

lock ((object)lockObj)
{
Assert.False(lockObj.IsHeldByCurrentThread);
Assert.True(Monitor.IsEntered(lockObj));
}

LockOnTWhereTIsLock(lockObj);

static void LockOnTWhereTIsLock<T>(T lockObj) where T : class
{
Assert.IsType<Lock>(lockObj);
lock (lockObj)
{
Assert.False(((Lock)(object)lockObj).IsHeldByCurrentThread);
Assert.True(Monitor.IsEntered(lockObj));
}
}
}
#pragma warning restore CS9216

// Attempts a single recursive acquisition/release cycle of a newly-created lock.
[Fact]
public static void BasicRecursion()
Expand All @@ -27,6 +58,10 @@ public static void BasicRecursion()
{
Assert.True(lockObj.IsHeldByCurrentThread);
}
lock (lockObj)
{
Assert.True(lockObj.IsHeldByCurrentThread);
}
Assert.True(lockObj.IsHeldByCurrentThread);
lockObj.Exit();
Assert.False(lockObj.IsHeldByCurrentThread);
Expand Down Expand Up @@ -64,6 +99,10 @@ public static void IsHeldByCurrentThread()
{
Assert.True(lockObj.IsHeldByCurrentThread);
}
lock (lockObj)
{
Assert.True(lockObj.IsHeldByCurrentThread);
}
Assert.False(lockObj.IsHeldByCurrentThread);
}

Expand Down
2 changes: 0 additions & 2 deletions src/libraries/System.Threading/tests/System.Threading.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<!-- CA2252: Opt in to preview features before using them (Lock) -->
<EnablePreviewFeatures>true</EnablePreviewFeatures>
</PropertyGroup>
<PropertyGroup Condition="'$(TargetOS)' == 'browser'">
<XunitShowProgress>true</XunitShowProgress>
Expand Down
Loading