Skip to content
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.

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

Encourage use of .NET 9.0's System.Threading.Lock #3601

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MarkCiliaVincenti
Copy link

Will improve performance once the library starts targeting .NET 9.0 as locking on System.Threading.Lock is ~25% more performant over locking on an object. Newly-added micro dependency allows backported use without affecting performance.

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal,

bUnit-dev/bUnit#1532 has a discussion about this which is relevant for NHibernate too.

For the simple usages we currently have (excepted the Monitor one which would have to be slightly different), using something like:

#if NET_9_0_OR_GREATER
private readonly Lock syncLock = new();
#else
private readonly object syncLock = new();
#endif

Seems enough and would avoid adding a new dependency for users to investigate if they want it in their project or not.

@@ -6,7 +6,7 @@
<VersionPatch Condition="'$(VersionPatch)' == ''">0</VersionPatch>
<!-- Clear VersionSuffix for making release and set it to dev for making development builds -->
<VersionSuffix Condition="'$(VersionSuffix)' == ''">dev</VersionSuffix>
<LangVersion Condition="'$(MSBuildProjectExtension)' != '.vbproj'">12.0</LangVersion>
<LangVersion Condition="'$(MSBuildProjectExtension)' != '.vbproj'">preview</LangVersion>
Copy link
Member

Choose a reason for hiding this comment

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

So, it needs language version 14.0? We will likely wait for it before merging your proposal, if we accept it.

Copy link
Member

Choose a reason for hiding this comment

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

I have not spotted what does require such a change. Why this change?

Copy link
Author

Choose a reason for hiding this comment

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

It's 13.0 as such, and comes with .NET 9.0, but for now you need to set the LangVersion to preview on .NET 9.0 in order to lock on a System.Threading.Lock object. The expectation is that this changes as soon as .NET 9.0 is released.

src/NHibernate.Test/NHSpecificTest/NH2030/Fixture.cs Outdated Show resolved Hide resolved
src/NHibernate/Cache/SyncCacheLock.cs Outdated Show resolved Hide resolved
src/NHibernate.Test/NHSpecificTest/NH2192/Fixture.cs Outdated Show resolved Hide resolved
@@ -73,6 +73,10 @@
<PackageReference Include="Microsoft.CSharp" Version="4.7.0" />
</ItemGroup>

<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))">
<PackageReference Include="Backport.System.Threading.Lock" Version="1.1.6" />
Copy link
Member

Choose a reason for hiding this comment

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

So, that is this Nuget package, coming from your repository.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that is correct.

@MarkCiliaVincenti
Copy link
Author

Thanks for the proposal,

bUnit-dev/bUnit#1532 has a discussion about this which is relevant for NHibernate too.

For the simple usages we currently have (excepted the Monitor one which would have to be slightly different), using something like:

#if NET_9_0_OR_GREATER
private readonly Lock syncLock = new();
#else
private readonly object syncLock = new();
#endif

Seems enough and would avoid adding a new dependency for users to investigate if they want it in their project or not.

Yes, it is very relevant.
bUnit-dev/bUnit#1532 (comment) in particular

Using the code you pasted above will give "CS9216: A value of type System.Threading.Lock converted to a different type will use likely unintended monitor-based locking in lock statement." when you try using it in MonitorLock / InternalLock if you do it globally, plus you won't get the performance benefits of .NET 9.0 on that class if you keep using Monitor.

@MarkCiliaVincenti
Copy link
Author

As for your concern regarding the additional dependency, I understand, but what we can do is copy the code into NHibernate and just put in a comment at the top giving credit. It's not ideal, but if the dependency is a deal-breaker for you we can do that instead.

@hazzik
Copy link
Member

hazzik commented Sep 5, 2024

Thanks for the contribution. It is unlikely we'll be targeting .NET 9 version as it is not LTS. Also, we would like to avoid an external dependency.

@MarkCiliaVincenti
Copy link
Author

Thanks for the contribution. It is unlikely we'll be targeting .NET 9 version as it is not LTS. Also, we would like to avoid an external dependency.

We could copy the code in. But if you don't intend on targeting .NET 9 that's another story because this would have to wait until .NET 10.

@MarkCiliaVincenti
Copy link
Author

@fredericDelaporte a new version of Backport.System.Threading.Lock has come out that acts as a source generator and basically dropping the DLL as a dependency. If this new development makes you reconsider, I will amend this PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants