Skip to content
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.

Sign up for GitHub

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

Jump to bottom

runtime: Add a metric that counts currently active tasks #5628

Merged
merged 9 commits into from
Apr 27, 2023
Merged

runtime: Add a metric that counts currently active tasks #5628

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c7d3512
runtime: Add a metric that counts currently active tasks
matildasmeds Apr 16, 2023
32343c2
Use atomic counter to track LinkedList length
matildasmeds Apr 18, 2023
e757921
Add CountedLinkedList type that wraps LinkedList
matildasmeds Apr 20, 2023
1110961
Update tokio/src/util/linked_list.rs
matildasmeds Apr 24, 2023
4157371
rustfmt
matildasmeds Apr 24, 2023
e7b00b1
Remove underscores from variable names
matildasmeds Apr 24, 2023
9292ed2
remove const from fn def
matildasmeds Apr 24, 2023
041d2a5
Make counter non-atomic
matildasmeds Apr 26, 2023
c1a03c7
Merge remote-tracking branch 'origin/master' into matildasmeds/runtim…
matildasmeds Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions tokio/src/runtime/task/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::future::Future;
use crate::loom::cell::UnsafeCell;
use crate::loom::sync::Mutex;
use crate::runtime::task::{JoinHandle, LocalNotified, Notified, Schedule, Task};
use crate::util::linked_list::{Link, LinkedList};
use crate::util::linked_list::{CountedLinkedList, Link, LinkedList};

use std::marker::PhantomData;

Expand Down Expand Up @@ -54,9 +54,13 @@ cfg_not_has_atomic_u64! {
}

pub(crate) struct OwnedTasks<S: 'static> {
inner: Mutex<OwnedTasksInner<S>>,
inner: Mutex<CountedOwnedTasksInner<S>>,
id: u64,
}
struct CountedOwnedTasksInner<S: 'static> {
list: CountedLinkedList<Task<S>, <Task<S> as Link>::Target>,
Comment on lines +60 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to also duplicate this one. For this particular case, I think it's fine to just keep the counter in both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it is not necessary for the code to function correctly, I believe using different types for OwnedTasks and LocalOwnedTasks helps convey our intention more clearly. If we use CountedLinkedList for LocalOwnedTasks, the reader may think we could be counting those too.

While it does add a few lines of code, I believe it is worth it to ensure clarity and readability. Looking forward to hearing your thoughts on this one!

closed: bool,
}
pub(crate) struct LocalOwnedTasks<S: 'static> {
inner: UnsafeCell<OwnedTasksInner<S>>,
id: u64,
Expand All @@ -70,8 +74,8 @@ struct OwnedTasksInner<S: 'static> {
impl<S: 'static> OwnedTasks<S> {
pub(crate) fn new() -> Self {
Self {
inner: Mutex::new(OwnedTasksInner {
list: LinkedList::new(),
inner: Mutex::new(CountedOwnedTasksInner {
list: CountedLinkedList::new(),
closed: false,
}),
id: get_next_id(),
Expand Down Expand Up @@ -300,4 +304,4 @@ mod tests {
last_id = next_id;
}
}
}
}
63 changes: 48 additions & 15 deletions tokio/src/util/linked_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ pub(crate) struct LinkedList<L, T> {

/// Node type marker.
_marker: PhantomData<*const L>,

/// Tracks the length of the linked list
_count: AtomicUsize,
}

unsafe impl<L: Link> Send for LinkedList<L, L::Target> where L::Target: Send {}
Expand Down Expand Up @@ -121,7 +118,6 @@ impl<L, T> LinkedList<L, T> {
head: None,
tail: None,
_marker: PhantomData,
_count: AtomicUsize::new(0),
}
}
}
Expand All @@ -147,7 +143,6 @@ impl<L: Link> LinkedList<L, L::Target> {
self.tail = Some(ptr);
}
}
self._count.fetch_add(1, Ordering::Relaxed);
}

