-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Speed-up waker by using uninitialized array #4055
Merged
Merged
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
7fd2ae7
Use uninitialized array in wake0 to prevent initialiation of each Opt…
01b91ab
avoid using Option
2bb9da5
rustfmt
c0aad94
implement WakeList to support Drop on it
b932ea5
don't return from push
03103d8
Prevent later dropping of uninitialized Waker if `wake` panics.
glebpom 9dd73eb
Fix `can_push` condition - allow usage of the last waker.
0fb5f46
Use slice_from_raw_parts_mut/drop_in_place in WakerList Drop implemen…
glebpom c7e65e6
remove unused import
5040110
fmt
7068c3e
try to fix imports
958d43b
clippy
7a55899
import core::ptr
905dc93
use for loop with rev iterator in wake_all
cbdcfd3
Revert "use for loop with rev iterator in wake_all"
0267a17
rustfmt
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
use core::mem::MaybeUninit; | ||
use core::ptr; | ||
use std::task::Waker; | ||
|
||
const NUM_WAKERS: usize = 32; | ||
|
||
pub(crate) struct WakeList { | ||
inner: [MaybeUninit<Waker>; NUM_WAKERS], | ||
curr: usize, | ||
} | ||
|
||
impl WakeList { | ||
pub(crate) fn new() -> Self { | ||
Self { | ||
inner: unsafe { MaybeUninit::uninit().assume_init() }, | ||
seanmonstar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
curr: 0, | ||
} | ||
} | ||
|
||
#[inline] | ||
pub(crate) fn can_push(&self) -> bool { | ||
self.curr < NUM_WAKERS | ||
} | ||
|
||
pub(crate) fn push(&mut self, val: Waker) { | ||
debug_assert!(self.can_push()); | ||
|
||
self.inner[self.curr] = MaybeUninit::new(val); | ||
self.curr += 1; | ||
} | ||
|
||
pub(crate) fn wake_all(&mut self) { | ||
assert!(self.curr <= NUM_WAKERS); | ||
hawkw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
while self.curr > 0 { | ||
self.curr -= 1; | ||
let waker = unsafe { ptr::read(self.inner[self.curr].as_mut_ptr()) }; | ||
waker.wake(); | ||
} | ||
} | ||
} | ||
|
||
impl Drop for WakeList { | ||
fn drop(&mut self) { | ||
let slice = | ||
ptr::slice_from_raw_parts_mut(self.inner.as_mut_ptr() as *mut Waker, self.curr); | ||
unsafe { ptr::drop_in_place(slice) }; | ||
} | ||
} |
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.
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.
Here's a thought: Are we not protecting against panics here in the wrong way? Should we really be panicking and not waking the other wakers just because someone gave us a waker that emitted a panic?
Maybe we should just catch all panics that happen when calling
wake
and ignore them.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.
What is the typical scenario of waker panic? Does it invoke polling directly? I'm not familiar with all internals, but it seems like there should be some validation of the solution. Writing the proper test cases with panicking would definitely help.
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.
These wakers are user supplied and could run literally any code in the
wake
call. That said, a well behaved waker should never panic.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.
Hmm, that's a good point. If one waker panics, failing to wake the others could result in those tasks never being notified.
On the other hand, what's the overhead of adding a
catch_unwind
in this fairly hot loop? Is that worth introducing to handle a case which can only happen if a user-supplied waker is not "well-behaved"?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.
We can still have the destructor call
wake
on the others without acatch_unwind
.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.
A destructor panicing would be even worse, since it could cause a double panic, which would result into an abort
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.
Can't user-supplied waker's Drop implementation panic as well? This will cause double panic in the Drop implementation even without waking
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.
Yes, if the wakers panic in the destructor, you can get a double abort.
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.
It looks like it's absolutely critical to not panic in any of the waker functions. The alternative approach is not to try to fix the erroneous implementation, but to forcefully abort the execution