driver: serialize /solve requests in background task#4187
driver: serialize /solve requests in background task#4187MartinquaXD wants to merge 4 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request moves the serialization of /solve requests to a background task using tokio::task::spawn_blocking to prevent blocking executor threads. While the overall approach is sound, the error handling in the solve method is problematic as it uses .expect() on the background task's result, which can lead to process-wide panics on serialization failures. This review focuses on improving the robustness of this error handling to align with the repository's technical standards for logic and reliability.
| let body = tokio::task::spawn_blocking(move || { | ||
| // pre-allocate a big enough buffer to avoid re-allocating memory | ||
| // as the request gets serialized | ||
| const BYTES_PER_ORDER: usize = 1_300; | ||
| let mut buffer = Vec::with_capacity(auction_dto.orders.len() * BYTES_PER_ORDER); | ||
| serde_json::to_writer(&mut buffer, &auction_dto).unwrap(); | ||
| bytes::Bytes::from(buffer) | ||
| }) | ||
| .await | ||
| .expect("errors bubble up"); |
There was a problem hiding this comment.
How long does it really take if it needs to use a separate thread pool?
There was a problem hiding this comment.
@jmg-duarte shared an interesting article https://ryhl.io/blog/async-what-is-blocking/, which explains why spawn_blocking might be suboptimal for non i/o jobs like CPU computations with no sleeps.
IIUC, for CPU-bound tasks, the best throughput is usually when the number of worker threads ~ number of CPU cores. While the blocking thread pool has up to 500 worker threads, which exceeds the number of CPU cores, this can lead to performance issues.
There was a problem hiding this comment.
I think we can merge this with the blocking thread pool right now and add a rayon one later on for these tasks
There was a problem hiding this comment.
Serializing the request currently takes ~90ms.
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
Ilya raises a valid point with the expect/unwrap
Other than that, this looks good
…er-request-in-blocking-task
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
3 participants
Description
Moves the serialization of the driver's
/solverequest to a blocking background task to not block any executor threads too long.