Implementation of notify_one_last_in method #6520

pfreixes · 2024-04-27T20:20:23Z

A new notify_one_last_in 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_one_last_in 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.

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.

Darksonn · 2024-04-30T13:27:20Z

tokio/src/sync/notify.rs

@@ -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, &notify.state, notify_state) {
+                if let Some(waker) =
+                    notify_locked(&mut waiters, &notify.state, notify_state, strategy.unwrap())


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.

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.

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.

Yeps, changing a bit this one.

carllerche

Left some thoughts inline.

carllerche · 2024-04-30T18:00:54Z

tokio/src/sync/notify.rs

@@ -269,30 +284,48 @@ struct AtomicNotification(AtomicUsize);

 impl AtomicNotification {
    fn none() -> Self {
-        AtomicNotification(AtomicUsize::new(NOTIFICATION_NONE))
+        AtomicNotification(AtomicUsize::new(0))


Is there a reason this isn't a const anymore?

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.

carllerche · 2024-04-30T18:03:09Z

tokio/src/sync/notify.rs

    }

    /// 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>) {


Do we need Option here or could callers default to a strategy?

This should no longer apply after the latest refactor.

carllerche · 2024-04-30T18:04:32Z

tokio/src/sync/notify.rs

    }

-    fn load(&self, ordering: Ordering) -> Option<Notification> {
-        match self.0.load(ordering) {
+    fn load(&self, ordering: Ordering) -> (Option<Notification>, Option<NotifyOneStrategy>) {


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, }

Good point, let me change this towards your suggestion.

carllerche · 2024-04-30T18:08:04Z

tokio/src/sync/notify.rs

+    ///
+    /// Check the `notify_one` documentation for more info and
+    /// examples.
+    pub fn notify_one_lifo(&self) {


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?

notify_one_last_in sounds better, users will easily understand the semantics of the method. Ill change this one.

carllerche · 2024-04-30T18:12:39Z

tokio/src/sync/notify.rs

@@ -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, &notify.state, notify_state) {
+                if let Some(waker) =
+                    notify_locked(&mut waiters, &notify.state, notify_state, strategy.unwrap())


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.

pfreixes · 2024-05-02T08:35:15Z

@carllerche and @Darksonn PR should be ready for another look 🙇

pfreixes · 2024-05-09T16:52:08Z

Any new feedback on this one? 🙇

Darksonn

Hi again. Sorry for the delay. I was at RustNL this week.

Darksonn · 2024-05-11T16:33:10Z

tokio/src/sync/notify.rs

+        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)


This is pretty hard to read. Can we just make three constants equal to 0b00, 0b01, 0b10 or whatever the values are?

Good call, Ive changed for using just constants which makes definitely more readable the code

tokio/src/sync/notify.rs

tokio/src/util/linked_list.rs

Darksonn · 2024-05-11T16:37:25Z

tokio/tests/sync_notify.rs

+
+    notify.notify_one_last_in();
+
+    drop(notified1);


Shouldn't you drop notified3 to trigger the forwarding logic?

Suggested change

drop(notified1);

drop(notified3);

My bad, fixed!

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

pfreixes · 2024-05-12T09:23:37Z

@Darksonn thanks for the review, comments should be addressed by now.

Implementation of notify_one_lifo method

c3bbef6

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.

github-actions bot added the R-loom-sync Run loom sync tests on this PR label Apr 27, 2024

Format code

0fd084f

pfreixes mentioned this pull request Apr 27, 2024

Proposal provide support for notifying the latest added waiter (LIFO) for the Notify interface #6506

Open

pfreixes added 2 commits April 27, 2024 22:27

Typos

f170624

Typos again

96d3823

Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Apr 30, 2024

Darksonn reviewed Apr 30, 2024

View reviewed changes

carllerche requested changes Apr 30, 2024

View reviewed changes

pfreixes added 4 commits May 2, 2024 09:37

Rename lifo method name to last in

942539c

Change Notification types

ff52e32

fmt

fbcb9a7

Apply linter feedback

a945474

pfreixes changed the title ~~Implementation of notify_one_lifo method~~ Implementation of notify_one_last_in method May 3, 2024

Darksonn reviewed May 11, 2024

View reviewed changes

pfreixes and others added 7 commits May 12, 2024 10:44

Update tokio/src/sync/notify.rs

c387ef1

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Update tokio/src/sync/notify.rs

fd474cb

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Update tokio/src/util/linked_list.rs

1dff2b9

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Fix invalid test

f498f7d

Make more clear current code

428b8ec

Try to fix documetnation

1ad00d2

Try to fix docs again

531ef54

Merge branch 'master' into notify_suppot_lifo

c0ffca2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of notify_one_last_in method #6520

Implementation of notify_one_last_in method #6520

pfreixes commented Apr 27, 2024 •

edited

Darksonn Apr 30, 2024

pfreixes Apr 30, 2024

carllerche Apr 30, 2024

pfreixes May 2, 2024

carllerche left a comment

carllerche Apr 30, 2024

pfreixes May 2, 2024

carllerche Apr 30, 2024

pfreixes May 2, 2024

carllerche Apr 30, 2024

pfreixes May 2, 2024

carllerche Apr 30, 2024

pfreixes May 2, 2024

carllerche Apr 30, 2024

pfreixes commented May 2, 2024

pfreixes commented May 9, 2024

Darksonn left a comment

Darksonn May 11, 2024

pfreixes May 12, 2024

Darksonn May 11, 2024

pfreixes May 12, 2024

pfreixes commented May 12, 2024

Implementation of notify_one_last_in method #6520

Are you sure you want to change the base?

Implementation of notify_one_last_in method #6520

Conversation

pfreixes commented Apr 27, 2024 • edited

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfreixes commented May 2, 2024

pfreixes commented May 9, 2024

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfreixes commented May 12, 2024

pfreixes commented Apr 27, 2024 •

edited