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
No critical issues found. The PR correctly optimizes the auction serialization process by offloading it to a blocking task and removing an unnecessary intermediate conversion to JsonValue. These changes align with the performance goals stated in the description and follow Rust best practices for handling CPU-intensive tasks in an async environment.
|
LGTM just fix the tests and should be G2G |
There was a problem hiding this comment.
Code Review
The pull request optimizes the auction persistence process by serializing the auction DTO directly to a JSON string and offloading this operation to a blocking task. This change eliminates an unnecessary intermediate conversion to JsonValue and prevents the async runtime threads from being blocked by the potentially slow serialization process. No critical issues found.
| .start_timer(); | ||
|
|
||
| let data = serde_json::to_value(new_auction_data)?; | ||
| let data = tokio::task::spawn_blocking(move || serde_json::to_string(&new_auction_data)) |
There was a problem hiding this comment.
Any estimates regarding how "heavy" this operation is would be helpful in the pr description to justify the usage of a separate thread pool.
There was a problem hiding this comment.
If this operation takes more than 100ms it's already more than eligible for the blocking thread pool
Martin mentions in the description that:
the entire process often takes more than 1s
So this sounds very reasonable
There was a problem hiding this comment.
My bad, i missed that somehow. Anyway, my concern is explained here #4187 (comment)
There was a problem hiding this comment.
According to some tokio blog I currently fail to find anything over 100µm run time should be considered a blocking task. For many tasks sprinkling in tokio::task::yield should already be pretty good but for anything over 1ms using tokio::task::spawn_blocking seems reasonable.
4 participants
Description
The task that writes the current auction to the DB has 2 performance issues:
JsonValueand letsqlxdo the conversion to stringChanges
How to test
adjusted existing e2e tests, they show that the serialized data can be read back into the exact JSON value we serialized in the first place