Skip to content

Commit 34d0c75

Browse files
authored
Update barbara_compares_some_cpp_code.md
@nikomatsakis I think this covers all the comments?
1 parent 92cb3bd commit 34d0c75

File tree

1 file changed

+20
-8
lines changed

1 file changed

+20
-8
lines changed

src/vision/status_quo/barbara_compares_some_cpp_code.md

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ also GC'd languages like Python.
1414
This code collates a large number of requests to network services, with each response containing a large amount of data.
1515
To speed this up, Barbara uses `buffer_unordered`, and writes code like this:
1616

17-
```
17+
```rust
1818
let mut queries = futures::stream::iter(...)
1919
.map(|query| async move {
2020
let d: Data = self.client.request(&query).await?;
@@ -31,7 +31,7 @@ Python's [asyncio.wait](https://docs.python.org/3/library/asyncio-task.html#asyn
3131
as well as some code her coworkers have written using c++20's `coroutines`,
3232
using [this](https://github.com/facebook/folly/blob/master/folly/experimental/coro/Collect.h#L321):
3333

34-
```
34+
```C++
3535
std::vector<folly::coro::Task<Data>> tasks;
3636
for (const auto& query : queries) {
3737
tasks.push_back(
@@ -60,7 +60,8 @@ implementation used above, noticing that is works primarily with *tasks*, which
6060
equivalent to rust `Future`'s.
6161
6262
Then it strikes her! `request` is implemented something like this:
63-
```
63+
64+
```rust
6465
impl Client {
6566
async fn request(&self) -> Result<Data> {
6667
let bytes = self.inner.network_request().await?
@@ -77,7 +78,7 @@ This problem hadn't shown up in test cases, where the results from the mocked ne
7778

7879
The solution is to spawn tasks (note this requires `'static` futures):
7980

80-
```
81+
```rust
8182
let mut queries = futures::stream::iter(...)
8283
.map(|query| async move {
8384
let d: Data = tokio::spawn(
@@ -93,18 +94,29 @@ let results = queries.collect::<Vec<Data>>().await;
9394
Barbara was able to figure this out by reading enough and trying things out, but had that not worked, it
9495
would have probably required figuring out how to use `perf` or some similar tool.
9596

96-
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`.
97-
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.
97+
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`:
98+
```rust
99+
impl Client {
100+
async fn request(&self) -> Result<Data> {
101+
let bytes = self.inner.network_request().await?
102+
Ok(tokio::task::block_in_place(move || serialization_libary::from_bytes(&bytes))?)
103+
}
104+
}
105+
```
106+
107+
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.
108+
98109

99110
## 🤔 Frequently Asked Questions
100111

101-
### **Is this actually the correct solution?**
112+
### **Are any of these actually the correct solution?**
102113
* 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:
103114
* This is dependent on how the runtime works.
104-
* `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.
115+
* `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`
105116
* `spawn_blocking`, at least in some executors, cannot be cancelled, a departure from the prototypical cancellation story in async Rust.
106117
* "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.
107118
* The way this problem manifests may be subtle, and it may be specific production load that triggers it.
119+
* 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).
108120
### **What are the morals of the story?**
109121
* Producing concurrent, performant code in Rust async is not always trivial. Debugging performance
110122
issues can be difficult.

0 commit comments

Comments
 (0)