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

Make use of more performant System.Threading.Lock where possible #2501

Closed
wants to merge 4 commits into from

Conversation

MarkCiliaVincenti
Copy link

No description provided.

Copy link

linux-foundation-easycla bot commented Aug 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@JamesNK
Copy link
Member

JamesNK commented Aug 13, 2024

Hi. I like the idea, but I don't want to add a NuGet dependency for this. I'd rather have an internal Lock type in the Shared folder that projects reference. Or perhaps just add a using alias for it:

#if !NET9_OR_GREATER
using Lock = System.Object;
#endif

@MarkCiliaVincenti
Copy link
Author

Hi. I like the idea, but I don't want to add a NuGet dependency for this. I'd rather have an internal Lock type in the Shared folder that projects reference. Or perhaps just add a using alias for it:

#if !NET9_OR_GREATER
using Lock = System.Object;
#endif

The problem is that some things won't compile properly that way, such as this code:

Debug.Assert(Lock.IsHeldByCurrentThread);

@MarkCiliaVincenti
Copy link
Author

I dropped the dependency, just giving some credit to the package in comments. What do you think?

@JamesNK
Copy link
Member

JamesNK commented Aug 13, 2024

Does Monitor.IsEntered(Lock) stop working in .NET 9 if you use the new lock type?

@MarkCiliaVincenti
Copy link
Author

Does Monitor.IsEntered(Lock) stop working in .NET 9 if you use the new lock type?

It's not the proper way to use it now but I have no idea. Will try it tomorrow.

@JamesNK
Copy link
Member

JamesNK commented Aug 14, 2024

I don't want to update source code to not use the lock statement. And I don't want to add a new package.

My preference is to have .NET 9 (and later) targets use the new lock type, and older targets to keep using object. To be honest I don't think there will be any noticeable difference in performance, none of the locks a very contented, so it's fine if just future runtimes get any benefit.

If Monitor.IsEntered isn't supported with lock, then it would be simple enough to add a new helper method to support the difference:

internal static class CompatibilityHelpers
{
#if NET9_OR_GREATER
    public static bool IsLockHeldByCurrentThread(Lock lock) => lock.IsHeldByCurrentThread;
#else
    public static bool IsLockHeldByCurrentThread(object lock) => Monitor.IsEntered(lock);
#endif
}

// usage
Debug.Assert(CompatibilityHelpers.IsLockHeldByCurrentThread(Lock));

@MarkCiliaVincenti
Copy link
Author

I already updated the code to drop the dependency.

It's not going to be possible to use every feature of the new lock class and have the same code compile for .NET 8 or earlier. The aim is to make it close enough, because then the alternative is using preprocessor directives and it all starts getting messy.

The problem where I stopped using locks was when used inside async methods. Even though a lock on an object would work when inside it there's no async calls, for some reason the lock on this fake lock object doesn't and I haven't figured out why and whether or not it can be fixed. To be honest if those locks are set back to an object it would be OK but then you end up with a mix of some sync locks using object and others using Lock.

@MarkCiliaVincenti
Copy link
Author

Ah gotcha!

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/lock-semantics?f1url=%3FappId%3Droslyn%26k%3Dk(CS9217)#lock-statement-errors

This is going to be the new reality moving forward and nothing to do with the backport code in fact but rather with C# 13.

That is, you will have this issue even if you only target .NET 9.0. If you're using the new System.Threading.Lock class, you can't lock on it in an async method, even if you're not awaiting anything inside (CS9217). Even if you try to convert it to an object, it will give warning CS9216.

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.

2 participants