Skip to content

o2-sim: Fixes, code refactor, simplification and performance enhancement - #15843

Merged
sawenzel merged 16 commits into
AliceO2Group:devfrom
sawenzel:swenzel/o2sim-multiprocess-fixes
Sep 25, 2026
Merged

o2-sim: Fixes, code refactor, simplification and performance enhancement#15843
sawenzel merged 16 commits into
AliceO2Group:devfrom
sawenzel:swenzel/o2sim-multiprocess-fixes

Conversation

@sawenzel

@sawenzel sawenzel commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR does

  • fix bugs in the parallel mode (see below)
  • code cleanup: move things from headers into source
  • performance and scalability improvements for the hit-merger to reduce the tail
  • improvements for the shared memory transport
  • reduce copying memory buffers

Bug fixes

  • Workers called the detectors' FinishEvent() after sending the hits, while the serial o2-sim calls it before writing them. TRD sorts its hits there, and PHOS
    and CPV also sum duplicates. With TMessage transport these hits were written unsorted, and PHOS/CPV duplicates were not summed. With shared memory the merger
    read the buffers while the worker was rewriting them.
  • --noemptyevents wrote empty events and dropped the following real event.
  • Per-event buffers in the hit merger were never freed.
  • The merger's output pointers were uninitialised with --noDiscOutput.
  • The external-kinematics generator was cached despite the intention not to, and the merger exit-status check was always true.
  • A malformed info request to the primary server got two replies.

Performance

The hit merger now merges and writes the kinematics and each detector's hit file concurrently. It moves hits instead of copying them, and takes ownership of decoded hit containers instead of copying them. E.g., For 2 PbPb events on 32 workers, the merge-and-flush tail after the last worker finishes drops from 12.6 s to 5.6 s

Shared memory

  • The busy flag is now atomic.
  • The SysV segment is freed when its last user detaches, also after a crash.
  • The transport mode is sent with each detector's hits instead of being read from a global flag at decode time.

Validation

The output was compared with daily-20260924 by a content hash of every branch, for pp and PbPb and with both transports. All hit trees and the kinematics are
identical to the daily's shared-memory output, and the shared-memory and TMessage transports now produce the same results.

This is the result of a comprehensive agentic review of o2-sim.

sawenzel and others added 16 commits September 24, 2026 20:52
@sawenzel @claude
Fix the merger exit-status check in o2-sim
f595dcc 
This fixes a problem in how the o2-sim driver judges the hit merger's exit code.

- The condition `!= 0 || != 128` is always true, so every normal merger exit was reported as an error.
- It now reads `!= 0 && != 128`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Do not cache external-kinematics generators in the primary server
7939bca 
This fixes the generator cache of the o2-sim primary server so that it skips external kinematics, as its comment intends.

- The condition `!= "extkin" || != "extkinO2"` is always true, so extkin generators were cached too.
- A service-mode reconfiguration with a new kinematics file then reused the old generator and file.
- The condition now uses `&&`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Skip malformed info requests in the primary server
0d35160 
This fixes the info thread of the o2-sim primary server for requests of unexpected size.

- After the error reply the thread still read the request and could send a second reply on the REP socket.
- It now continues to the next request after the error reply.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Fix the event-flush loop of the o2-sim hit merger
c372745 
This fixes how the hit merger advances to the next event while flushing.

- The skip paths advanced the event counter but then carried on with the current event.
- With --noemptyevents, an event without hits was written when the next event was already complete, and that next event was then skipped.
- An event without buffered info dereferenced the end iterator.
- The loop now iterates over flushable event IDs and every skip path is a plain continue.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Release per-event buffers in the o2-sim hit merger
72dab53 
This fixes a memory leak in the hit merger and makes its merge flag thread-safe.

- cleanEvent was empty, so the decoded SubEventInfo objects of every event were never freed.
- The MC tracks and track references of events dropped by --noemptyevents were never freed either.
- cleanEvent now owns the release of all three buffers; the merge functions no longer delete.
- Buffer entries are emptied rather than erased, since erasing from a tbb::concurrent_unordered_map is not safe while the receiving thread inserts.
- handleSimData keeps the event id and event count as values, since a merge thread may free the event info.
- mergingInProgress is shared by two threads and is now std::atomic<bool>.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Initialise the output pointers of the o2-sim hit merger
745cb1a 
This fixes the hit merger when it runs without disc output.

- The kinematics and MC-header file and tree pointers were never initialised.
- With --noDiscOutput they stayed garbage, and the merge tested them as if they were valid trees.
- They are now initialised to nullptr.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Finish detector events before sending hits in parallel o2-sim
43df410 
This fixes the order in which a parallel o2-sim worker finalizes and sends the hits of an event.

