-
-
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
Implementation of notify_last method #6520
Conversation
A new notify_one_lifo method is implemented for notifying the most recent waiter, LIFO, instead of the oldest one, FIFO, which is used by the current notify_one method.
tokio/src/sync/notify.rs
Outdated
@@ -1138,7 +1205,9 @@ impl Drop for Notified<'_> { | |||
// the notification was triggered via `notify_one`, it must be sent | |||
// to the next waiter. | |||
if notification == Some(Notification::One) { | |||
if let Some(waker) = notify_locked(&mut waiters, ¬ify.state, notify_state) { | |||
if let Some(waker) = | |||
notify_locked(&mut waiters, ¬ify.state, notify_state, strategy.unwrap()) |
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 think we can drop the Option
around the strategy. Then we don't need an unwrap
here. We can ignore the strategy when we are waking everything.
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.
Would you prefer to just pass or return the NOTIFICATION_NOTIFY_ONE_STRATEGY_NONE
? If so we can also avoid this two indirection using the const and the enum and having somthing like
enum NotifyOneStrategy {
None = 0
Fifo = 1,
Lifo = 2,
}
And use the NotifyOneStrategy
as a parameter for the store and as a return value for the load.
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 commented above, but it looks like the strategy is an argument to Notification
. Something like:
enum Notification {
One(Strategy),
All,
}
You can implement the conversion of the enum to size using a match statement, the compiler will optimize it anyway.
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.
Yeps, changing a bit this one.
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.
Left some thoughts inline.
tokio/src/sync/notify.rs
Outdated
@@ -269,30 +284,48 @@ struct AtomicNotification(AtomicUsize); | |||
|
|||
impl AtomicNotification { | |||
fn none() -> Self { | |||
AtomicNotification(AtomicUsize::new(NOTIFICATION_NONE)) | |||
AtomicNotification(AtomicUsize::new(0)) |
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.
Is there a reason this isn't a const anymore?
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.
Maybe there was since bot attributes, if there were a notification and the strategy, but once Ive changed the interfaces we can go back to what was originally, so this should be "fixed" now.
tokio/src/sync/notify.rs
Outdated
} | ||
|
||
/// Store-release a notification. | ||
/// This method should be called exactly once. | ||
fn store_release(&self, notification: Notification) { | ||
self.0.store(notification as usize, Release); | ||
fn store_release(&self, notification: Notification, strategy: Option<NotifyOneStrategy>) { |
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 need Option
here or could callers default to a strategy?
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.
This should no longer apply after the latest refactor.
tokio/src/sync/notify.rs
Outdated
} | ||
|
||
fn load(&self, ordering: Ordering) -> Option<Notification> { | ||
match self.0.load(ordering) { | ||
fn load(&self, ordering: Ordering) -> (Option<Notification>, Option<NotifyOneStrategy>) { |
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.
Is there ever a time when this will return None
for Notification
but will return Some
for NotifyOneStrategy
? I don't think there is. Perhaps, Notification
the enum should take the strategy as an argument:
enum Notification {
One(Strategy),
All,
}
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.
Good point, let me change this towards your suggestion.
tokio/src/sync/notify.rs
Outdated
/// | ||
/// Check the `notify_one` documentation for more info and | ||
/// examples. | ||
pub fn notify_one_lifo(&self) { |
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'm not against calling this notify_one_lifo
. As a consideration, do you think notify_last_in
or notify_one_last_in
could be better options?
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.
notify_one_last_in
sounds better, users will easily understand the semantics of the method. Ill change this one.
tokio/src/sync/notify.rs
Outdated
@@ -1138,7 +1205,9 @@ impl Drop for Notified<'_> { | |||
// the notification was triggered via `notify_one`, it must be sent | |||
// to the next waiter. | |||
if notification == Some(Notification::One) { | |||
if let Some(waker) = notify_locked(&mut waiters, ¬ify.state, notify_state) { | |||
if let Some(waker) = | |||
notify_locked(&mut waiters, ¬ify.state, notify_state, strategy.unwrap()) |
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 commented above, but it looks like the strategy is an argument to Notification
. Something like:
enum Notification {
One(Strategy),
All,
}
You can implement the conversion of the enum to size using a match statement, the compiler will optimize it anyway.
@carllerche and @Darksonn PR should be ready for another look 🙇 |
Any new feedback on this one? 🙇 |
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.
Hi again. Sorry for the delay. I was at RustNL this week.
tokio/src/sync/notify.rs
Outdated
let data: usize = match notification { | ||
Notification::All => NOTIFICATION_ALL & NOTIFICATION_TYPE_MASK, | ||
Notification::One(NotifyOneStrategy::Fifo) => { | ||
(((NOTIFICATION_NOTIFY_ONE_STRATEGY_FIFO) | ||
<< NOTIFICATION_NOTIFY_ONE_STRATEGY_SHIFT) | ||
& NOTIFICATION_NOTIFY_ONE_STRATEGY_MASK) | ||
| (NOTIFICATION_ONE & NOTIFICATION_TYPE_MASK) | ||
} | ||
Notification::One(NotifyOneStrategy::Lifo) => { | ||
(((NOTIFICATION_NOTIFY_ONE_STRATEGY_LIFO) | ||
<< NOTIFICATION_NOTIFY_ONE_STRATEGY_SHIFT) | ||
& NOTIFICATION_NOTIFY_ONE_STRATEGY_MASK) | ||
| (NOTIFICATION_ONE & NOTIFICATION_TYPE_MASK) |
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.
This is pretty hard to read. Can we just make three constants equal to 0b00
, 0b01
, 0b10
or whatever the values are?
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.
Good call, Ive changed for using just constants which makes definitely more readable the code
tokio/tests/sync_notify.rs
Outdated
|
||
notify.notify_one_last_in(); | ||
|
||
drop(notified1); |
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.
Shouldn't you drop notified3
to trigger the forwarding logic?
drop(notified1); | |
drop(notified3); |
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.
My bad, fixed!
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
@Darksonn thanks for the review, comments should be addressed by now. |
@Darksonn @carllerche any feedback on this one? 🙇 |
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 implementation looks good to me. But I have a question about the naming. Why notify_one_last_in
? Last in what?
last in to be added into the queue. @carllerche recommended this name over the |
@carllerche any chance that you could take another look to the PR? |
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.
Thanks for the changes. After a quick chat in Discord, @Darksonn convinced me that notify_last
would be a better name.
Sorry for the churn. The code is good, could you change the name to notify_last
and then we should be good to go.
Updated to a comment on the name to not block
No problemo @carllerche, Ive already renamed it in my latest commit, PTAL when you have a moment /cc @Darksonn |
tokio/src/sync/notify.rs
Outdated
/// examples. | ||
/// | ||
/// [`notify_one()`]: Notify::notify_one | ||
pub fn notify_one_last(&self) { |
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 suggested name was notify_last
, not notify_one_last
.
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.
🤦 my bad.
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.
done!
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.
Thank you.
A new
notify_last
method is implemented for notifying the most recent waiter, LIFO, instead of the oldest one, FIFO, which is used by the current notify_one method.Motivation
As explained here #6506 the
notify_last
method provides the user a method for waking up the most recent waiter rather than the oldest one. When using a LIFO strategy we sacrifice waiters that have been waiting longer by first dequeuing the ones that have been in the queue for shorter time, this should provide benefits in situations of contention where LIFO dequeueing should provide a better latency observation compared to the usage of the FIFO.Solution
Ive followed main recommendations for storing the strategy for dequeueing followed by the user as part of the waiter notification state. No impact on the memory footprint should be observed.