Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
- Coverage 70.15% 69.75% -0.41%
==========================================
Files 22 23 +1
Lines 1280 1296 +16
==========================================
+ Hits 898 904 +6
- Misses 382 392 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // visible. Redundant writers store the same bytes, so concurrent reads | ||
| // always produce the correct hash. |
There was a problem hiding this comment.
Redundant writers sounds bad. I think you mentioned a variant of this that tracked whether hashing was already in-progress?
I can imagine we might get a lot of redundant writers will e.g. hashing the same list in parallel (e.g. two beacon states), or hashing a list with lots of repetition (e.g. a list that has had intra_rebase called on it).
Maybe that's an acceptable trade-off though, to be lock-free.
There was a problem hiding this comment.
I think this case should only occur if two threads hit the same tree node at the exact same time, but perhaps something like intra_rebase makes that statistically likely if they navigate the tree in the same pattern.
I think in the case where we track in-progress hashing, I actually think it doesn't help that much since the thread basically has to spin-lock waiting for the hash to become available anyway.
One spicy idea we could try is adding some randomness (like a coin-flip) so two similar trees being hashed in parallel are unlikely to follow the same hashing path. Making it non-deterministic seems pretty spooky though
There was a problem hiding this comment.
Actually, I think two similar lists (beacon states) being hashed in parallel is something we general should be avoiding. Hashing two similar states sequentially is probably much faster than in parallel, since the first rayon will load all cores anyway and the second one will primarily be loading cached values. Unless I'm misunderstanding. Does parallel hashing happen a lot in Lighthouse?
| ready: AtomicBool, | ||
| /// The cached Hash256 hash value, stored as 4 × AtomicU64 for lock-free | ||
| /// unconditional writes without data races. | ||
| value: [AtomicU64; 4], |
There was a problem hiding this comment.
Apparently AtomicU128 exists 👀
https://docs.rs/portable-atomic/1.13.1/portable_atomic/struct.AtomicU128.html
Could be worth a bench.
There was a problem hiding this comment.
Ooh interesting. I'll take a look
3 participants
Addresses #96
An alternative to #97
Implement a custom "write-once"
HashCellwhich stores aHash256and allows for lock-free reads using atomics.