[hist] Implement RHistEngine::SnapshotAtomic - #22492
Conversation
Test Results 22 files 22 suites 3d 15h 4m 41s ⏱️ Results for commit c957255. ♻️ This comment has been updated with latest results. |
e9e8303 to
fad7ce2
Compare
After the changes introduced in commit 43566dc ("Ensure consistency of RBinWithError::AtomicAdd"), it is possible to detect a. ongoing updates if fSum2 is negative, and b. inconsistent reads if the value of fSum2 changes.
This ensures correctness on weakly-ordered systems. This also requires a store release of fSum in AtomicAdd to make sure a previous write of fSum2 is visible in AtomicLoad.
It returns a consistent copy of the histogram, during concurrent filling. The implementation is based on a successful double collect as described by Afek et al in their 1993 paper "Atomic Snapshots of Shared Memory." The idea is to iterate twice over all bins until no change is observed. This is guaranteed to work for unweighted filling because bin contents are monotonically increasing. With weighted filling and potentially negative weights, there could be the situation where a bin goes back to its previous content and a change is missed. This is probably unlikely in practice, or can be entirely avoided by using RBinWithError that also tracks the sum of squares, which is monotonically increasing even for negative weights.
This ensures correctness on weakly-ordered systems. If not using SnapshotAtomic, release stores would not be needed in FillAtomic. However, tests indicate that it's not measurable in practice so we use it unconditionally to keep the implementation simple.
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
Cool stuff!
Maybe one general point on progress (not for this PR): IIUC, theoretically, the snapshot operation may never return. I wonder if this can be a problem, and if it is, if we can use the spinlocks after some waiting to block fillings and let a snapshot finish.
Yes, this is a problem especially because fill operations are much faster than snapshots.
... and yes, I already have this implemented in a branch locally 😃 |
2 participants
It returns a consistent copy of the histogram, during concurrent filling. The implementation is based on a successful double collect as described by Afek et al in their 1993 paper "Atomic Snapshots of Shared Memory." The idea is to iterate twice over all bins until no change is observed.
This is guaranteed to work for unweighted filling because bin contents are monotonically increasing. With weighted filling and potentially negative weights, there could be the situation where a bin goes back to its previous content and a change is missed. This is probably unlikely in practice, or can be entirely avoided by using
RBinWithErrorthat also tracks the sum of squares, which is monotonically increasing even for negative weights.