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

matildasmeds · 2023-04-16T11:04:15Z

Adds a new metric for the number of active tasks for the Runtime. We count the elements on the OwnedTasks LinkedList, and access that through RuntimeMetrics.

I have added simple tests for the new metric, mimicking the style of existing tests. LinkedList does not have tests currently, so I did not add tests for LinkedList::count() either. Happy to add those tests if requested.

Runtime metrics come in two flavors, either prefixed with num_ or postfixed with _count. The latter seems to be more common, so I have opted for that, but num_active_tasks() and even just active_tasks() as suggested in the original issue are certainly valid options as well.

Looking forward to any feedback! Thank you for taking a look. 😄

Closes: #5400

Adds a new metric for the number of active tasks for the Runtime. We count the elements on the OwnedTasks LinkedList, and access that through RuntimeMetrics. Closes: tokio-rs#5400

matildasmeds · 2023-04-16T12:50:19Z

Force pushed after rebasing with master, to resolve the FreeBSD checks and one merge conflict. Given no comments were yet given, it felt like a reasonable thing to do. 🙏

tokio/src/util/linked_list.rs

Darksonn · 2023-04-20T21:59:41Z

tokio/src/runtime/task/list.rs

+struct CountedOwnedTasksInner<S: 'static> {
+    list: CountedLinkedList<Task<S>, <Task<S> as Link>::Target>,


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.

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!

tokio/src/util/linked_list.rs

Darksonn · 2023-04-20T22:01:36Z

tokio/src/util/linked_list.rs

+    _list: LinkedList<L, T>,
+    _count: AtomicUsize,


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.

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.

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.

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.

Makes sense, all good! 😄

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

Darksonn · 2023-04-26T20:45:16Z

You can fix the CI failures by merging master into your branch.

Darksonn

This looks good to me. Thank you for the PR. I can merge it once it passes the CI checks.

…e-metrics-active-tasks

Resolves a semantic conflict between tokio-rs#5608 and tokio-rs#5628. ref: https://discord.com/channels/500028886025895936/500336346770964480/1100809662208675982

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.28.2` -> `1.29.1` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.28.2` -> `1.29.1` | --- ### Release Notes <details> <summary>tokio-rs/tokio (tokio)</summary> ### [`v1.29.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.29.1): Tokio v1.29.1 [Compare Source](tokio-rs/tokio@tokio-1.29.0...tokio-1.29.1) ##### Fixed - rt: fix nesting two `block_in_place` with a `block_on` between (#5837]) #5837]: tokio-rs/tokio#5837 ### [`v1.29.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.29.0): Tokio v1.29.0 [Compare Source](tokio-rs/tokio@tokio-1.28.2...tokio-1.29.0) Technically a breaking change, the `Send` implementation is removed from `runtime::EnterGuard`. This change fixes a bug and should not impact most users. ##### Breaking - rt: `EnterGuard` should not be `Send` (#5766]) ##### Fixed - fs: reduce blocking ops in `fs::read_dir` (#5653]) - rt: fix possible starvation (#5686], #5712]) - rt: fix stacked borrows issue in `JoinSet` (#5693]) - rt: panic if `EnterGuard` dropped incorrect order (#5772]) - time: do not overflow to signal value (#5710]) - fs: wait for in-flight ops before cloning `File` (#5803]) ##### Changed - rt: reduce time to poll tasks scheduled from outside the runtime (#5705], #5720]) ##### Added - net: add uds doc alias for unix sockets (#5659]) - rt: add metric for number of tasks (#5628]) - sync: implement more traits for channel errors (#5666]) - net: add nodelay methods on TcpSocket (#5672]) - sync: add `broadcast::Receiver::blocking_recv` (#5690]) - process: add `raw_arg` method to `Command` (#5704]) - io: support PRIORITY epoll events (#5566]) - task: add `JoinSet::poll_join_next` (#5721]) - net: add support for Redox OS (#5790]) ##### Unstable - rt: add the ability to dump task backtraces (#5608], #5676], #5708], #5717]) - rt: instrument task poll times with a histogram (#5685]) #5766]: tokio-rs/tokio#5766 #5653]: tokio-rs/tokio#5653 #5686]: tokio-rs/tokio#5686 #5712]: tokio-rs/tokio#5712 #5693]: tokio-rs/tokio#5693 #5772]: tokio-rs/tokio#5772 #5710]: tokio-rs/tokio#5710 #5803]: tokio-rs/tokio#5803 #5705]: tokio-rs/tokio#5705 #5720]: tokio-rs/tokio#5720 #5659]: tokio-rs/tokio#5659 #5628]: tokio-rs/tokio#5628 #5666]: tokio-rs/tokio#5666 #5672]: tokio-rs/tokio#5672 #5690]: tokio-rs/tokio#5690 #5704]: tokio-rs/tokio#5704 #5566]: tokio-rs/tokio#5566 #5721]: tokio-rs/tokio#5721 #5790]: tokio-rs/tokio#5790 #5608]: tokio-rs/tokio#5608 #5676]: tokio-rs/tokio#5676 #5708]: tokio-rs/tokio#5708 #5717]: tokio-rs/tokio#5717 #5685]: tokio-rs/tokio#5685 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1958 Reviewed-by: crapStone <crapstone01@gmail.com> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

