Update OverlayTiming to support BIB random mixing - #413
Conversation
|
The downstream build failure doesn't seem related to the changes - but I can't retrigger it. |
|
Hi! I tried this implementation, and if I simply set |
|
Hello! while testing the implementation, I think I might have an idea why only one background file was being used across the bunch train.
fileIndices[0]And I guess this means that the same randomly selected background file is reused for all BXs of the signal event. I tried adding a counter outside the BX loop: size_t fileCursor = 0;and changing the file selection to: const int fileIndex =
m_randomMix ? fileIndices[fileCursor++ % fileIndices.size()] : 0;With this change, the code continues through the shuffled list instead of starting again from the first file for every BX. I rebuilt and tested this with 120 IPC background files and the overlay completed successfully, the different events showed different background patterns. |
|
Thanks for the checks @andread3vita and the suggestion for a fix @ArinaPon. |
…Core into add_overlay_BIB_random_mix
| With many large background files the algorithm is dominated by reading and | ||
| decompressing them. Set `OverlayThreads` to a value greater than 1 to read and | ||
| decompress the background files of a single event on several threads: |
There was a problem hiding this comment.
Conceptually this might interfere with Gaudis internal scheduling (even if we also use tbb to do our multithreading). It's unclear to me whether the Gaudi internal tbb bits communicate with the tbb bits here.
There is precedent for doing this though as the CKF in k4ActsTracking also does some internal multithreading. This might need some policy discussion as it could imply different usage patterns for different community (e.g. run the general chain on a single thread but branch out to multi-threading in dedicated algorithms vs. running the full chain on multiple threads with Gaudi scheduling but no algorithm-internal multi-threading).
There was a problem hiding this comment.
Indeed. In the ideal world you might want to allow users to do a combination of both, if possible.
For now, especially in colliders that are computationally challenging per event, being able to use MT inside the same event is much more important than multi-threading over events, which can be done trivially in batch jobs anyway.
There was a problem hiding this comment.
Maybe @jmcarcell knows if functional algorithms can already propagate that to the Gaudi scheduler somehow. Otherwise the potential interplay will for now just be another thing to document.
| // Advance the cursor for (group, file) and return the raw entry to read. | ||
| // Cheap and I/O-free, so calling it serially (during the work-list build) | ||
| // does not limit read parallelism. | ||
| size_t reserve(int group, int file) { | ||
| std::lock_guard<std::mutex> lock(m_ioMutex); | ||
| size_t& entry = m_nextEntry[group][file]; | ||
| const size_t e = entry; | ||
| const size_t total = m_totalNumberOfEvents[group][file]; | ||
| entry = (total > 0) ? (entry + 1) % total : entry + 1; // wrap once the total is known | ||
| return e; | ||
| } |
There was a problem hiding this comment.
Do we need to lock this? The comment seems to imply it happens in a sequential piece of code(?).
There was a problem hiding this comment.
Within a single event this is only called from the sequential work-list build.
However, my understanding is that operator() is const and the scheduler may run several events concurrently that all share this EventHolder, so the lock is still needed.
(The code comment is misleading and I'll fix it)
There was a problem hiding this comment.
Ok, I think now I understand. The lock is there for "Gaudi does the multithreading"-mode.
| // the shared per-group reader is used under the mutex. | ||
| podio::Frame readAt(int group, int file, size_t rawEntry) { | ||
| if (m_randomMix) { | ||
| podio::Reader reader = podio::makeReader(m_fileNames[group][file]); |
There was a problem hiding this comment.
Intuitively this has a lot of overhead hat we are repeating for every event, because opening the file initializes a bunch of state in the reader which is quite expensive since the assumption is that this amortizes over many events and so we do a bunch of work upfront to save later in the event.
I can't say how much overhead this is in practice, but if there is a way to avoid it that might be a good idea.
There was a problem hiding this comment.
I had measured it on some synthetic files. It's real, but small.
Timing breakdown for the one-pseudo-event-per-file pattern:
| per-file size | open (makeReader+getEntries) |
readEvent |
decompress/materialise | crop+merge |
|---|---|---|---|---|
| ~40 MB (BIB-like) | 1.8% (≈5 ms/file) | 52% | 35% | 12% |
| 55 KB | 62% (≈1.2 ms/file) | 33% | 3% | 1% |
So the repeated open costs ~5 ms/file, ≈2% of the overlay time for the ~40 MB files we use.
There was a problem hiding this comment.
Thanks a lot for testing and the numbers. IIUC, the driving factor here is the size of the event / collection that you read and not the file-size per se? Does the 55 KB file have a realistic number of elements? I am trying to understand whether we should document that this random mixing as it is done at the moment might incur some (quite significant) overhead if it is used on small overlay events.
BEGINRELEASENOTES
OverlayTimingwith random background-file mixing: the newRandomMixBackgroundFilesoption treats each file in a background group as an independent event source and picks a random set of files for every overlaid event.BackgroundFileNamesentries may now be directories (their.rootfiles are used).MergeMCParticlesoption toOverlayTiming(defaulttrue); whenfalse, background MCParticles are not stored, tracker hits keep the momentum of their originating particle and calorimeter contributions get an empty particle.OverlayTimingis safe to run with intra-event multithreading.ENDRELEASENOTES
This PR updates OverlayTiming with the logic used by the muon collider software to overlay the BIB pseudo-events (from https://github.com/MuonColliderSoft/k4Reco/blob/main/k4Reco/Overlay/components/OverlayTimingRandomMix.cpp).
I opted for porting the changes over rather than asking to include a second algorithm, since the code was 95% the same.
The updated algorithm uses TBB for intra-event multithreading in processing the thousands of inputs for the overlay.