coop: allow disabling cooperative scheduling

[krallin]

With deeply nested FuturesUnordered where most leaf futures are ready,
yields forced by the cooperative scheduling mechanism result in a lot of
useless polls when Tokio attempts to force a return to the runtime.

Indeed, it takes 32 useless polls to exit a single layer of
FuturesUnordered, so with e.g. 5 layers, it takes 32^5 polls (it doesn't
matter how many futures are in the FuturesUnordered instance since it'll
poll 32 times, even if it's the same future being polled 32 times). This
has to be done # total futures / 128 times, which can be a prohibitively
high amount of polling.

In our services, we've seen this result in ~50% of our CPU time being
spent in wake_by_ref() called by poll_proceed. Conversely, we've not
actually seen benefits from using cooperative scheduling, so turning it
off seems like a natural solution (in earlier conversations on other
issues related to cooperative scheduling, it was mentioned that the
Tokio team felt making it configurable could be good).

This commit adds support for doing this in the runtime builder. It
targets the 2.x branch since that is the one we're currently using, but
I'm happy to port this to 1.x.

[Darksonn]

I only had a quick look. Is this the only change in the public API?

In any case, it needs better docs.

[krallin]

There is this and a similar change in LocalSet. I agree this needs more docs — I'm happy to add the polish here as soon as we agree on the general direction :)

Any guidance on the kind of documentation this warrants? The underlying feature didn't have that much I could link to, but let me know what you think?

[Darksonn]

Yeah, making sure we agree on the general direction first is probably good.

Regarding the kind of doc, it could look something like this:

Disable the automated cooperative scheduling feature.

The coop feature limits the number of operations that can be performed before a task yields to the executor. You can read more about the coop feature in the blog post Reducing tail latencies with automatic cooperative task yielding.

Example

use tokio::runtime;

let rt = runtime::Builder::new_multi_thread()
    .enable_all()
    .disable_coop()
    .build()
    .unwrap();

[krallin]

I'll take a look at the failed CI run. I think the field isn't needed in all cfg configurations.

[carllerche]

I just skimmed this. I don't think I want to expose budgets / coop at the runtime level. It seems that the core issue is FuturesUnordered behaves in a way that is not ideal for the coop strategy. So, in practice, we want to disable budgeting for certain non-compatible futures.

An alternate strategy may be providing a Unconstrained<T> future combinator or something that only disables budgeting for that scope. Or maybe an async fn:

mod task {
    async fn unconstrained<T>(f: impl Future<Output=T>) -> T { ... }
}

unconstrained(async {
    // do things that are not compatible w/ Tokio's budgeting
}).await;

Thoughts @tokio-rs/maintainers?

[jonhoo]

I think @taiki-e's patch in rust-lang/futures-rs#2333 will also help a decent amount here, since it'll eliminate the 32-factor in favor of using the actual number of futures.

I personally feel like the "right" way to go about this is to fix up coop so that sub-executors don't cause this problem in the first place, such as by implementing the suggestion I had in the last paragraph of #3493 (comment). That will mitigate the impact most consumers feel today from coop without exposing any external knobs.

That said, having some escape hatch from coop is probably worthwhile, simply because we're unlikely to fix all issues with coop on any kind of known schedule, and people need to have running code in the meantime. I'm not sure how that should be exposed though, given that coop isn't currently exposed in the public API of tokio at all (as @carllerche is getting at). Part of the reason for that is that coop is still not something tokio is committed to long-term, and adding a method for "opting out" of it would commit us to it. Or at least commit to having it documented in the API long-term (we could always deprecate and document as "this option does nothing").

[carllerche]

@jonhoo the API I propose doesn't necessarily constrain us to maintaining the budgeting behavior. If we remove it, the fn becomes a no-op. If we alter it in any way, the API ensures opt-out. It is relatively safe. Either way, we have to maintain some level of budgeting in 1.0 otherwise it would be a breaking change.

[krallin]

Thanks for taking a look everyone!

People need to have running code in the meantime

Indeed — for the time being, we can't use newer versions of Tokio due to this, and we're having to run a forked version instead using this patch, which isn't ideal :(

An alternate strategy may be providing a Unconstrained<T> future combinator or something that only disables budgeting for that scope.

I think that would be a good solution for egregious cases where we can't start our service at all, yes. I do however worry about missing degenerate edge cases or smaller performance issues:

• We might just not spot them.
• Even if we do, root-causing those compatibility issues with Tokio's cooperative mechanism tends to be a time-consuming effort for us, which often requires a profiler (but our profilers for async Rust aren't awesome so there's quite a bit of guesswork involved to find callsites).

I think @taiki-e's patch in rust-lang/futures-rs#2333 will also help a decent amount here, since it'll eliminate the 32-factor in favor of using the actual number of futures.

I have to admit solutions that require changing the ecosystem around Tokio make me a little uneasy. We still have legacy code that's floating here or there and using 0.1 futures (this bit us before), and I'm guessing that won't receive this fix :(

Now, to be clear, I do realize it's not on Tokio to maintain the whole ecosystem (or to support older versions of libraries indefinitely). However, I do think the current design for coop tends to require quite a few "ecosystem-level" fixes. In fact, that's largely where the temptation to just turn it off came from:

• Our codebase is fairly large, and so is our dependency tree. It's not trivial to ensure all of this works well-enough with Tokio's coop. Indeed, we tried the update to Tokio w/ coop 2 times, and ran into 4 different issues (most of which our test coverage was helpless at spotting since tests tend to be structurally a bit simpler). Maybe we just got particularly unlucky though — there's no way to tell.
• On the other hand, there's just one place where we instantiate a runtime. So ... if we simply turn off coop there then our problems sort of go away. That said, that's not great either as now we're diverging from the default configuration upstream (i.e. you) would like to support (that is, Tokio with coop)..

Now, that's where I was coming form, but it sounds like you're saying that the current design might eventually change. This is (welcome!) news to me — I hadn't seen that most recent issue you linked, most of the conversations I had or read around coop date to at least half a year ago :)

Given that, I'm perfectly happy with giving coop another shot and opting out the 1 bit of code I currently know is incompatible with it via an unconstrained wrapper. I'm still a little worried about unseen regressions as I mentioned earlier, but at this point I also don't think it's worth diverging from upstream's default configuration yet.

Accordingly, I'm happy to send a PR to add unconstrained. Any suggestions on where this should live? The coop module is private so I reckon not there? (or should we make it public and expose just that?)

[jonhoo]

I have to admit solutions that require changing the ecosystem around Tokio make me a little uneasy. We still have legacy code that's floating here or there and using 0.1 futures (this bit us before), and I'm guessing that won't receive this fix :(

Now, to be clear, I do realize it's not on Tokio to maintain the whole ecosystem (or to support older versions of libraries indefinitely). However, I do think the current design for coop tends to require quite a few "ecosystem-level" fixes.

The situation here is a little tricky. What's happening is that coop is exposing starvation bugs in other libraries, which is a "Good Thing™", but unfortunately that causes real world code to break. It's not really coop's fault, even though that's what exposes the problem and triggers the faulty behavior in the external library. Part of the problem is that starvation isn't generally part of the async contract, so it's not something most library authors have been thinking about.

I'm very sympathetic to the problem — enabling coop causes your code to perform poorly, so therefore it should be turned off to mitigate the problem. At the same time, disabling coop comes with own problems — specifically, it allows starvation again, which might manifest in other (harder to debug) performance problems, especially under contention. But as you say, fixing the ecosystem takes time, and may even not happen for legacy code. So what to do?

I like the Unconstrained approach because it lets you opt out for code that you specifically know has the problematic behavior, while leaving the remainder of your application still (relatively) safe from task starvation. It's harder to know you have everything covered, that's true, but it feels like a better long-term solution. My personal preference would be to also mark Unconstrained as permanently deprecated so that using it generates a warning (unless you silence it), as over time those wrappers should be removed when the ecosystem fixes land. But that may be too opinionated for tokio proper :)

