-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 core::task::yield_now #74335
Add core::task::yield_now #74335
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
Note that the same feature request has been submitted to |
@taiki-e Thanks for sharing both links. They're interesting reads I hadn't seen before. However I'm not sure to which degree they cover the reasoning I laid out in my opening post. Could you perhaps elaborate? In particular I have questions about smol-rs/smol#43 (comment). The comment is constructed based off the following assumption:
I'm not aware of any such executor in the ecosystem built on futures? Throughput-oriented runtimes such as rayon don't build on futures as a primitive and to my knowledge don't intend to either. In practice we know that
To me this sounds about accurate? My hopes with this PR is to consolidate the multiple implementations we're currently seeing in the ecosystem into the stdlib. These implementations all do the exact same thing: wake the waker, and return |
It is not clear what "correctly" means here. Whether this means "looks like it's working, but it's actually a no-op" or "it behaves as the user expected" should depend on the executor. (see issue and pr linked in my previous comment)
Note that these are implementation details. There is no guarantee in the future that those executors will be compatible with the current implementation of |
I don't see the issue. This construct yields once and wakes, how the executor its run in responds to that yielding is that executor's business. Maybe there needs to be a safety note about this, but it isn't a problem for the construct. The OS you're running on could just as easily no-op Main question to me is whether or not this construct is useful. |
Oops, sorry! Those are rust-lang/futures-rs#1430 and smol-rs/smol#43 (comment) that linked in my previous comment. |
Sorry, I meant "I don't see the problem." This construct would have well defined semantics, its the executor's concern how it handles it and its fine if its different on different executors. |
@withoutboats On usefulness of
|
So I think what @taiki-e is saying is something like this: The intent of invoking In other words, of course all executors will do "something" when That said, it'd be nice to have some more concrete counterexample. It seems like async-std used to have a different Otherwise, I imagine it would have to be a scheduler that was expecting some kind of "extra-protocol" around futures to help them specify how they'd like to be scheduled, and which behaved poorly with generic futures like I guess an example might be that you might have a runtime that has some special case check for "a future returned (More generally, I could imagine that if we start to see a need for more "scheduling hints" of this kind, it's precisely the kind of thing we could add by extending the Anyway I guess the TL;DR is that adding this construct kind of "locks in" (in a soft way...) the idea that when a future returns (@taiki-e, is this a fair summary?) |
Is that any different from See also Linus' rant on misuse of
If they are merely hints then they only serve to improve latency, which shouldn't be as relevant to some sort of throughput scheduler. If progress strictly requires another future to run then a hint is not sufficient and async-aware synchronization mechanisms should be used instead. Additional hints only make sense if you have tiered or priority-based scheduling or want to jump ahead of the queue (e.g. javascript's micro vs. macro tasks), but those would be scheduler-specific. |
I also agree that having no effect in some executors is a perfectly fine behavior for this code. Putting something as a method on |
Adding things to What I was imagining is that one could imagine, in some future, that In any case, my point is more to say "if this were a problem we cared to address, we have some means of doing so", I don't really see it as a problem now. It seems to me that |
One disadvantage of task yielding as implemented here is that it tends to yield execution only within a scope of the innermost executor, rather than the outermost, which arguably would be more useful. In particular, I am referring to embeddable executors like FuturesUnordered, which use their own wakers, and their own scheduling queue which is polled until empty. When combined with yielding as implemented here, they won't yield processing to tasks outside, only to those within. If there are no other ready tasks in FuturesUnordered, yield_now has the opposite effect to the desired one. For additional discussion see rust-lang/futures-rs#2053. (I am referring to FuturesUnordered as it existed before it was equipped with an arbitrary polling limit). An actual executor would be in position to provide a yielding primitive with stronger guarantees, working in conditions described above, e.g., by stashing the wakers for invocation on the next tick of the executor rather than doing it immediately. I think that the documentation for yield_now should describe how the implementation works, mention that actual behaviour is executor specific, and yielding is advisory in nature. |
That was a hack I did until we implemented yielding 'properly'. So the condition for 'proper' yielding is: Has the task been woken while it was being polled? If yes, then the task is yielding. For async-std, that means we put the task at the back of the task queue. We didn't have an elegant way of doing this check before so I just hacked up a poor version of the yielding condition: Has
The solution seems straightforward to me: we need to add the check to |
☔ The latest upstream changes (presumably #74493) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased to resolve the merge conflict, and addressed @jyn514's feedback. |
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
So I think that @stjepang's comment is interesting -- it seems to be mentioning an "implied" or "hidden" heuristic of "if the task was awoken during its poll, we should not re-awaken it immediately". This feels like it merits mention in the documentation of the |
@yoshuawuyts |
Updated to resolve merge conflicts!
@nikomatsakis great point; will file a separate PR for that. |
Triage: It seems this is kind of blocked... I'm going to mark this as such and add corresponding wg labels. |
That actually surprised me, but I think that may be because we have different backgrounds. I suspect the difference between async Rust and other languages is that cooperative scheduling in Rust is only required to enable other tasks to run intermittently, but not used as a tool to guarantee correctness. For example in Node.js Still though, that does still leave the use case open for manually yielding in order to break up computations. I suspect this may be most desired in systems that have specific time limits. For example in web browsers (the example I'm most comfortable with) no computation should ever exceed 8ms total, or else frames are dropped because the main thread doesn't get the opportunity to render. This makes it essential to be able to yield back to the event loop periodically. Though taking a step back to this PR: I guess what we're indeed doing here is more deeply tied to the question: "How does cooperative scheduling work in async Rust?" which I guess we've only ever partially defined. I think this is a topic that probably warrants a blog post. |
This is may be true in a single-threaded environment. But in a multi-threaded environment the solutions usually look different. E.g. separate task pools (e.g. compute and IO) or tasks marked as background jobs which can only take up to a certain fraction of the available threads so that there remaining threads available for time-critical tasks. Yielding seems more like a hack. |
@the8472 Yes, I'm aware. And even then it's common to periodically yield back to the runtime. Spawning tasks is not free. Runtimes are different and it can be risky to make assumptions about the scheduling behavior of different runtimes. Especially in Rust, where a Future is different from a Task, it's useful to be able to yield back to the runtime from within a Future that does did not spawn the task it's running in. Being able to voluntarily yield back to the executor is a basic capability for any system that use non-preemptive concurrency. Even a search for "cooperative scheduling" will yield back the following definition as the first result:
Wikipedia: Cooperative multitasking, emphasis mine. I think it's important that we pay our dues and ensure that providing a way to yield back to the executor works correctly. So far the review process in this thread has been incredibly helpful in assuring that. But I don't know how else to say that we are in fact missing basic functionality here. |
This came up again in Zulip today in the context of OS development. |
Did you ever file that PR? I feel like actually this change belongs in this PR, because this PR would be the one establishing and committing to that convention. |
@nikomatsakis I did not; agree that that would make sense — though wondering if it may actually warrant an RFC since it specs out a part of Rust's Futures model? |
Yeah, that's a good question. I guess I would leave that up to @rust-lang/libs to decide as it feels like a libs matter to me. |
|
||
// Most futures executors are implemented as a FIFO queue, so all this | ||
// future does is re-schedule the future back to the end of the queue, | ||
// giving room for other futures to progress. |
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.
Most futures executors are implemented as a FIFO queue
Shouldn't this assumption be mentioned in the doc comments?
This is modeled after |
r? @sfackler I've already voiced my opinion in the comments above, and I do think this is important to resolve. I've assigned @sfackler in the hopes that they can represent the libs team here. I would also still love input from @carllerche or someone else familiar with the tokio executor internals to give input on how feasible it would be to add the above guarantee to tokio. |
I discussed this issue with @carllerche today. They're skeptical about the idea of adding This strategy could also be made to cooperate with intermediate task containers such as impl core::task::Context<'a> {
fn from_waker_and_budget(waker: &'a Waker, budget: &'a mut usize) -> Self { ... }
// consider deprecating `from_waker`
fn has_budget(&self) -> bool {
match self.budget {
None | Some(x) if *x > 0 => true,
Some(_) => false,
}
}
fn spend_budget(&mut self) -> Poll<_> {
if self.has_budget() {
*self.budget.unwrap() -= 1;
Poll::Ready(())
} else {
Poll::Pending
}
}
} "Leaf futures" (those that wait on asynchronous work, including async locks, channels, IO objects, timers, etc.) would then be in charge of calling Personally, I think the best next step would be for interested folks to discuss higher-bandwidth, perhaps in the async foundations zulip channel and come up with a comprehensive design for task pre-emption which could then be written up as an RFC. I think there's risk to adding one-off functionality like P.S. As for the guarantee I suggested about executors running other ready tasks before the current one if Edit: we'd also probably want a plain |
/// Calling this function calls move the currently executing future to the back | ||
/// of the execution queue, making room for other futures to execute. This is |
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.
Calling this function doesn’t do anything, awaiting the returned future does. The rest of this sentence sounds like a definitive guarantee whereas the comment in the code says "most executors" which is a much weaker.
This doc should probably be rephrased along the lines of:
Awaiting the future returned by this function yields control back to the executor, giving it an opportunity to run other tasks. Executors typically move the current task to the back of their execution queue.
☔ The latest upstream changes (presumably #79927) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
Good, that's what I wanted to confirm. The shared designs certainly are interesting, and I agree we should talk about this more. Going to close this issue for now then in favor of speccing out Futures pre-emption through the RFC process. Thanks everyone! |
I would be very interested in the "comprehensive design for task pre-emption" discussion. This is really cool stuff, everyone! :) |
Giving the runtime the opportunity to schedule other tasks was debated at length in this issue, but one thing I didn't see raised is the ability to use We have what is effectively a database where a lot of the internals are implemented with futures. We wrap everything we dispatch to the runtime in a small wrapper future struct that is connected to the specific query that triggered it. Each time the outermost future is polled, it checks to see if the query has been cancelled, and if it has, it returns For us something like |
Adds
core::task::yield_now
as a counterpart tostd::thread::yield_now
. Tracking issue is #74331.cc/ @rust-lang/wg-async-foundations
Motivation
Schedulers generally infer when tasks should be run to optimize performance. However some of the time the programmer has more information than the scheduler, and may want to manually yield back to the scheduler to enable other code to run. For threads this mechanism is
std::thread::yield_now
.This PR introduces a counterpart to
thread::yield_now
in the form oftask::yield_now
. The way this works is that the first time the future is polled it returnsPoll::Pending
and immediately callsWaker::wake
.Poll::Pending
signals to executors that the current task is not ready and will be suspended, while the call toWaker::wake
then immediately puts it at the back of the execution queue.This somewhat depends on behavior of executors; but in practice this is compatible with all executors in the ecosystem right now. Both
async_std::task::yield_now
andtokio::task::yield_now
share similar implementations. With neither implementation being bound to any particular runtime, this is a good candidate to start trialing in nightly.Implementation notes
Much like
core::future::ready
andcore::future::pending
this function returns a concrete future rather than being implemented as anasync fn
. This enables it to be inspected and stored inside structs, which is a useful property for types found in the stdlib.We also make note of the concept of an "executor", which has precedent in the
Wake
trait.References
tokio
Add a yield primitive for computation-heavy tasks tokio-rs/tokio#592async_std
Add task::yield_now as "unstable" async-rs/async-std#300async_std
stabilize task::yield_now async-rs/async-std#514