-
Notifications
You must be signed in to change notification settings - Fork 199
Replace IngredientCache lock with atomic primitive
#687
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
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
| assert!(v < (u32::MAX as usize)); | ||
| assert!(v <= u32::MAX as usize); |
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.
I believe both of these checks (here and for MemoIngredientIndex) were accidentally wrong
CodSpeed Performance ReportMerging #687 will not alter performanceComparing Summary
|
4316b76 to
a93ba43
Compare
a93ba43 to
d894400
Compare
src/zalsa.rs
Outdated
| _ = self.cached_data.compare_exchange( | ||
| Self::UNINITIALIZED, | ||
| packed, | ||
| Ordering::AcqRel, |
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.
Release is sufficient here, there's no need for Acquire if the current thread wins the race (though of course both are correct).
d894400 to
971e52b
Compare
src/zalsa.rs
Outdated
| // This is a packed representation of `Option<(Nonce<StorageNonce>, IngredientIndex)>` | ||
| // allowing us to avoid a lock in favor of an atomic load. This works thanks to `Nonce` | ||
| // having a niche and so the entire type can fit into a u64. |
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.
| // This is a packed representation of `Option<(Nonce<StorageNonce>, IngredientIndex)>` | |
| // allowing us to avoid a lock in favor of an atomic load. This works thanks to `Nonce` | |
| // having a niche and so the entire type can fit into a u64. | |
| // A packed representation of `Option<(Nonce<StorageNonce>, IngredientIndex)>`. | |
| // | |
| // This allows us to replace a lock in favor of an atomic load. This works thanks to `Nonce` | |
| // having a niche, which means so the entire type can fit into an `AtomicU64`. |
davidbarsky
left a comment
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.
i think this looks good to me? the only note that i'd have is "it'd be nice to have a Loom/shuttle test for IngredientCache, but I think I committed to that...
005375e to
3396a1a
Compare
|
Ye I'd push the test onto the rest of the Loom work. I don't think without Loom writing a test is too useful here right now Though we can also delay this until we have loom integrated, no preference from me. |
3396a1a to
f2b1517
Compare
|
I think we can/should merge this, but codespeed is reporting 1–2% regressions. do you happen to why they're showing up? |
|
I don't know why, I'd expect this to not regress it. Maybe its just within noise |
|
1-2% is something I consider noise. |
f2b1517 to
6181ddf
Compare
No description provided.