-
Notifications
You must be signed in to change notification settings - Fork 89
Barbara compares some cpp code (and has a performance problem) #144
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
Changes from all commits
7906f87
16b484a
a040e02
23085f8
92cb3bd
34d0c75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
# 😱 Status quo stories: Barbara compares some code (and has a performance problem) | ||
|
||
## 🚧 Warning: Draft status 🚧 | ||
|
||
This is a draft "status quo" story submitted as part of the brainstorming period. It is derived from real-life experiences of actual Rust users and is meant to reflect some of the challenges that Async Rust programmers face today. | ||
|
||
If you would like to expand on this story, or adjust the answers to the FAQ, feel free to open a PR making edits (but keep in mind that, as they reflect peoples' experiences, status quo stories [cannot be wrong], only inaccurate). Alternatively, you may wish to [add your own status quo story][htvsq]! | ||
|
||
## The story | ||
|
||
Barbara is recreating some code that has been written in other languages they have some familiarity with. These include C++, but | ||
also GC'd languages like Python. | ||
|
||
This code collates a large number of requests to network services, with each response containing a large amount of data. | ||
To speed this up, Barbara uses `buffer_unordered`, and writes code like this: | ||
|
||
```rust | ||
let mut queries = futures::stream::iter(...) | ||
.map(|query| async move { | ||
let d: Data = self.client.request(&query).await?; | ||
d | ||
}) | ||
.buffer_unordered(32); | ||
|
||
use futures::stream::StreamExt; | ||
let results = queries.collect::<Vec<Data>>().await; | ||
``` | ||
|
||
Barbara thinks this is similar in function to things she has seen using | ||
Python's [asyncio.wait](https://docs.python.org/3/library/asyncio-task.html#asyncio.wait), | ||
as well as some code her coworkers have written using c++20's `coroutines`, | ||
using [this](https://github.com/facebook/folly/blob/master/folly/experimental/coro/Collect.h#L321): | ||
|
||
```C++ | ||
std::vector<folly::coro::Task<Data>> tasks; | ||
for (const auto& query : queries) { | ||
tasks.push_back( | ||
folly::coro::co_invoke([this, &query]() -> folly::coro::Task<Data> { | ||
co_return co_await client_->co_request(query); | ||
} | ||
) | ||
} | ||
auto results = co_await folly:coro::collectAllWindowed( | ||
move(tasks), 32); | ||
``` | ||
|
||
However, *the Rust code performs quite poorly compared to the other impls, | ||
appearing to effectively complete the requests serially, despite on the surface | ||
looking like effectively identical code.* | ||
|
||
While investigating, Barbara looks at `top`, and realises that her coworker's C++20 code sometimes results in her 16 core laptop using 1600% CPU; her Rust async code never exceeds 100% CPU usage. She spends time investigating her runtime setup, but Tokio is configured to use enough worker threads to keep all her CPU cores busy. This feels to her like a bug in `buffer_unordered ` or `tokio`, needing more time to investigate. | ||
|
||
Barbara goes deep into investigating this, spends time reading how `buffer_unordered` is | ||
implemented, how its underlying `FuturesUnordered` is implemented, and even thinks about | ||
how polling and the `tokio` runtime she is using works. She evens tries to figure out if the | ||
upstream service is doing some sort of queueing. | ||
|
||
Eventually Barbara starts reading more about c++20 coroutines, looking closer at the folly | ||
implementation used above, noticing that is works primarily with *tasks*, which are not exactly | ||
equivalent to rust `Future`'s. | ||
|
||
Then it strikes her! `request` is implemented something like this: | ||
|
||
```rust | ||
impl Client { | ||
async fn request(&self) -> Result<Data> { | ||
let bytes = self.inner.network_request().await? | ||
Ok(serialization_libary::from_bytes(&bytes)?) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is still problematic, even with the
or, if the library was normally fast, but slow on some (big?) inputs you might prefer
Goes to show how hard this user story is to get right - the fix in this story still doesn't work well. There's a fun sequence here where you add One that's bitten us in Mononoke (issue #131 here) is that even if the long running work is also nicely async (i.e. just makes another network request), it can still go wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each worker thread has its own (fixed-size) queue of tasks to service. There's a global queue for overflows, too, and a certain amount of rebalancing done by each worker thread to avoid starving the tasks in the global queue. If a worker task goes idle, and the global queue is empty, then it will steal work from other workers. On the other hand, if you have a small system (say an older laptop) with 4 harts (two cores, two threads per core), and you have still have 100 tasks to run, without I'm guessing that you've never see the starvation issue because you normally have lots of worker threads, and not many runnable tasks at a time - as a result, a worker goes idle and steals work from the blocked thread. If you had small numbers of worker threads, but still lots of tasks, you'd see starvation more often. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds like it might make a nice addition to the story. e.g., Barbara is happy for a while, but then she sees starvation or something like that. That said, it could also be a good FAQ. I wouldn't change the main part of the story, though, since invoking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-reading this comment I was definitely struck by the challenge of "how to jigger this exactly right". I'm curious, @farnz, whether you think that a more adaptive runtime -- similar to what Go does, or what was proposed in this blog post, but never adopted -- would be a better fit for Mononoke? One of the challenges here seems to be a matter of composition. The "right place" to insert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I guess I was wrong about that last bit. Still, it seems like these kinds of issues could frequently cross library boundaries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh also I see you referenced async-rs/async-std#631 in the FAQ :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced by more adaptive runtimes - they tend to result in developers not realising when they've introduced accidental blocking, and I have a bias towards engineers knowing the cost of their decisions. There are two orthogonal problems that I perceive here:
For point 2 there, The missing bit in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @farnz. Helpful. |
||
} | ||
} | ||
``` | ||
|
||
The results from the network service are sometimes (but not always) VERY large, and the `BufferedUnordered` stream is contained within 1 tokio task. | ||
**The request future does non-trivial cpu work to deserialize the data. | ||
This causes significant slowdowns in wall-time as the the process CAN BE bounded by the time it takes | ||
the single thread running the tokio-task to deserialize all the data.** | ||
This problem hadn't shown up in test cases, where the results from the mocked network service are always small; many common uses of the network service only ever have small results, so it takes a specific production load to trigger this issue, or a large scale test. | ||
|
||
The solution is to spawn tasks (note this requires `'static` futures): | ||
|
||
```rust | ||
let mut queries = futures::stream::iter(...) | ||
.map(|query| async move { | ||
let d: Data = tokio::spawn( | ||
self.client.request(&query)).await??; | ||
d | ||
}) | ||
.buffer_unordered(32); | ||
|
||
use futures::stream::StreamExt; | ||
let results = queries.collect::<Vec<Data>>().await; | ||
``` | ||
|
||
Barbara was able to figure this out by reading enough and trying things out, but had that not worked, it | ||
would have probably required figuring out how to use `perf` or some similar tool. | ||
|
||
nikomatsakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Later on, Barbara gets surprised by this code again. It's now being used as part of a system that handles a very high number of requests per second, but sometimes the system stalls under load. She enlists Grace to help debug, and the two of them identify via `perf` that all the CPU cores are busy running `serialization_libary::from_bytes`. Barbara revisits this solution, and discovers `tokio::task::block_in_place` which she uses to wrap the calls to `serialization_libary::from_bytes`: | ||
```rust | ||
impl Client { | ||
async fn request(&self) -> Result<Data> { | ||
let bytes = self.inner.network_request().await? | ||
Ok(tokio::task::block_in_place(move || serialization_libary::from_bytes(&bytes))?) | ||
} | ||
} | ||
``` | ||
|
||
This resolves the problem as seen in production, but leads to Niklaus's code review suggesting the use of `tokio::task::spawn_blocking` inside `request`, instead of `spawn` inside `buffer_unordered`. This discussion is challenging, because the tradeoffs between `spawn` on a `Future` including `block_in_place` and `spawn_blocking` and then not spawning the containing `Future` are subtle and tricky to explain. Also, either `block_in_place` and `spawn_blocking` are heavyweight and Barbara would prefer to avoid them when the cost of serialization is low, which is usually a runtime-property of the system. | ||
|
||
|
||
## 🤔 Frequently Asked Questions | ||
|
||
### **Are any of these actually the correct solution?** | ||
* Only in part. It may cause other kinds of contention or blocking on the runtime. As mentioned above, the deserialization work probably needs to be wrapped in something like [`block_in_place`](https://docs.rs/tokio/1/tokio/task/fn.block_in_place.html), so that other tasks are not starved on the runtime, or might want to use [`spawn_blocking`](https://docs.rs/tokio/1/tokio/task/fn.spawn_blocking.html). There are some important caveats/details that matter: | ||
* This is dependent on how the runtime works. | ||
* `block_in_place` + `tokio::spawn` might be better if the caller wants to control concurrency, as spawning is heavyweight when the deserialization work happens to be small. However, as mentioned above, this can be complex to reason about, and in some cases, may be as heavyweight as `spawn_blocking` | ||
* `spawn_blocking`, at least in some executors, cannot be cancelled, a departure from the prototypical cancellation story in async Rust. | ||
* "Dependently blocking work" in the context of async programming is a hard problem to solve generally. https://github.com/async-rs/async-std/pull/631 was an attempt but the details are making runtime's agnostic blocking are extremely complex. | ||
* The way this problem manifests may be subtle, and it may be specific production load that triggers it. | ||
* The outlined solutions have tradeoffs that each only make sense for certain kind of workloads. It may be better to expose the io aspect of the request and the deserialization aspect as separate APIs, but that complicates the library's usage, lays the burden of choosing the tradeoff on the callee (which may not be generally possible). | ||
### **What are the morals of the story?** | ||
* Producing concurrent, performant code in Rust async is not always trivial. Debugging performance | ||
issues can be difficult. | ||
* Rust's async model, particularly the blocking nature of `polling`, can be complex to reason about, | ||
and in some cases is different from other languages choices in meaningful ways. | ||
* CPU-bound code can be easily hidden. | ||
|
||
### **What are the sources for this story?** | ||
* This is a issue I personally hit while writing code required for production. | ||
|
||
### **Why did you choose *Barbara* to tell this story?** | ||
That's probably the person in the cast that I am most similar to, but Alan | ||
and to some extent Grace make sense for the story as well. | ||
|
||
### **How would this story have played out differently for the other characters?** | ||
* Alan: May have taken longer to figure out. | ||
* Grace: Likely would have been as interested in the details of how polling works. | ||
* Niklaus: Depends on their experience. |
Uh oh!
There was an error while loading. Please reload this page.