- Detectors ran FinishEvent after SendData, while serial o2-sim (FairMCApplication) runs it before filling the output.
- TRD sorts its hits in FinishEvent, and PHOS and CPV sort them and sum duplicates.
- With TMessage transport their hits were written unsorted and, for PHOS and CPV, with duplicates.
- With shared memory the merger read the buffers while the worker rewrote them, so TRD output differed between the two transports.
- FinishEvent now runs for all detectors before SendData, and EndOfEvent after it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Make the shared-memory hit-buffer busy flag atomic
f085736 
This makes the flag that guards a hit buffer between a sim worker and the hit merger safe across processes.

- The flag was a plain bool in shared memory, written by the merger and polled by the worker.
- Without atomic semantics its stores may become visible out of order with the buffer reads, notably on aarch64.
- It is now a std::atomic<bool> (ShmBusyFlag) placed in the segment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Remove the o2-sim shared-memory segment when its last user detaches
e4b6438 
This stops a killed o2-sim from leaking its SysV shared-memory segment.

- The segment (1 GB per worker) was only removed in the driver's normal cleanup.
- It is now marked IPC_RMID right after creation; Linux still lets the workers and the merger attach by id.
- The kernel frees it once the last process detaches, also after a crash.
- The segment is now created with mode 0600 instead of 0666.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Send the hit transport mode with the hits in o2-sim
35c4f5c 
This makes the hit merger decode each detector's hits the way the worker sent them.

- Worker and merger each read the global ShmManager::isOperational() to choose between shared memory and TMessage.
- That flag lives in the segment and flips when any late worker fails to attach, so hits already in flight could be decoded the wrong way.
- The per-detector header message is now a HitsHeader carrying the DetID and the mode, decided once per detector in attachHits.
- collectHits takes the mode from the header.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Adopt decoded hit containers in the o2-sim hit merger
f019420 
This removes a full copy of every hit container that reaches the hit merger as a TMessage.

- collectHits allocated a new container, copied the decoded one into it and deleted the original.
- It now takes ownership of the decoded container.
- Hits in shared memory belong to the worker and are still copied.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Move instead of copy when merging hits in the o2-sim hit merger
3c27650 
This removes a deep copy of every hit when the hit merger joins the sub-events of an event.

- mergeAndAdjustHits copied each hit into the merged container; it now reserves the total size and moves them.
- The track and track-reference merges now reserve their output.
- TPC HitGroup declared a defaulted destructor, which suppresses its implicit move; the line is removed so HitGroup moves its five vectors.
- The printf debug output in the merge loops is dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Flush the output files of the o2-sim hit merger concurrently
64771ff 
This runs the per-event merge and flush of the kinematics and of each detector in parallel.

- The hit merger merged and filled the kinematics and all detector trees one after another.
- In PbPb this serial step took 12.9 s of a 66 s run, after all workers had finished.
- Each of these writes to its own TFile, so they now run as tasks of a tbb::task_group.
- The final TFile::Write calls run in parallel as well.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Move the o2-sim device implementations into source files
9a567e6 
This moves the implementation of the o2-sim primary server, worker and hit merger devices out of their headers.

- The three device headers defined all member functions and several non-inline free functions.
- Each header now declares its class; the definitions are in O2PrimaryServerDevice.cxx, O2SimDevice.cxx and O2HitMerger.cxx, compiled into their runners.
- querySimConfig moves from O2SimDevice to PrimaryServerState.cxx, so the hit merger no longer includes the worker and macro/o2sim.C.
- The helpers in SimPublishChannelHelper.h are now inline, and PrimStateToString is inline constexpr.
- The unused nested TMessageWrapper of the hit merger and CustomCleanup of the worker are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Create the hit-merger detector instances from a table
038d046 
This replaces the chain of per-detector conditions in the hit merger with a table of factories.

- Each detector that can be merged now has one entry mapping its DetID to a factory.
- The warning compared the number of active detectors with DetID::nDetectors and fired in practically every run.
- It now names an active readout detector that has no merger instance.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel @claude
Keep the hit-merger hit buffers as a detector member
ee5503a 
This replaces the static per-type map of hit buffers in DetImpl with a member of each detector instance.

- collectHits kept the buffers in a function-local static map keyed by 'this' and passed them on through a char pointer.
- They now live in a type-erased std::shared_ptr<void> member, reached through hitCollector().
- The instance keeps its own buffers, which also covers several external detectors sharing one type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sawenzel
sawenzel merged commit 900b5e4 into AliceO2Group:dev Sep 25, 2026
9 of 11 checks passed
@sawenzel
sawenzel deleted the swenzel/o2sim-multiprocess-fixes branch September 25, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shahor02 shahor02 Awaiting requested review from shahor02 shahor02 is a code owner
@davidrohr davidrohr Awaiting requested review from davidrohr davidrohr is a code owner
@wiechula wiechula Awaiting requested review from wiechula wiechula is a code owner

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sawenzel