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
Use the parking_lot locking primitives #56410
Use the parking_lot locking primitives #56410
Changes from all commits
7b0ad27
1aab11d
e313500
20ab8d2
7cafa6d
7c42fc8
3b23add
f58fc9b
81a1e21
5a3dd8a
d354b95
8edc128
4f0f8e1
a0b366c
8dfad4b
d6fab63
fc379ee
142bda2
eaaca60
0d679ef
e8e355e
55f1ed2
c6c6fb1
f1a805b
0b1a6e9
a562580
b50a20e
6ad5485
0070390
c294204
c301ce0
dd30a55
e0fd309
0048440
e49bbd1
f40d609
56c2805
7124cae
6695be8
795285f
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.
Where should this be pointing before merging? Back to @Amanieu's repo, or will that be moved to the @rust-lang organization, like what I think is the plan for hashbrown?
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.
So everything else uses std's
RawLock
but this uses the one fromparking_lot
... why that?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.
Oh, that's because it needs the
RwLock
, not theMutex
, I guess.Seems like another agument to make the rest also use parking_lot's stuff directly, though.
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.
But they are not equivalent. See my other comment.
parking_lot::RawMutex
is lower level and would require a lot of manual unlocking where we now get that much nicer with the guard. The standard library had a mutex with a guard prior to this PR. So going for theparking_lot
stuff directly for the mutexes would make everything more low level and harder to get fully correct.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.
The previous
crate::sys_common::rwlock::RWLock
did not have any guards. But thecrate::sys_common::mutex::Mutex
did. So doing what this PR does currently keeps libstd as similar as possible. Going directly forparking_lot::RawMutex
would be a step backwards IMO. We could however write a small wrapper for the RwLock that has guards. But given it's only used in so few places that felt a bit overkill.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.
Okay, so this one file here is where we are okay with not having guards?
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.
I suppose so. It's how the standard library is currently implemented. So someone must have thought it was okay at some point. Well, here and in one place SGX also uses
parking_lot::RawRwLock
directly in this PR. I would not be against introducing a simplesync::RawRwLock
with guards. But it would not be that efficient to use in the SGX place, so it would still only bepanicking.rs
that would use it 🤷♂️