-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add task::consume_budget util for cooperative scheduling #4498
Conversation
f34981b
to
5c0bbc6
Compare
I think this is probably a good feature to add. I would probably not name it "maybe_yield_now()". Instead, name it something to reflect that it represents conceptually tracking work.
^^ Don't love either of those names, but yeah. |
Right, the name should reflect that there's a side effect - burning budget. I'll figure out a new name and send it in v2, along with the tests |
5c0bbc6
to
adaa0f5
Compare
v2:
|
7ae1285
to
9f0e842
Compare
@jonhoo I would love to get your thoughts on naming and the feature in general. |
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.
Overall, I think something like this is probably a good idea. I had some minor nitpicks and suggestions, but they're not hard blockers.
If we want to avoid a very long bikeshed over this API, it occurs to me that we could just go ahead and ship it as an unstable feature, so people can start experimenting with it, and then we can come back and stabilize the API separately?
tokio/src/task/consume_budget.rs
Outdated
/// Consumes a unit of budget and returns the execution back to the Tokio | ||
/// runtime *if* the task went out ouf budget. | ||
/// | ||
/// The task will only yield if it ran out of its coop budget. | ||
/// It can be used in order to insert optional yield points into long | ||
/// computations that do not use Tokio resources like sockets or semaphores, | ||
/// without redundantly yielding to runtime each time. |
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.
one note about the documentation for this is that this is the first actual mention of the cooperative scheduling budget in the API documentation. I think it is probably worth adding a high-level summary of the task budget somewhere in the tokio::task
documentation and having this API documentation reference that? Currently, the task budget is basically only documented in this blog post which seems maybe not ideal...
/// computations that do not use Tokio resources like sockets or semaphores, | ||
/// without redundantly yielding to runtime each time. | ||
#[cfg_attr(docsrs, doc(cfg(feature = "rt")))] | ||
pub async fn consume_budget() { |
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.
i wonder if we should pre-emptively mark this as an unstable feature, so that we can ship it sooner in the next release while reserving the ability to bikeshed the API for a bit longer?
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.
Could you point me to a code example showing how to mark a feature as unstable?
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.
nvm, just found #4499, I'll follow it as a guideline
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.
If I view the generated docs, then it is not listed as unstable. You should add that to the doc(cfg(..))
.
#[cfg_attr(docsrs, doc(cfg(all(tokio_unstable, feature = "rt"))))]
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.
done, thanks!
tokio/src/task/consume_budget.rs
Outdated
struct ConsumeBudget { | ||
status: Poll<()>, | ||
} | ||
|
||
impl Future for ConsumeBudget { | ||
type Output = (); | ||
|
||
fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<()> { | ||
if self.status.is_ready() { | ||
return self.status; | ||
} | ||
self.status = crate::coop::poll_proceed(cx).map(|restore| { | ||
restore.made_progress(); | ||
}); | ||
self.status | ||
} | ||
} | ||
|
||
ConsumeBudget { status: Poll::Pending }.await |
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.
nit, take it or leave it: i think we could probably implement this a bit more simply using poll_fn
:
struct ConsumeBudget { | |
status: Poll<()>, | |
} | |
impl Future for ConsumeBudget { | |
type Output = (); | |
fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<()> { | |
if self.status.is_ready() { | |
return self.status; | |
} | |
self.status = crate::coop::poll_proceed(cx).map(|restore| { | |
restore.made_progress(); | |
}); | |
self.status | |
} | |
} | |
ConsumeBudget { status: Poll::Pending }.await | |
let mut status = Poll::Pending; | |
crate::future::poll_fn(move |cx| { | |
if status.is_ready() { | |
return status; | |
} | |
status = crate::coop::poll_proceed(cx).map(|restore| { | |
restore.made_progress(); | |
}); | |
status | |
}).await |
i believe this should work identically, and avoids a little bit of boilerplate by not having to impl Future
for a type.
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.
Neat! Looks much better, thanks
tokio/src/task/consume_budget.rs
Outdated
use std::task::{Context, Poll}; | ||
|
||
/// Consumes a unit of budget and returns the execution back to the Tokio | ||
/// runtime *if* the task went out ouf budget. |
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.
nit: how about
/// runtime *if* the task went out ouf budget. | |
/// runtime *if* the task's coop budget was exhausted. |
tokio/src/task/consume_budget.rs
Outdated
/// The task will only yield if it ran out of its coop budget. | ||
/// It can be used in order to insert optional yield points into long |
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.
nit: maybe
/// The task will only yield if it ran out of its coop budget. | |
/// It can be used in order to insert optional yield points into long | |
/// The task will only yield if its entire coop budget has been exhausted. | |
/// This function can be used in order to insert optional yield points into long |
tokio/src/task/consume_budget.rs
Outdated
/// The task will only yield if it ran out of its coop budget. | ||
/// It can be used in order to insert optional yield points into long | ||
/// computations that do not use Tokio resources like sockets or semaphores, | ||
/// without redundantly yielding to runtime each time. |
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.
nit:
/// without redundantly yielding to runtime each time. | |
/// without redundantly yielding to the runtime each time. |
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.
also, it could be nice to have an example in the documentation, maybe showing this in a long-running loop or something?
We can always make this a no-op down the line should we decide that coop isn't a thing we want, and make it forward to an external lib if that's where coop ends up living, so I'm 👍 on this in general. In fact, a method like this existed in the original implementation in #2160, initially called |
399968e
to
613d125
Compare
v3:
I also retested everything with the additional |
This failure in CI/test tokio full run looks unrelated to my patch - it errors out on some macro tests that I haven't touched and I'm able to reproduce the same failures on current master branch. |
Those errors are due to the previous release of rustc. Please rebase or merge master into your branch to fix them. |
got it, rebased, thanks |
(ping) |
(rebased on top of newest master) |
(monthly rebase) |
For cpu-only computations that do not use any Tokio resources, budgeting does not really kick in in order to yield and prevent other tasks from starvation. The new mechanism - consume_budget, performs a budget check, consumes a unit of it, and yields only if the task exceeded the budget. That allows cpu-intenstive computations to define points in the program which indicate that some significant work was performed. It will yield only if the budget is gone, which is a much better alternative to unconditional yielding, which is a potentially heavy operation.
The test case ensures that the task::consume_budget utility actually burns budget and makes the task yield once the whole budget is gone.
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.
There are some other coop APIs that I wonder if they are more important to add. (Mainly, a public version of our poll_proceed
.) However, this function does seem useful too, and it is marked as unstable, so it seems fine to just go ahead and merge this.
Sorry for taking so long. |
Thanks! No worries, I often take active part in review processes that take years to finish, this one was actually quite painless and swift (: |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.18.2` -> `1.19.1` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.18.2` -> `1.19.1` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.19.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.1) [Compare Source](tokio-rs/tokio@tokio-1.19.0...tokio-1.19.1) ##### 1.19.1 (June 5, 2022) This release fixes a bug in `Notified::enable`. ([#​4747]) [#​4747]: tokio-rs/tokio#4747 ### [`v1.19.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.0) [Compare Source](tokio-rs/tokio@tokio-1.18.2...tokio-1.19.0) ##### 1.19.0 (June 3, 2022) ##### Added - runtime: add `is_finished` method for `JoinHandle` and `AbortHandle` ([#​4709]) - runtime: make global queue and event polling intervals configurable ([#​4671]) - sync: add `Notified::enable` ([#​4705]) - sync: add `watch::Sender::send_if_modified` ([#​4591]) - sync: add resubscribe method to broadcast::Receiver ([#​4607]) - net: add `take_error` to `TcpSocket` and `TcpStream` ([#​4739]) ##### Changed - io: refactor out usage of Weak in the io handle ([#​4656]) ##### Fixed - macros: avoid starvation in `join!` and `try_join!` ([#​4624]) ##### Documented - runtime: clarify semantics of tasks outliving `block_on` ([#​4729]) - time: fix example for `MissedTickBehavior::Burst` ([#​4713]) ##### Unstable - metrics: correctly update atomics in `IoDriverMetrics` ([#​4725]) - metrics: fix compilation with unstable, process, and rt, but without net ([#​4682]) - task: add `#[track_caller]` to `JoinSet`/`JoinMap` ([#​4697]) - task: add `Builder::{spawn_on, spawn_local_on, spawn_blocking_on}` ([#​4683]) - task: add `consume_budget` for cooperative scheduling ([#​4498]) - task: add `join_set::Builder` for configuring `JoinSet` tasks ([#​4687]) - task: update return value of `JoinSet::join_one` ([#​4726]) [#​4498]: tokio-rs/tokio#4498 [#​4591]: tokio-rs/tokio#4591 [#​4607]: tokio-rs/tokio#4607 [#​4624]: tokio-rs/tokio#4624 [#​4656]: tokio-rs/tokio#4656 [#​4671]: tokio-rs/tokio#4671 [#​4682]: tokio-rs/tokio#4682 [#​4683]: tokio-rs/tokio#4683 [#​4687]: tokio-rs/tokio#4687 [#​4697]: tokio-rs/tokio#4697 [#​4705]: tokio-rs/tokio#4705 [#​4709]: tokio-rs/tokio#4709 [#​4713]: tokio-rs/tokio#4713 [#​4725]: tokio-rs/tokio#4725 [#​4726]: tokio-rs/tokio#4726 [#​4729]: tokio-rs/tokio#4729 [#​4739]: tokio-rs/tokio#4739 </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. --- - [x] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- 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/1394 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Motivation
For cpu-only computations that do not use any Tokio resources, budgeting does not really kick in in order to yield and prevent other tasks from starvation. One way to take part in budgeting is to use a Tokio resource, but some workloads simply don't need any, as they just need to compute a lot and occasionally yield in order to keep latencies low (e.g. a Wasm engine).
Solution
The new mechanism -
task::consume_budget
, performs a budget check and consumes a unit of it as if a Tokio resource was used (except it wasn't, so it's cheaper) and yields only if the task exceeded the budget. That allows cpu-intenstive computations to define yield points that yield only conditionally, since unconditional yielding to runtime is a potentially heavy operation.