/// Removes the last element from a list and returns it, or None if it is
Expand All @@ -166,8 +161,6 @@ impl<L: Link> LinkedList<L, L::Target> {
L::pointers(last).as_mut().set_prev(None);
L::pointers(last).as_mut().set_next(None);

self._count.fetch_sub(1, Ordering::Relaxed);

Some(L::from_raw(last))
}
}
Expand All @@ -182,11 +175,6 @@ impl<L: Link> LinkedList<L, L::Target> {
true
}

// Returns the number of elements contained in the LinkedList
pub(crate) fn count(&self) -> usize {
self._count.load(Ordering::Relaxed)
}

/// Removes the specified node from the list
///
/// # Safety
Expand Down Expand Up @@ -228,8 +216,6 @@ impl<L: Link> LinkedList<L, L::Target> {
L::pointers(node).as_mut().set_next(None);
L::pointers(node).as_mut().set_prev(None);

self._count.fetch_sub(1, Ordering::Relaxed);

Some(L::from_raw(node))
}
}
Expand All @@ -243,6 +229,53 @@ impl<L: Link> fmt::Debug for LinkedList<L, L::Target> {
}
}

// ===== impl CountedLinkedList ====

// Delegates operations to the base LinkedList implementation, and adds a counter to the elements
// in the list.
pub(crate) struct CountedLinkedList<L: Link, T> {
_list: LinkedList<L, T>,
_count: AtomicUsize,
matildasmeds marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a mutable reference any time you modify it, so you don't need an atomic.

Suggested change
_list: LinkedList<L, T>,
_count: AtomicUsize,
_list: LinkedList<L, T>,
_count: usize,

If you want an atomic, then you should place it outside of the mutex. In that case, the logic would need to go in OwnedTasks rather than in the linked list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to have marked this as resolved, but it does not appear to have been addressed.

The point of using an atomic is to avoid locking the mutex when accessing the counter. However, because the atomic is inside the mutex, you are still locking the mutex when accessing the counter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies marking this resolved without actually implementing the changes you had requested. It was not intentional, and I regret any inconvenience this may have caused. I will make sure to be more careful in the future to avoid similar mistakes. Thank you for your time and understanding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks better now.

There's no need to apologize for something like this. I was not offended or unhappy about it — I just wanted to clarify what I meant in case my review comment was unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, all good! 😄

}

impl<L: Link> CountedLinkedList<L, L::Target> {
pub(crate) const fn new() -> CountedLinkedList<L, L::Target> {
CountedLinkedList {
_list: LinkedList::new(),
_count: AtomicUsize::new(0),
}
}

pub(crate) fn push_front(&mut self, val: L::Handle) {
self._list.push_front(val);
self._count.fetch_add(1, Ordering::Relaxed);
}

pub(crate) fn pop_back(&mut self) -> Option<L::Handle> {
let val = self._list.pop_back();
if val.is_some() {
self._count.fetch_sub(1, Ordering::Relaxed);
}
val
}

pub(crate) fn is_empty(&self) -> bool {
self._list.is_empty()
}

pub(crate) unsafe fn remove(&mut self, node: NonNull<L::Target>) -> Option<L::Handle> {
let val = self._list.remove(node);
if val.is_some() {
self._count.fetch_sub(1, Ordering::Relaxed);
}
val
}

pub(crate) fn count(&self) -> usize {
self._count.load(Ordering::Relaxed)
}
}

#[cfg(any(
feature = "fs",
feature = "rt",
Expand Down Expand Up @@ -736,7 +769,7 @@ pub(crate) mod tests {

#[test]
fn count() {
let mut list = LinkedList::<&Entry, <&Entry as Link>::Target>::new();
let mut list = CountedLinkedList::<&Entry, <&Entry as Link>::Target>::new();
assert_eq!(0, list.count());

let a = entry(5);
Expand Down