[taiki-e]

We still have legacy code that's floating here or there and using 0.1 futures (this bit us before), and I'm guessing that won't receive this fix :(

I'm okay with backport the patch to 0.1. (Unlike before, I have permission to merge/publish, so you don't have to wait six months after the patches are filed. I can merge & release them soon:)

UPDATE: Published in 0.1.31

[krallin]

I'm okay with backport the patch to 0.1. (Unlike before, I have permission to merge/publish, so you don't have to wait six months after the patches are filed. I can merge & release them soon:)

Thanks! I sent that backport PR and I see you merged it & published it.

An alternate strategy may be providing a Unconstrained future combinator or something that only disables budgeting for that scope.

This sounds reasonable, and it offers a way to unblock us :) I sent #3547 for this.

enabling coop causes your code to perform poorly, so therefore it should be turned off to mitigate the problem. At the same time, disabling coop comes with own problems — specifically, it allows starvation again, which might manifest in other (harder to debug) performance problems, especially under contention

I have to admit that I just don't think we've really seen or expected that much benefit from coop. I understand the argument that starvation could theoretically happen, and I can totally imagine that for some use cases it could be an improvement. Unfortunately, for us so far, coop really has been the cause of problems — not their solution.

I think a big part of this is that we aren't that latency sensitive (i.e. we generally care a lot more about throughput), and that we usually ensure our services run with enough headroom that we just don't commonly expect all workers to be busy.

Again, I totally understand that others could have a different experience. I just wanted to clarify that at least from where we stand today, wanting to turn off coop isn't a rash decision. It really is what we think is the best option at this time (though at least as far as I'm concerned, my perspective would change if there was buy-in from the ecosystem such that things like FuturesUnordered could be explicitly aware of Tokio's desire to preempt tasks, instead of having to approximate it from seeing all their children yield).

