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

merged 9 commits into from
Apr 27, 2023

Conversation

matildasmeds
Copy link
Contributor

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

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Apr 16, 2023
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics labels Apr 16, 2023
@matildasmeds
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 matildasmeds force-pushed the matildasmeds/runtime-metrics-active-tasks branch from 1a50d95 to c7d3512 Compare April 16, 2023 12:46
@matildasmeds
Copy link
Contributor Author

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. 🙏

Darksonn
Darksonn reviewed Apr 20, 2023
View reviewed changes
tokio/src/runtime/task/list.rs Show resolved Hide resolved
tokio/src/util/linked_list.rs Outdated Show resolved Hide resolved
tokio/src/util/linked_list.rs Outdated
Comment on lines 237 to 238
_list: LinkedList<L, T>,
_count: AtomicUsize,
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! 😄

@Darksonn
Copy link
Contributor

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

Darksonn
Darksonn approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

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

@Darksonn Darksonn merged commit 1d785fd into tokio-rs:master Apr 27, 2023
jswrenn added a commit to jswrenn/tokio that referenced this pull request Apr 27, 2023
@jswrenn
tokio: add CountedLinkedList::for_each
0f1fba5 
Resolves a semantic conflict between tokio-rs#5608 and tokio-rs#5628.

ref: https://discord.com/channels/500028886025895936/500336346770964480/1100809662208675982
@jswrenn jswrenn mentioned this pull request Apr 27, 2023
Merged
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jul 6, 2023
@crapStone
Update Rust crate tokio to 1.29.1 (#1958)
03776c3 
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 (#&#8203;5837])

#&#8203;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` (#&#8203;5766])

##### Fixed

-   fs: reduce blocking ops in `fs::read_dir` (#&#8203;5653])
-   rt: fix possible starvation (#&#8203;5686], #&#8203;5712])
-   rt: fix stacked borrows issue in `JoinSet` (#&#8203;5693])
-   rt: panic if `EnterGuard` dropped incorrect order (#&#8203;5772])
-   time: do not overflow to signal value (#&#8203;5710])
-   fs: wait for in-flight ops before cloning `File` (#&#8203;5803])

##### Changed

-   rt: reduce time to poll tasks scheduled from outside the runtime (#&#8203;5705], #&#8203;5720])

##### Added

-   net: add uds doc alias for unix sockets (#&#8203;5659])
-   rt: add metric for number of tasks (#&#8203;5628])
-   sync: implement more traits for channel errors (#&#8203;5666])
-   net: add nodelay methods on TcpSocket (#&#8203;5672])
-   sync: add `broadcast::Receiver::blocking_recv` (#&#8203;5690])
-   process: add `raw_arg` method to `Command` (#&#8203;5704])
-   io: support PRIORITY epoll events (#&#8203;5566])
-   task: add `JoinSet::poll_join_next` (#&#8203;5721])
-   net: add support for Redox OS (#&#8203;5790])

##### Unstable

-   rt: add the ability to dump task backtraces (#&#8203;5608], #&#8203;5676], #&#8203;5708], #&#8203;5717])
-   rt: instrument task poll times with a histogram (#&#8203;5685])

#&#8203;5766]: tokio-rs/tokio#5766

#&#8203;5653]: tokio-rs/tokio#5653

#&#8203;5686]: tokio-rs/tokio#5686

#&#8203;5712]: tokio-rs/tokio#5712

#&#8203;5693]: tokio-rs/tokio#5693

#&#8203;5772]: tokio-rs/tokio#5772

#&#8203;5710]: tokio-rs/tokio#5710

#&#8203;5803]: tokio-rs/tokio#5803

#&#8203;5705]: tokio-rs/tokio#5705

#&#8203;5720]: tokio-rs/tokio#5720

#&#8203;5659]: tokio-rs/tokio#5659

#&#8203;5628]: tokio-rs/tokio#5628

#&#8203;5666]: tokio-rs/tokio#5666

#&#8203;5672]: tokio-rs/tokio#5672

#&#8203;5690]: tokio-rs/tokio#5690

#&#8203;5704]: tokio-rs/tokio#5704

#&#8203;5566]: tokio-rs/tokio#5566

#&#8203;5721]: tokio-rs/tokio#5721

#&#8203;5790]: tokio-rs/tokio#5790

#&#8203;5608]: tokio-rs/tokio#5608

#&#8203;5676]: tokio-rs/tokio#5676

#&#8203;5708]: tokio-rs/tokio#5708

#&#8203;5717]: tokio-rs/tokio#5717

#&#8203;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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNi4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=-->

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carllerche carllerche carllerche left review comments

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics R-loom Run loom tests on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API for number of active tokio tasks?
3 participants
@matildasmeds @Darksonn @carllerche