-
Notifications
You must be signed in to change notification settings - Fork 26
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
Use AtomicU64 instead of u64 for the position field in Receiver #70
Conversation
Hello @zeenix, Please let me know what do you think about this PR. |
Thanks for the citing a source on this. I'll read it later..
It would be nice to have some numbers on this but I guess we know for sure that at least atomics are not more expensive. So if atomics are not more expensive than their non-atomic primitive types for most machines out there, there is no downside to using atomics and that's good enough for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could you please also bump the semver in this PR (in a separate commit on top please)? That would help avoid breaking API by accident.
The `recv()`, `recv_direct()`, `recv_blocking()`, and `try_recv()` methods currently require `&mut self` to modify the value of the `position` field. By using AtomicU64 for the `position` field eliminates the need for mutability. Fixes issue smol-rs#66
After conducting benchmarks for the new changes, it turns out using atomic operations results in a performance regression of around 10-11%. This regression is likely to increase with more receivers. Here are the benchmark results on an Intel x86_64 architecture CPU by running Without atomic operations:
With atomic operations:
I am closing this PR due to this regression. |
@hozan23 Thanks so much for your hard work still. I certainly learnt things in the course of this PR and now we've a very good answer to "why not allow Receiver to be non-mut?". |
The
recv()
,recv_direct()
,recv_blocking()
, andtry_recv()
methods currently require&mut self
to modify the value of theposition
field. By using AtomicU64 for theposition
field eliminates the need for mutability.Regarding performance, it's worth noting that on most processor architectures, there is no much difference between an atomic store and a regular non-atomic store (source: 1, 2).
Additionally, Atomic operations are generally not as expensive as mutexes, as mutexes themselves are implemented using atomic operations but with additional overhead for locking.
This PR has breaking API changes.
Fixes issue #66