That said, I can totally understand Tokio not wanting to add support for globally disabling this, and that's fair. In the short term, I do however want to make sure we're not having to stay on a forked version of Tokio, so I'm happy to go with an Unconstrained combinator until the situation evolves. I do expect to use it pretty liberally though :)

[carllerche]

I would like to hear what @Darksonn and @jonhoo think about adding an option to disable coop at the runtime level based on this discussion.

[jonhoo]

I think a big part of this is that we aren't that latency sensitive (i.e. we generally care a lot more about throughput), and that we usually ensure our services run with enough headroom that we just don't commonly expect all workers to be busy.

I think this maybe gets at the heart of it. With that kind of arrangement, you generally won't run into problems, though I still don't think you're totally immune. For example (correct me if I'm wrong and this has changed since @carllerche), tokio has an optimization where workers have a "slot" for a future that is not what is currently running and that cannot be stolen. If a future sits in that slot behind a rarely-yielding future, then that future may potentially take a very long time to run next. It's true that this primarily affects latency, but it can introduce large latency spikes in your p99/p999 even if you have plenty of threads/hardware resources.

My biggest concern with adding a coop opt-out is that it's very easy to reach for to "fix" problems, only to then find other performance problems like ^ which then become very hard to debug. But perhaps my worry is unwarranted and we can document this risk well enough on that disabling method. Perhaps we could also give it a name like allow_task_starvation to make it even more obvious. Under those conditions, I think it'd be okay to add.

[krallin]

I think this maybe gets at the heart of it. With that kind of arrangement, you generally won't run into problems, though I still don't think you're totally immune.

That's fair. To be clear, I'm not saying we never would want to use coop, just that for the time being it's primarily caused us pain. We're definitely supportive of the concept, and task starvation is something we are broadly concerned about (I happened to chat with @carllerche earlier this week about this and noted that being able to monitor executor latency & starvation is our top # wish from Tokio, which IMO would help motivate / inform those kinds of decisions).

I do want to reiterate that in the long term, we want to be running Tokio's default configuration if we can. Doing so right now isn't practical for the reasons I mentioned above, but whether we go with a runtime-level flag to opt out, or an unconstrained() wrapper, that is something we would ideally want to remove (as an aside, I suppose that is one argument in favor of unconstrained(): it could prove easier to remove than a global runtime-level flag).

For us, I think it really comes down to whether we should expect the mechanism to evolve over time:

On the one hand, we would not want to use the mechanism as it exists right now because we'd be concerned about code that doesn't work well with it, either because that code is slow but not so slow that we see it, or because it is noticeably slow and we end up spending serious time debugging it. In this case, we really would want a runtime-level opt-out.

On the other hand, if Tokio eventually provides a way to efficiently detect those problems (I would guess count of forced-yields before returning to runtime would be one), or if the design evolves in a way that makes them less likely or eliminates them (like your suggestion to wake things up only once control has returned to the runtime, or by upstreaming the notion of preemption into futures), then we want to be using it. In that case, the question is just about "what temporary solutions do we use until that future arrives?", and I don't think we feel strongly about it at all (edit: assuming that future comes somewhat soon).

---

As a quick aside, adding a runtime-level flag largely amounts to making the current coop feature a runtime decision (as opposed to compile time), right? Technically, that option is already there is just you remove the coop feature, isn't it?

However, unfortunately for us, we can't do that... Given how we pull OSS crates at $work, we can't remove the feature flag without opting out the entire company, and that's no something I want to do (other folks in the company might have a totally different use case, and at the end of the day it's not up to me what the default Tokio experience should be!)

[jonhoo]

Doing so right now isn't practical for the reasons I mentioned above, but whether we go with a runtime-level flag to opt out, or an unconstrained() wrapper, that is something we would ideally want to remove

In some sense, I'm not really worried about you doing the right thing in your code base. I'm worried about the next user of tokio that comes along, finds some problem that they think is due to coop, throw in a no_coop() call on their runtime constructor, and then forgets about it. And then complains about weird tail latency spikes in tokio :) That's why in my comment above I'm trying to find good ways for us to flag that you only want to take the runtime-wide opt-out if you really know what you're doing, you understand the trade-offs, and you want to eventually remove that override when it is possible to do so.

Technically, that option is already there is just you remove the coop feature, isn't it?

I think that's technically true, but yeah, as you observe, in practice it is very difficult to remove a feature since cargo is very liberal about adding them back in 😅

[Darksonn]

I guess now that #3547 is merged, we should just close this?

[jonhoo]

I think that's reasonable. We could add a #[doc(hidden)] runtime-level opt-out, but we should avoid that unless we feel that it's really necessary.

[Darksonn]

Okay. I am closing this for now. If you want to discuss adding this feature further, please go ahead and open a new issue.