[hist] Pause filling during snapshot - #23093
Conversation
Test Results 23 files 23 suites 3d 15h 3m 0s ⏱️ For more details on these failures, see this check. Results for commit 863501e. ♻️ This comment has been updated with latest results. |
a14b914 to
313652f
Compare
| std::swap(fAxes, rhs.fAxes); | ||
| std::swap(fBinContents, rhs.fBinContents); |
There was a problem hiding this comment.
Can a snapshot be in progress while this is called? Maybe it's OK to assume that the answer is "no", because moving from something that's in use is likely a problem.
There was a problem hiding this comment.
Yes, I think we can assume here that no snapshot is in progress, only few methods are allowed to be called concurrently (basically FillAtomic). Exact guarantees will need to be documented in a later PR.
| do { | ||
| while (fSnapshot.load(std::memory_order_relaxed)) { | ||
| // Spin while another snapshot is running | ||
| } | ||
| } while (fSnapshot.exchange(true, std::memory_order_relaxed)); |
There was a problem hiding this comment.
| do { | |
| while (fSnapshot.load(std::memory_order_relaxed)) { | |
| // Spin while another snapshot is running | |
| } | |
| } while (fSnapshot.exchange(true, std::memory_order_relaxed)); | |
| // Spin while another snapshot is running and reserve our turn | |
| bool expected = false; | |
| while (! fSnapshot.compare_exchange_strong(expected, true, std::memory_order_relaxed)); |
There was a problem hiding this comment.
The proposed code is worse because compare-exchange evicts the cache line (holding RHistEngine) on every iteration. See also the section on optimizations in https://en.wikipedia.org/wiki/Spinlock and test-and-test-and-set lock (TTAS).
FillAtomic is much faster than SnapshotAtomic, so in case of heavy contention it would starve the snapshot. By adding a new atomic flag FillAtomic can pause while a Snapshot is running. The successful double collect is still needed if a thread is already in FillAtomic. If SnapshotAtomic is not used, the performance impact of the added check is minimal / not measurable because fSnapshotInProgress is always false and already in the cache line of the RHistEngine.
313652f to
863501e
Compare
3 participants
FillAtomicis much faster thanSnapshotAtomic, so in case of heavy contention it would starve the snapshot. By adding a new atomic flagFillAtomiccan pause while a Snapshot is running. The successful double collect is still needed if a thread is already in FillAtomic.If
SnapshotAtomicis not used, the performance impact of the added check is minimal / not measurable becausefSnapshotis always false and already in the cache line of theRHistEngine.