github-actions bot added the R-loom Run loom tests on this PR label Apr 16, 2023

Darksonn added A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics labels Apr 16, 2023

runtime: Add a metric that counts currently active tasks

c7d3512

Adds a new metric for the number of active tasks for the Runtime. We count the elements on the OwnedTasks LinkedList, and access that through RuntimeMetrics. Closes: tokio-rs#5400

matildasmeds force-pushed the matildasmeds/runtime-metrics-active-tasks branch from 1a50d95 to c7d3512 Compare April 16, 2023 12:46

Darksonn reviewed Apr 16, 2023

View reviewed changes

tokio/src/util/linked_list.rs Outdated Show resolved Hide resolved

Use atomic counter to track LinkedList length

32343c2

Darksonn reviewed Apr 19, 2023

View reviewed changes

tokio/src/util/linked_list.rs Outdated Show resolved Hide resolved

Add CountedLinkedList type that wraps LinkedList

e757921

Darksonn reviewed Apr 20, 2023

View reviewed changes

matildasmeds and others added 5 commits April 24, 2023 19:13

Update tokio/src/util/linked_list.rs

1110961

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

rustfmt

4157371

Remove underscores from variable names

e7b00b1

remove const from fn def

9292ed2

Make counter non-atomic

041d2a5

Darksonn approved these changes Apr 26, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into matildasmeds/runtim…

c1a03c7

…e-metrics-active-tasks

Darksonn merged commit 1d785fd into tokio-rs:master Apr 27, 2023
57 checks passed

jswrenn mentioned this pull request Apr 27, 2023

Resolve a semantic conflict between #5608 and #5628. #5660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

matildasmeds commented Apr 16, 2023

matildasmeds commented Apr 16, 2023

Darksonn Apr 20, 2023

matildasmeds Apr 24, 2023

Darksonn Apr 20, 2023

Darksonn Apr 24, 2023

matildasmeds Apr 26, 2023

Darksonn Apr 26, 2023

matildasmeds Apr 27, 2023

Darksonn commented Apr 26, 2023

Darksonn left a comment

		struct CountedOwnedTasksInner<S: 'static> {
		list: CountedLinkedList<Task<S>, <Task<S> as Link>::Target>,

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

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

Conversation

matildasmeds commented Apr 16, 2023

matildasmeds commented Apr 16, 2023

Darksonn Apr 20, 2023

Choose a reason for hiding this comment

matildasmeds Apr 24, 2023

Choose a reason for hiding this comment

Darksonn Apr 20, 2023

Choose a reason for hiding this comment

Darksonn Apr 24, 2023

Choose a reason for hiding this comment

matildasmeds Apr 26, 2023

Choose a reason for hiding this comment

Darksonn Apr 26, 2023

Choose a reason for hiding this comment

matildasmeds Apr 27, 2023

Choose a reason for hiding this comment

Darksonn commented Apr 26, 2023

Darksonn left a comment

Choose a reason for hiding this comment