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

Refactor lock on object usage to System.Threading.Lock #11672

Closed
elachlan opened this issue Jul 14, 2024 · 10 comments
Closed

Refactor lock on object usage to System.Threading.Lock #11672

elachlan opened this issue Jul 14, 2024 · 10 comments
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation tenet-performance Improve performance, flag performance regressions across core releases
Milestone

Comments

@elachlan
Copy link
Contributor

elachlan commented Jul 14, 2024

In .NET9 there is the new System.Threading.Lock
https://learn.microsoft.com/en-us/dotnet/api/system.threading.lock?view=net-9.0

It is recommended to use the EnterScope method with a language construct that automatically disposes the returned Lock.Scope such as the C# using keyword, or to use the C# lock keyword, as these ensure that the lock is exited in exceptional cases. These patterns might also have performance benefits over using Enter/TryEnter and Exit. The following code fragment illustrates various patterns for entering and exiting a lock.

Refactor the object used for the lock to Lock where appropriate. The new Lock class when used with lock() will not use system.threading.monitor, but instead will use system.threading.lock.

Benchmarks in this blog post suggest a 25% performance improvement in lock operations:
https://steven-giesel.com/blogPost/4cf6d48a-ec9d-4c68-961c-31fd8d8c1340

There are some limitations, I think the team is best positioned to decide whats best here.

We won't make changes until C# takes the change to the lock keyword out of preview.
dotnet/csharplang#7104

@elachlan elachlan added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation tenet-performance Improve performance, flag performance regressions across core releases labels Jul 14, 2024
@elachlan
Copy link
Contributor Author

@kirsan31 I saw you commented on the runtime issue. What are your thoughts on this?

@elachlan
Copy link
Contributor Author

@JeremyKuhne you might be able to shoot this down early.

@elachlan
Copy link
Contributor Author

One thing I am wondering is if the heavy usage of lock (this) is on purpose?

@JeremyKuhne
Copy link
Member

We can explore this when the feature leaves preview state.

@JeremyKuhne JeremyKuhne added this to the .NET 10.0 milestone Jul 16, 2024
@elachlan
Copy link
Contributor Author

elachlan commented Jul 16, 2024

Runtime issue: dotnet/runtime#34812
C# issue: dotnet/csharplang#7104

@kirsan31
Copy link
Contributor

kirsan31 commented Jul 21, 2024

@kirsan31 I saw you commented on the runtime issue. What are your thoughts on this?

My thots on this:

  1. Need to wait until release.
  2. Need a full sets of benchmarks (with all usage types):
    • Acquire / release without waiting. I hope this must be faster then current object lock :)
    • Long time waiting. Must be at least not worse then current :)
    • Short time waiting. Here the main question because of SpinWait using. In some cases we can get better performance but in some case much worse (many short waiters will eat cpu ) :(

So if acquire/release without waiting will be faster (it must) then we can feel free to replace object locking in those cases where waiting is rare. In other cases some analysis will be needed I think...

@elachlan
Copy link
Contributor Author

The code base locks a lot on this. Which usually isn't a good idea. I'd like this issue to review that as well.

@elachlan
Copy link
Contributor Author

I've raised an roslyn-analyzer rule over at: dotnet/runtime#106907

It would make changes like this easier.

@elachlan
Copy link
Contributor Author

@lonitra do you know who we can ask about the new Lock type? I am wondering if we can use it based on this comment:

If we want to have a lock object type not integrated into Monitor, then there is quite a bit of work required to make sure it integrates with the diagnostics infrastructure and the threading model correctly. (Monitor is an interruptible lock as it uses a COM interruptible wait so that STA frameworks such as WinForms and WPF work correctly. Any other lock that is intended for general use will also need to follow those strictures.)

dotnet/runtime#34812 (comment)

@JeremyKuhne
Copy link
Member

Going to do this in .NET 10. I'm comfortable enough with what I've read to commit to this.

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this issue Aug 8, 2024
Change internal lock objects to System.Threading.Lock

Fixes dotnet#11672
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation tenet-performance Improve performance, flag performance regressions across core releases
Projects
None yet
Development

No branches or pull requests

3 participants