Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[c11] define mono atomics in terms of standard atomics #91489
[c11] define mono atomics in terms of standard atomics #91489
Changes from 12 commits
4f87b95
f0f5fa0
3685100
5728b5b
1303731
a44166e
5e43c4e
5935cff
4f29f31
da6b965
b33ab29
d8821d8
ec6f654
6984ac0
2017e5d
32c55e2
0b30143
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we have strong guarantees for mono_atomic_cas_* functions? I believe we normally call these in loops to handle potential spuriously fails on platforms that could trigger them. If we would like to make the default implementation using strong guarantees, then maybe we should also offer the option to use weak when we already do cas in loops to potentially be more performant on platforms that takes advantage of spuriously fails?
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 good point. I need to look at the callers and check if they're all in loops. My feeling is that we should make weak the default and add strong as a second option. I didn't want to change the mono functions as part of this PR (and strong seemed like a safer default). But I can audit and report back and then we can add the non-default versions
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.
Found 173 occurrences of
mono_atomic_cas
in src/mono/mono. Looked through about 2/3rds of them, chasing down callers if it's in a static helper method or a macro. About half are weak uses and about half are strong. Sostrong
is definitely the right default. In a few places we could switch over to weak (particularly inside sgen there are some loops that could use weak). But it should be follow-up work, IMO.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.
That is what I expected, we have made different assumption around the strong/weak guarantees around the runtime over years. Moving over to strong will potential correct some false assumptions previously made in runtime (at least it won't break anything), so it should be safe, and I agree that we could identify and use weak versions where it might benefit.
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.
Created #91747 for follow-up work