-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Remove RequiresPreviewFeatures
attribute on Lock
, add test using the lock
keyword
#102222
Conversation
…the `lock` keyword - Also removed the `EnablePreviewFeatures` project properties that were added with `Lock` changes - Added some basic tests with `Lock` using the `lock` keyword
Note regarding the
|
Tagging subscribers to this area: @mangod9 |
@@ -15158,7 +15158,6 @@ public enum LazyThreadSafetyMode | |||
PublicationOnly = 1, | |||
ExecutionAndPublication = 2, | |||
} | |||
[System.Runtime.Versioning.RequiresPreviewFeaturesAttribute] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…the `lock` keyword (dotnet#102222) - Also removed the `EnablePreviewFeatures` project properties that were added with `Lock` changes - Added some basic tests with `Lock` using the `lock` keyword
EnablePreviewFeatures
project properties that were added withLock
changesLock
using thelock
keyword