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

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

Merged
merged 1 commit into from
May 15, 2024

Conversation

kouvel
Copy link
Member

@kouvel kouvel commented May 14, 2024

  • Also removed the EnablePreviewFeatures project properties that were added with Lock changes
  • Added some basic tests with Lock using the lock keyword

…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
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@@ -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
Member 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
Member 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.

@kouvel kouvel merged commit 1ee61b8 into dotnet:main May 15, 2024
146 checks passed
@kouvel kouvel deleted the LockNonPreview branch May 15, 2024 18:49
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants