-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
runtime: Add a metric that counts currently active tasks #5628
Changes from 3 commits
c7d3512
32343c2
e757921
1110961
4157371
e7b00b1
9292ed2
041d2a5
c1a03c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ use core::fmt; | |||||||||
use core::marker::{PhantomData, PhantomPinned}; | ||||||||||
use core::mem::ManuallyDrop; | ||||||||||
use core::ptr::{self, NonNull}; | ||||||||||
use std::sync::atomic::{AtomicUsize, Ordering}; | ||||||||||
|
||||||||||
/// An intrusive linked list. | ||||||||||
/// | ||||||||||
|
@@ -228,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
If you want an atomic, then you should place it outside of the mutex. In that case, the logic would need to go in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||||||||||
|
@@ -719,6 +767,26 @@ pub(crate) mod tests { | |||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
#[test] | ||||||||||
fn count() { | ||||||||||
let mut list = CountedLinkedList::<&Entry, <&Entry as Link>::Target>::new(); | ||||||||||
assert_eq!(0, list.count()); | ||||||||||
|
||||||||||
let a = entry(5); | ||||||||||
let b = entry(7); | ||||||||||
list.push_front(a.as_ref()); | ||||||||||
list.push_front(b.as_ref()); | ||||||||||
assert_eq!(2, list.count()); | ||||||||||
|
||||||||||
list.pop_back(); | ||||||||||
assert_eq!(1, list.count()); | ||||||||||
|
||||||||||
unsafe { | ||||||||||
list.remove(ptr(&b)); | ||||||||||
} | ||||||||||
assert_eq!(0, list.count()); | ||||||||||
} | ||||||||||
|
||||||||||
/// This is a fuzz test. You run it by entering `cargo fuzz run fuzz_linked_list` in CLI in `/tokio/` module. | ||||||||||
#[cfg(fuzzing)] | ||||||||||
pub fn fuzz_linked_list(ops: &[u8]) { | ||||||||||
|
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.
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.
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.
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!