[hist] Pause filling during snapshot - #23093
Conversation
Test Results 23 files 23 suites 3d 4h 53m 53s ⏱️ For more details on these failures, see this check. Results for commit a14b914. |
|
|
||
| std::atomic_flag snapshotter; | ||
| StressInParallel(NThreads, [&] { | ||
| if (!snapshotter.test_and_set()) { |
There was a problem hiding this comment.
If I understand correctly, this means that the first thread to run will start N consecutive snapshots and lead all the other thread to all do Fills.
Should we also test when a second thread is attempting to do a snapshot (to exercise the while loop on line 873)?
The time taken by the snapshot-ing thread between the end of a snapshot and the start of another one is smallish (2 GetBinContent), is there a risk that with this PR the execution now mostly does all the snapshots before (most of) the Fills? (i.e. not quite but almost executing the whole snapshot-ing threads before the other)
There was a problem hiding this comment.
Should we also test when a second thread is attempting to do a snapshot (to exercise the while loop on line 873)?
That's exactly what the "new" test StressSnapshotAtomic does, calling SnapshotAtomic from 4 threads, no? The renamed StressFillSnapshotAtomic then tests the interaction between FillAtomic and SnapshotAtomic, but here we can never have two SnapshotAtomic interacting.
is there a risk that with this PR the execution now mostly does all the snapshots before (most of) the Fills
I don't think so, it also has to destruct the snapshot histogram.
There was a problem hiding this comment.
That's exactly what the "new" test StressSnapshotAtomic does,
Indeed.
The renamed StressFillSnapshotAtomic ... but here we can never have two SnapshotAtomic interacting.
It 'could' (straightforwardly) be extended to have 2 Snapshot threads hence testing the 3 ways interactions.
I don't think so, it also has to destruct the snapshot histogram.
Fair enough.
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 fSnapshot is always false and already in the cache line of the RHistEngine.
a14b914 to
313652f
Compare
| { | ||
| ROOT::TestSupport::CheckDiagsRAII diagRAII; | ||
| diagRAII.optionalDiag(kWarning, "TKey::TKey", "no public constructor", /*matchFullMessage=*/false); | ||
| diagRAII.optionalDiag(kWarning, "TStreamerInfo::Build", "data member \"fSnapshot\" will not be saved", |
There was a problem hiding this comment.
In which configuration did that appears? (Since fSnapshot is marked transient, we should not be issuing this message)
2 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.