Use atomic instead of mutex in du function
#13253
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
This was an intentional change from Atomic64 for compatiblity reason. See #12981 and rust-lang/rust#117916 (comment).
|
My apologies, I did not realize it was originally an atomic. Would you like me to make the atomic conditional as mentioned in the related comment? |
If the shim doesn't bring too much code and is easy to understand, it could work I guess :) |
| #[cfg(target_has_atomic = "64")] | ||
| return Ok(total.load(Ordering::Relaxed)); |
There was a problem hiding this comment.
I needed to use a return here so it would compile. Rust doesn't know that the two cfg blocks are mutually exclusive.
|
Thanks for the PR! However, I'm having trouble understanding why we would make this change. It appears to be quite a lot more code from before, which adds maintenance burden and complexity. In my performance testing, I was not able to determine a noticeable difference. Are there some benchmarks that indicate that this is an issue? |
No, I just happened to be browsing the codebase and noticed that the I'm going to close this, since the additional code complexity does not deem the small performance gain. I apologize for wasting your time. |
|
Can you please make a PR adding a comment explaining why it is a |
This is a good idea. I created #13273. |
Document why `du` function uses mutex ### What does this PR try to resolve? After closing #13253, it [was suggested](#13253 (comment)) to document why the `du` function uses a `Mutex` instead of an `AtomicU64`. This will prevent others from making the same mistake I did. :) ### How should we test and review this PR? N/A ### Additional information N/A
What does this PR try to resolve?
This optimizes the disk usage (
du()) function in cargo-util to use an atomic to track the total size instead of a mutex.The
dufunction recursively checks the amount of bytes a folder. It uses a parallel iterator to read the size of multiple files at once. The variabletotalis au64that tracks this size. It was a mutex to prevent data races in the parallel iterator and make sure that the total size is accurately counted.Because
totalis a simple number and is only being incremented in parallel, it can be turned into anAtomicU64. Atomics take up less bytes than mutexes. It also uses builtin CPU instructions to instrument locking, instead of using a manual mutex. This means it is generally faster, but results can vary.Atomics require an
Orderingto specify synchronization. After reading Rust Atomics and Locks, I choseOrdering::Relaxed. The code only needs to synchronize one atomic. By the time thewalker.runreturns, all the threads will be closed and the atomic's value will be final. If someone sees an issue with usingRelaxedordering, please say so now. The general goal is avoid data races. :)How should we test and review this PR?
Data races are tricky to reproduce, which is why I would like the logic behind my change to be verified. Additionally, I noticed that
duwas being used in multiple other tests, like the global cache tracker, so I ran those as well.I couldn't find any specific test that just tested
du. I'd be willing to write one if it would be useful.Additional information
Most of my knowledge about atomics and parallelism come from Rust Atomics and Locks. If recommend reading it if you want learn more about atomics and memory ordering.