Skip to content

WIP: Communication refactor - #5404

Open
RudolfWeeber wants to merge 70 commits into
espressomd:pythonfrom
RudolfWeeber:comm
Open

WIP: Communication refactor#5404
RudolfWeeber wants to merge 70 commits into
espressomd:pythonfrom
RudolfWeeber:comm

Conversation

@RudolfWeeber

@RudolfWeeber RudolfWeeber commented Jul 29, 2026

Copy link
Copy Markdown
Contributor
  • more readable and flexible ghost comm setup, in prep or load banacing
  • programmatic validation
  • async point-to-point in stead of staggered communication
  • latency hiding: overlap of ghost force reduction and integration step 2 on inner cells
  • only communicate quaternions/torques when needed

RudolfWeeber and others added 30 commits July 26, 2026 19:15
@RudolfWeeber @claude
docs: design spec for async, topology-agnostic ghost communication
a817027 
Approved brainstorming output for the ghost-communication refactor:
replace the staggered axis-relay with async point-to-point messages,
add a split-phase API for latency hiding, a topology-agnostic HaloPlan
declaration with a unified validator, and interior/boundary cell tags
(enabling tag-based Euclidean-vs-MI). Particle ghost layer now; P3M
charge-assignment halo as a follow-on. No Cartesian assumptions, so a
future tree/SFC load balancer plugs into the same interface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: Ph0+Ph1 implementation plan for async ghost communication
82769fd 
Bite-sized, TDD-structured plan to instrument the existing ghost path
and lock an A/B baseline (Ph0), then stand up the async HaloPlan engine
and route all three cell systems through it at physics parity (Ph1),
deleting the staggered relay. Ph2-Ph5 scoped as follow-on plans.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: add Caliper markers to CellStructure ghost entry points
b723ea1 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
testsuite: pin ghost-communication Caliper labels
d0b4657 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber
benchmarks: ghost A/B runner + machine-idle gate
92cfe5e
@RudolfWeeber @claude
core/ghosts: add topology-agnostic HaloPlan data model
112e755 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: extract reusable particle packing (no behavior change)
da9c0d8 
Move CommBuf, SerializationSizeCalculator, ReductionPolicy,
SerializationDirection, serialize_and_reduce, both calc_transmit_size
overloads, pack_cells (was prepare_send_buffer), unpack_cells (was
put_recv_buffer), add_forces (was add_forces_from_recv_buffer),
add_rattle (was add_rattle_correction_from_recv_buffer), and
local_cell_copy (was cell_cell_transfer body) out of ghosts.cpp into
src/core/ghosts/particle_packing.{hpp,cpp} under namespace GhostComm.

ghost_communicator() now delegates to GhostComm:: functions; the
GHOST_LOCL branch calls local_cell_copy per src/dst pair.
Serialization field order and all #ifdefs are unchanged — byte layout
is identical to before.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: async halo_exchange engine (p2p + local, split-phase)
17552cc 
Split-phase, non-blocking ghost-communication engine driving a HaloPlan:
post all irecvs, pack + post all isends, run local copies, wait_all, then
unpack/reduce. Reduce swaps send/recv roles and adds on arrival; PARTNUM
posts a fixed-size recv; BONDS uses a second serialized message off the
hot path. Per-neighbor buffers/requests live on the GhostExchange handle.
Collective section stubbed for Task 1.6.

Guarded by a 2-rank unit test: Push positions, Reduce forces (sum), and a
Bonds round-trip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: enforce one-NeighborComm-per-peer precondition in halo_e…
5d3c4ed 
…xchange

The (peer, data-part tag) MPI matching in halo_exchange_start is safe only
when each peer rank appears at most once in plan.neighbors; duplicate peers
would make two messages share the same (peer, tag) and could cross-match or
deadlock. Add a @pre docblock to halo_exchange_start and a NOTE near
main_tag documenting this invariant. Under ESPRESSO_ADDITIONAL_CHECKS,
assert the invariant at runtime with an O(n) unordered_set scan so CI/test
builds catch any future plan builder that violates it while Release builds
pay nothing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: RegularDecomposition emits a direct-neighbor HaloPlan
0e08d3f 
Add GhostComm::HaloPlan make_halo_plan() to RegularDecomposition: a
topology-agnostic, direct-neighbor ghost plan where each of the up-to-26
stencil neighbors is an explicit peer (no axis relay). Cached in
m_halo_plan and exposed via halo_plan() (base-class default returns
nullptr = not migrated).

Enumeration is ghost-cell-centric so each ghost is filled exactly once
even when node-grid dims of 1 or 2 collapse several directions onto one
peer. Per peer, recv (my ghosts) and send (my boundary reals, the dual of
each ghost) are sorted by the mirrored real cell's global index so
recv[k] <-> peer.send[k] without exchanging index arrays. Self-ghosts
(node-grid dim == 1) become LocalComm (replaces GHOST_LOCL). shift is
zero, matching the legacy communicator (R5). Empty plan on a single rank.

The legacy staggered communicators are unchanged and still used by the
facade; the plan is built alongside them (Task 1.5 flips the facade).

Coverage test at NUM_PROC 4 (node grids {2,2,1} and {4,1,1}): every
ghost cell is the recv/dst of exactly one entry, union of recv/dst ==
ghost_cells(), peers unique, send.size()==recv.size(), plus a cross-rank
send/recv count consistency check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
benchmarks: fix ghost_ab idle gate falsely refusing on idle machine (…
d1d8211 
…set -e/grep no-match)

Two set -e traps in the machine-idle gate:
1. `read -r load1 _ < <(...)` — bash read(1) exits 1 when stdin closes without
   a trailing newline; fixed by using load1=$(awk '{print $1}' /proc/loadavg).
2. `grep -v "^${USER}$"` — exits 1 when no other users are logged in (the normal
   idle case); fixed by doing the filtering in awk via $1 != me so grep is never
   invoked. Also switches user detection to $(id -un) for robustness in detached
   shells where $USER may be unset.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
benchmarks: capture staggered ghost-comm baseline (LJ + P3M, 1/2/4/8 …
626180f 
…ranks)

Reference for the async-engine A/B (R7 no-regression). Per-step LJ mean:
1r=2.73e-4 2r=3.90e-4 4r=4.60e-4 8r=6.31e-4 s. Caliper (LJ@4 ranks):
ghosts_update ~12.4% + ghosts_reduce_forces ~11.6% of integration.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: route regular ghost exchange through async engine (parity)
d388d60 
Branch each ghost façade method (ghosts_count, ghosts_update,
ghosts_reduce_forces, ghosts_reduce_rattle_correction) on
decomposition().halo_plan(): when non-null use the async
GhostComm::halo_exchange engine; otherwise fall back to the legacy
ghost_communicator. This keeps Atom/Hybrid decompositions on the old
path (their halo_plan() returns nullptr) while routing RegularDecomposition
through the Task 1.3 engine.

Op mapping:
- ghosts_count         → GHOSTTRANS_PARTNUM, {Push, Overwrite}
- ghosts_update        → map_data_parts(parts), {Push, Overwrite}
- ghosts_reduce_forces → GHOSTTRANS_FORCE, {Reduce, Add}
- ghosts_reduce_rattle → GHOSTTRANS_RATTLE, {Reduce, Add}

Parity: lj/lees_edwards/p3m_fft/virtual_sites/lb_momentum pass @1/2/4.
collision_detection FAILS @2/4 ranks with "get_next_capacity, allocator's
max size reached" in boost::container::vector. Root cause isolated to
pack_regions() in HaloExchange.cpp (Task 1.3): it calls pack_cells() once
per SendRegion and concatenates the resulting bond archives — but multiple
boost binary archives cannot be read back as one, causing compact_vector
deserialization to read a garbage size from the second archive's header.
Fix is in Task 1.3, not this façade.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: pack all NeighborComm cells in one archive (fix bond-buf…
314e7c3 
…fer concat)

pack_regions() was calling pack_cells() once per SendRegion, then
concatenating the resulting bond buffers.  Each pack_cells() call opens
its own boost binary archive; concatenating N archives is invalid -- the
receiver's single unpack_cells() reads the first archive cleanly, then
hits the second archive's header bytes and misinterprets them as a
compact_vector<int> element count, producing a garbage huge integer that
triggers boost::container: get_next_capacity allocator's max size reached.

Fix: collect all region cell pointers and call pack_cells() once with the
full list so there is exactly one bond archive per neighbor message,
symmetric with the receiver's single unpack_cells() call.  An
ESPRESSO_ADDITIONAL_CHECKS assertion documents the invariant that all
shifts in a NeighborComm are equal.

Fixes collision_detection.py crash on 2+ ranks.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: document + forward-note the pack_regions shift-equality …
bfe119d 
…invariant

Enhance the pack_regions() comment to explicitly state where the invariant
is established (RegularDecomposition::make_halo_plan sets all shifts to {})
and add a forward-note documenting what would change if future decompositions
(e.g. Lees-Edwards) ever store distinct per-region shifts. Keep the existing
ESPRESSO_ADDITIONAL_CHECKS shift-equality assert unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: n-square and hybrid emit HaloPlan collective section
ed0bd33 
Implement run_collective() in HaloExchange.cpp: mirrors legacy GHOST_BCST/
GHOST_RDCE loop — for each root rank, Push does boost::mpi::broadcast of
the packed cell; Reduce does boost::mpi::reduce with std::plus<double> on
the raw double buffer.  op.direction selects which path; CollectivePattern
acts as the "active" marker.

Add AtomDecomposition::make_halo_plan() + halo_plan() override: produces a
HaloPlan with empty neighbors/local and a CollectiveSection{Broadcast,
cells[0..n-1]} where cells[root] = &cells.at(root).particles().  Cached
in m_halo_plan, built in configure_comms().

Add HybridDecomposition::make_halo_plan() + halo_plan() override: copies
neighbors/local from the regular child's plan and the collective section
from the n-square child's plan into one HaloPlan with comm = m_comm.

Add collective_broadcast_and_reduce test to HaloExchange_test.cpp (2 ranks).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: document collective PARTNUM-first invariant + plan point…
5e527d2 
…er-lifetime TODO

Document the PARTNUM-first ordering invariant in run_collective() with a
precondition comment explaining that non-PARTNUM data parts require ghost
cell sizes to already be synced. Add TODO notes on cached m_halo_plan
members noting that they hold raw ParticleList* pointers into decomposition
cells, making value-copying unsafe (Task 1.7 cleanup).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: remove legacy staggered engine; async HaloPlan is the on…
f5474fc 
…ly path

Delete ghost_communicator(), GhostCommunicator/GhostCommunication structs,
GHOST_SEND/RECV/BCST/RDCE/LOCL/JOBMASK/PREFETCH/PSTSTORE macros, prepare_comm(),
revert_comm_order(), assign_prefetches(), and m_exchange_ghosts_comm /
m_collect_ghost_force_comm members from all three decompositions. Remove
exchange_ghosts_comm()/collect_ghost_force_comm() from the ParticleDecomposition
interface and all overrides. Make halo_plan() pure virtual. Remove the legacy
else-fallback from the CellStructure ghost façade methods. Delete ghosts.cpp
(now empty) and remove it from CMakeLists.txt. Retain GHOSTTRANS_*/map_data_parts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
test: drop unused variable in HaloExchange collective test (gcc-14 -W…
9bfddfd 
…error)

CI (gcc-14, RelWithAssert, -Werror) flagged an unused 'other' local in the
collective test case; the local Release/gcc-13 build did not. Remove it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: Ph2 implementation plan (unified HaloPlan validator)
68f5ee9 
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: add local HaloPlan validator (coverage, neighborship-mat…
c174e65 
…ch, peer-uniqueness)

Adds GhostComm::validate_halo_plan() returning per-violation strings for
coverage (every ghost filled exactly once, no out-of-set targets),
neighborship-match (all local-cell ghost neighbors are covered recv/dst
targets), peer-uniqueness, and send/recv shape. Unit test covers all four
defect classes. Always compiled; production call sites will gate on
ESPRESSO_ADDITIONAL_CHECKS (Task 2.3).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
test: cover recv-outside-ghost-set and isolated shape-mismatch in Hal…
4f7858d 
…oPlan validator

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: add cross-rank HaloPlan symmetry check (all-to-all send/…
bf5c6b9 
…recv counts)

Implements validate_halo_plan_symmetry() using a collective all-to-all
exchange of per-rank send-counts, avoiding the deadlock that per-neighbor
isend/irecv would cause on asymmetric plans.  Extends HaloPlanValidator_test
with good-case (real RegularDecomposition) and asymmetric-mismatch cases,
both guarded by NUM_PROC 4.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: validate HaloPlan at build under ADDITIONAL_CHECKS; drop…
98d380d 
… ad-hoc Ph1 asserts

Wire GhostComm::validate_halo_plan / validate_halo_plan_symmetry right after
m_halo_plan = make_halo_plan() in all three decompositions (RegularDecomposition,
AtomDecomposition, HybridDecomposition), guarded by ESPRESSO_ADDITIONAL_CHECKS.

Remove the now-redundant hand-rolled coverage/peer-uniqueness block that was
inside RegularDecomposition::make_halo_plan() (the wiring above replaces it).

In HaloExchange.cpp: remove the inline seen_peers peer-uniqueness assert (now
covered at plan-build time by validate_halo_plan); leave a one-line comment
noting this. Add an op-sanity assert at the top of halo_exchange_start:
Combine::Add is only valid for GHOSTTRANS_FORCE or GHOSTTRANS_RATTLE.

Add unit-test case local_checks_pass_real_decomposition (NUM_PROC 4) in
HaloPlanValidator_test.cpp: builds a real RegularDecomposition and asserts
validate_halo_plan passes on the live plan, providing a checks-OFF guard that
the wiring won't false-trip in CI (which builds with ADDITIONAL_CHECKS on).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: validator handles collective section; guard RATTLE; vali…
25055ba 
…date Atom on populated cells

Fix 1 (compile break): nest the GHOSTTRANS_RATTLE arm of the op-sanity
assert in HaloExchange.cpp under #ifdef ESPRESSO_BOND_CONSTRAINT
(GHOSTTRANS_RATTLE is only defined there), so BOND_CONSTRAINT-off builds
with ADDITIONAL_CHECKS compile.

Fix 2 (real gap): validate_halo_plan now treats collective-covered
ghosts as covered. AtomDecomposition (collective-only) and
HybridDecomposition (neighbors + collective) fill ghosts via
plan.collective (n-square broadcast/reduce), not via neighbors/local.
Build a collective_set from plan.collective->cells (precision) and
excuse zero-fill coverage / neighborship-match only for those cells.
Checks stay strict for pure point-to-point plans and for double-fills,
out-of-ghost-set p2p targets, and per-neighbor shape mismatches.

Fix 3: move AtomDecomposition's wired validate asserts out of
configure_comms() (which runs before mark_cells()) to the end of the
constructor, after cells are populated, so Atom is genuinely validated
rather than checking empty spans. Add missing <cassert> to
AtomDecomposition.cpp and HybridDecomposition.cpp.

Fix 4: add NUM_PROC-4 unit tests that build a real AtomDecomposition and
a real HybridDecomposition and assert both validators return empty.
These call the validator directly (checks-OFF build), guarding against
the Fix-2 false-positives.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: fix op-sanity assert -Werror portability
92c5e98 
Computing the reducible-parts predicate outside assert() so the
ESPRESSO_BOND_CONSTRAINT #ifdef is not embedded within macro arguments
(non-portable; gcc-14 -Werror rejected it). [[maybe_unused]] guards NDEBUG.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: fix HaloPlan validator/make_halo_plan for 1-rank & small…
957fbb9 
…-grid configs

On a single MPI rank make_halo_plan() returns an intentionally empty plan
(init_cell_interactions wires periodic neighbourships directly between local
cells), yet mark_cells() still classifies the halo layer as ghost cells. Those
ghosts are never referenced by any local cell's neighbor stencil, so they carry
no physics and need no communication. validate_halo_plan()'s coverage check was
too strict: it flagged every such ghost as "never filled", aborting 7 unit
tests under a RelWithAssert (ADDITIONAL_CHECKS) build.

Fix the validator (the empty plan is correct): only require a ghost to be
filled when some local cell actually references it. Double-fill, out-of-ghost
targets, shape, peer-uniqueness and the neighborship-match guarantee stay
strict, so a real missing-communication defect is still caught. 4-rank plans
are unaffected (every ghost is referenced there).

Add a 1-rank regression test that builds a real RegularDecomposition and calls
the validator directly, and register HaloPlanValidator_test at NUM_PROC 1 and 4.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: Ph3 plan (interior/boundary tags + Euclidean-vs-MI)
043d3d8 
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: classify interior/boundary cells; validator checks consi…
e4377fc 
…stency

Add Cell::m_is_boundary / is_boundary() flag (default false). Implement
GhostComm::mark_boundary_cells() in ghosts/mark_boundary_cells.hpp: resets
all local cells to interior, then marks a cell boundary iff any neighbor is
in the ghost set. Wire it into RegularDecomposition, AtomDecomposition, and
HybridDecomposition right after make_halo_plan(). Add interior/boundary
consistency check to validate_halo_plan(): an interior cell must have no
ghost neighbor. TDD: Cell_boundary_test.cpp covers marking + idempotency +
validator fire/silent cases (RED confirmed, GREEN 149/149 on /ssd).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: Ph4 plan (latency hiding / 8-rank mitigation, profile-then-fix)
490b838 
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RudolfWeeber and others added 11 commits July 29, 2026 13:08
@RudolfWeeber @claude
core/ghosts: run local transfers in halo_exchange_start (overlap with…
3b48757 
… in-flight messages)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: boundary tag accounts for periodic-wrap neighborships; v…
50c874c 
…alidator overlap invariant

mark_boundary_cells gains an optional pair-predicate (wrap_predicate) that fires
when a local↔local neighbour relation crosses a periodic box boundary.  Interior
= no ghost neighbour AND no wrap-predicate hit.

RegularDecomposition supplies a precise predicate: axis i wraps iff
node_grid[i]==1 and box.periodic(i), and the two cells are in the first↔last
local layer (ghost_coord==1 ↔ ghost_coord==cell_grid[i]).  This correctly marks
periodic boundary cells on a single MPI rank where no ghost cells exist for
that axis.

AtomDecomposition and HybridDecomposition are conservative: all local cells are
marked boundary after mark_boundary_cells(); the overlap degrades to a no-op
interior pass.

Validator (validate_halo_plan) gains the overlap-safety invariant (check 6):
an interior cell must not appear as a NeighborComm send source or LocalComm src.
This is the exact precondition Task 5.3 requires.

New tests: wrap_predicate_makes_boundary, no_wrap_no_ghost_stays_interior,
validator_fires_for_interior_cell_in_send_region,
validator_fires_for_interior_cell_in_local_comm_src,
validator_silent_for_boundary_cell_in_send_region (Cell_boundary_test);
single_rank_wrap_axis_cells_are_boundary (HaloPlanValidator_test, 1-rank).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: cell_grid==1 wrap axes force all-boundary (fix false int…
24fe5fd 
…erior)

When node_grid[i]==1 && periodic[i] && cell_grid[i]==1, the single cell
layer folds back onto itself; init_cell_interactions() drops the self-pair
(ind1==ind2), so neighbors().all() is empty along that axis and the
wrap predicate in mark_boundary_cells() is never invoked — leaving the
cell INTERIOR despite self-interaction across the periodic boundary.
Fix: after mark_boundary_cells(), if any wrap axis has cell_grid[i]==1,
mark every local cell boundary.  New test (single_rank_cell_grid_one_all_
cells_are_boundary) confirms RED before fix, GREEN after.

Also: HaloPlanValidator.hpp check-#6 note on why LocalComm.dst/
NeighborComm.recv are omitted; mark_boundary_cells.hpp collapse two
break-ing ifs into one condition.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: append fix-round entry to task-5.2-report.md
5149cc3 
Documents the cell_grid==1 degenerate-wrap bug, root cause, fix, and
RED→GREEN test evidence.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core: overlap ghost force reduction with interior-cell velocity update
438dd54 
Add a split-phase ghost force reduction (Task 5.3) that overlaps the
non-blocking MPI force reduce with the velocity-Verlet / symplectic-Euler
half-kick on interior cells:

  calculate_forces() [starts async reduce] →
    integrator_step_2 on interior cells →
  ghosts_reduce_forces_finish() →
    integrator_step_2 on boundary cells

CellStructure gains ghosts_reduce_forces_start(), ghosts_reduce_forces_finish(),
and has_pending_ghost_reduce(); a mutable std::optional<GhostExchange> member
(m_pending_ghost_reduce) tracks the in-flight exchange.  Lifecycle assertions
fire in resort_particles(), set_particle_decomposition(), and the destructor.

The split is gated behind a conservative eligibility whitelist
(ghost_reduce_overlap_eligible in forces.cpp): comm.size > 1, comfixed
inactive, force_cap == 0, integrator is NVT/SE, no NPT propagation, no
LB-tracer arm.  All other configs fall back to today's blocking path
byte-for-byte.

The per-particle step_2 kernel is factored into make_step2_particle_kernel()
so full, interior-only, and boundary-only passes share the same lambda
verbatim.  The filtered passes use the new for_each_interior_particle /
for_each_boundary_particle helpers, which call parallel_for_each_particle_impl
on a filtered cell span — preserving the existing Kokkos parallelization.

Caliper: reuses the existing "ghosts_reduce_forces" region label around both
start and finish episodes via ESPRESSO_CALI_MARK_BEGIN/END.

Bitwise check (lj WCA fluid, 125 particles, 4 ranks, 200 steps): overlap-on
and overlap-off (scratch build, eligibility forced false) produce identical
sum(positions)=1873.97333048674 and sum(|forces|)=15249.8149458956.

Unit tests: 149/149 pass (RelWithAssert + -Werror).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core: exception-safe pending ghost reduce; caliper region ordering
51f629d 
I-1: in ghosts_reduce_forces_start, move CALI_MARK_BEGIN to after
emplace() so a throwing start cannot leave the region open without a
matching END.  In ghosts_reduce_forces_finish, move CALI_MARK_END to
before reset() so a throwing finish closes the region before clearing
the optional.

I-2: wrap the interior integrator_step_2_filtered pass in a minimal
RAII ReduceGuard that calls ghosts_reduce_forces_finish() on unwind if
still pending.  ESPResSo propagates errors via runtimeErrorMsg rather
than exceptions, so this guard fires only in exceptional circumstances;
the normal path calls finish() explicitly and the guard becomes a no-op.
No behaviour change on the normal path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: grow exchange buffers without zero-fill (perf: -1% memse…
d1c4c67 
…t at 10k/8)

CommBuf::buf switches from std::vector<char> to
std::vector<char, DefaultInitAllocator<char>>.  DefaultInitAllocator
overrides construct(T*) to use default-initialization (::new (p) T)
instead of value-initialization, so resize()-on-grow leaves new bytes
uninitialized rather than memset-zeroing them.

Invariant: every byte in the non-bond buffer is written by the caller
(pack_cells / MPI irecv) before it is read (unpack_cells / add_forces).
All eight call sites audited — see task-5.4-report.md.  The bond buffer
(boost.mpi-serialized, cold path) stays as plain std::vector<char>.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: task-5.4 report (DefaultInitAllocator CommBuf scheme, audit, te…
9f9cc38 
…st evidence)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: Ph5 Task 5.5 (runtime-conditional quat/torque ghost payload)
99efa70 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: send quat/torque only when orientation physics is active
5736762 
Add GHOSTTRANS_QUAT/TORQUE bits (under ESPRESSO_ROTATION) so the
position push and force reduce carry orientation payload only when the
active physics actually needs it.  For plain LJ the bits stay off,
cutting 32 B/particle from the position push and 24 B/particle from the
force reduce.

Whitelist in orientation_ghosts_needed(): rotational propagation modes,
vs_relative, dipolar solver, Gay-Berne, engine/LB.  Reader audit covers
all ghost-particle quat/torque consumers.

Bug fix: vs_relative_update_particles must now explicitly request
DATA_PART_QUAT because quat is no longer bundled with POSITION; the
call was missing it, causing ~0.2% force error at 2+ ranks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core: explicit Stoner-Wohlfarth arm in orientation ghost whitelist
50b93bf 
integrate_magnetodynamics calls get_reference_particle/get_local_particle
which can return a ghost, then reads p_ref->calc_director() on it.  Add
an explicit #ifdef ESPRESSO_THERMAL_STONER_WOHLFARTH branch to
orientation_ghosts_needed() gated on thermo_switch & THERMO_LANGEVIN
(SW requires Langevin; enforced in integrator_sanity_checks).

Also correct three doc-comment inaccuracies in the whitelist:
- dipolar solvers: dp3m_heffte unique_particles includes ghosts (CellStructure
  set_index_map lines 310-322) — covered by dipole-solver condition; fix wording
- ShapeBasedConstraint/calc_non_central_force: reads p.quat() on local
  particles only (constraints iterate local_particles) — safe without the bit
- ICC blocking reduce: may carry zero-valued TORQUE payload on the wire when
  orientation physics is concurrently active — harmless (bytes only)

Update task-5.5-report.md audit table with corrected rows.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber

RudolfWeeber commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Surprisingly, load imbalances between mpi ranks seemt ot be more of an issue with larger particel count epr core (10k vs 1k).
Probably the actual data volume matters more than the relative imbalance, which according to the law of large umbers would decrease for larger particle counts per core.

Next step is to benchmark on ant, both intra and inter-node.

RudolfWeeber and others added 17 commits July 29, 2026 17:25
@RudolfWeeber @claude
core: fix PR 5404 CI (AppleClang unused lambda capture; doxygen refs)
5d39db5 
- RegularDecomposition: precompute wrap_axis by value instead of capturing
  the node_grid reference (AppleClang -Werror,-Wunused-lambda-capture on
  non-odr-use captures).
- Replace @ref with @c for targets Doxygen cannot resolve from file-level
  or member doc blocks (halo_exchange_start/finish, make_halo_plan,
  parallel_for_each_particle_impl) — fixes the GitLab style job.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: sweep remaining unresolvable doxygen refs in HaloExchang…
7ea9cf3 
…e.hpp

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: reduce self-copy ghost forces back to owner (fix ICC 3-r…
3c62b13 
…ank)

On a periodic axis with node_grid[i]==1 the halo plan fills the wrap
ghost via a same-rank self-copy (plan.local, ex-GHOST_LOCL). The engine
ran that copy as src(real)->dst(ghost) unconditionally, which is correct
only for the POSITION push. On the FORCE reduce the roles must swap: the
self-ghost accumulated the short-range force from the periodic-wrap
neighbours during the pair loop, and that contribution has to be added
back into the owning real cell (dst->src, ReductionPolicy::UPDATE gives
+=). Because the direction never swapped, the self-wrap force was
silently dropped, biasing forces on particles that interact across a
node_grid==1 periodic boundary.

The bias is ~1e-11 per force but ICC amplifies it (charge =
force/q * del_eps * pref * area, gain ~4770), so testsuite/python/icc.py
failed at 3 ranks (box [20,20,25], cell_grid[x]==1) with induced-dipole
ratio 1.0001253740658 vs 1.0; sum_up broke the |sum_low|==|sum_up|
mirror symmetry that the legacy engine held to 1e-15. Bisect first-bad
was the Ph1 async-engine facade d388d60; the legacy
m_collect_ghost_force_comm was revert_comm_order'd so its GHOST_LOCL
copied ghost->real, which this restores.

icc.py passes at 2/3/4 ranks; force/imgbox probes stay bitwise identical
to the legacy engine at node_grid 3,1,1; 149/149 C++ unit tests pass;
parity gate lj@2+3+4, lees_edwards@4, nsquare@4, collision_detection@4,
hybrid_decomposition@4 all pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
test: pin LocalComm reduce role-swap in halo_exchange (regression for 3…
2d0fb8a 
…c62b13)

Add two single-rank tests exercising plan.local (same-rank LocalComm entries,
which were never covered by the existing 2-rank tests):
- local_push_position_shift: Push copies real->ghost as expected.
- local_reduce_force_role_swap: Reduce adds ghost force into real cell;
  the test FAILS if the src/dst arguments at the fixed call site are swapped
  back (RED: real_cell.force[1]{0} != 20, force[2]{0} != 300).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
cell_system: mark plan-exported cells boundary; diagnostic halo-plan …
e839e36 
…asserts

Add GhostComm::mark_plan_cells_boundary(HaloPlan const &, local_cells) in
mark_boundary_cells.hpp and call it in RegularDecomposition after the two
geometric marking passes.  The geometric rules (ghost-neighbor adjacency +
periodic-wrap predicate + cell_grid==1 degenerate) miss plan shapes that
export cells the topology never sees: Lees-Edwards fully-connected-boundary
whole-plane regions and ELC periodicity-change paths.  Those exported-but-
geometric-interior cells violated the overlap-safety invariant (check #6)
in ESPRESSO_ADDITIONAL_CHECKS builds, aborting dozens of python tests
(regular_decomposition with order-dependent leftover state, coulomb_interface
ELC path, and ~20 others).  The plan-membership pass adds source-2 marking so
the invariant "interior ⇒ not exported by the plan" holds for any plan shape.

Also carry the controller's diagnostic changes: GhostComm::report_violations
helper and assert(report_violations(...)) wiring in all three decomposition
constructors, plus a double-blank-line format fix in HaloPlanValidator.cpp.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: validate plan symmetry at first use, not construction
430591b 
Construction-time collective all_to_all in a decomposition ctor is
wrong for two reasons:
(a) During checkpoint loading, decompositions are transiently rebuilt
    while maximal_cutoff is rank-divergent (ranks may have different
    cell grids for a brief window before the next consistent rebuild).
    The transient plan is never used, so its asymmetry is harmless, but
    the ctor-time collective fires and aborts -- making the checkpoint
    load fatal on CI.
(b) A collective inside a ctor risks partial-abort deadlock: if one rank
    throws before entering the all_to_all, the others spin forever.

Remove validate_halo_plan_symmetry from the three decomposition ctors
(RegularDecomposition, AtomDecomposition, HybridDecomposition) and
replace them with explanatory comments.

Add a mutable bool symmetry_validated = false flag to HaloPlan; the
flag rearms naturally whenever a new HaloPlan is constructed.  In
halo_exchange_start (pool overload, ESPRESSO_ADDITIONAL_CHECKS), run
the once-per-plan symmetry check before the existing op-sanity assert,
skipped for GHOSTTRANS_NONE (early return stays first).  All ranks
enter halo_exchange_start together so the all_to_all is safe there.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: Ph5 Task CI-r3 (plan symmetry at first use, checkpoint fix)
eba737a 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
docs: fix last unresolvable doxygen ref (for_each_interior_particle)
ed1c582 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core: appease clang-tidy configs (emplace_back, guarded whitelist hel…
aa8620f 
…per, ctor-qualified calls, braced returns)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core/ghosts: null-safe buffer pointers in collective exchange (UBSan)
aae9223 
boost::mpi's pointer-based broadcast/reduce bind a reference to the buffer
even for count == 0; empty CommBufs yield data() == nullptr, which UBSan
flags as reference-binding-to-null in the n-square collective section.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber
rm specs/plans/ledger
e0a05b4
@RudolfWeeber
Merge branch 'python' of ssh://github.com/espressomd/espresso into wo…
f5d2335 
…rktree-comm
@RudolfWeeber @claude
Reduce cross-L3 traffic of per-step buffer sweeps and parallelize resort
cedc374 
On CPUs where small groups of cores share an L3 cache (e.g. AMD CCX),
the once-per-step whole-array sweeps over the duplicated ScatterView
buffers dominate the OpenMP scaling loss when threads span multiple L3
domains, while the pair loop and Verlet build are nearly unaffected.
Eliminate most of that traffic:

* Track with dirty flags whether any kernel scattered into the torque
  or virial ScatterViews since their last reset. When only the
  specialized force-only pair kernel runs (and no Gay-Berne or dipolar
  pair interaction is configured), skip the O(n_threads * N) zeroing
  and reduction of buffers that are known to be all-zero. All other
  torque sources (Langevin rotation, virtual sites back-transfer,
  dipolar solvers except dp3m, constraints) write Particle::torque()
  directly and are unaffected; dp3m marks the flag at its scatter site.

* Skip the redundant second zeroing of the force/torque buffers in
  init_forces_and_thermostat(); prepare_verlet_list_cabana() already
  zeroed them earlier in the same force calculation. ICC now zeroes
  the force buffers itself when its iteration loop exits.

* Stop freeing and reallocating the ScatterView scratch buffers on
  every Verlet list rebuild when the particle count is unchanged
  (always the case with a single MPI rank); zero them in place.

* Fix false sharing of the per-thread counters in set_index_map()
  (one cache line per thread instead of adjacent ints).

* Parallelize the fold-and-classify sweep of
  RegularDecomposition::resort() over cells with Kokkos; a serial
  replay phase then applies the recorded moves in exactly the order of
  the original single-pass sweep, so cell contents and the particle
  index diff are unchanged. Single-threaded ranks keep the original
  one-pass sweep.

LJ benchmark (N=4000, 1 MPI rank, fixed skin 0.4, EPYC 9374F): 4
threads across 3 L3 domains 0.851 -> 0.681 ms/step (-20%); 4 threads
on one CCX 0.417 -> 0.379 ms/step (-9%). LJ forces and Gay-Berne
torques are bit-identical to the previous code at 1 and 4 threads and
with 2 MPI ranks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber
Merge branch 'l3_fixes' into comm
4c7945a 
# Conflicts:
#	src/core/cell_system/CellStructure.cpp
#	src/core/cell_system/RegularDecomposition.cpp
@RudolfWeeber
Merge branch 'comm_tmp' into worktree-comm
1a56a9e
@RudolfWeeber @claude
core/ghosts: exception-safe ghost-reduce finish; plug buffer-pool leak
433a96a 
Review fixes on the split-phase reduce path:
- ghosts_reduce_forces_finish() resets the pending state before
  rethrowing, so a failed finish is never re-run on half-consumed
  MPI state (e.g. by the ReduceGuard).
- ReduceGuard destructor swallows exceptions from finish(): it only
  runs during unwind from another exception, and a second exception
  escaping the implicitly-noexcept destructor would std::terminate.
- Convenience halo_exchange_start overload allocates its buffer pool
  via make_unique before the throwing call instead of transferring a
  raw allocation afterwards (leak on exception).
- pack_regions reuses region_cells() instead of an inline copy.
- clang-format pass over set_index_map (merged unformatted).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RudolfWeeber @claude
core: share Gay-Berne activity predicate; assert torque scatter impli…
60961fc 
…es TORQUE reduce

orientation_ghosts_needed() (rotation ghost payload) and the torque
ScatterView dirty-marking in forces.cpp tested the same Gay-Berne
pair-mask expression independently; both now call the new
InteractionsNonBonded::pair_potential_active(). The dipolar arms stay
separate on purpose: the scatter decision keys on the real-space pair
kernel (torque writer this pass), the ghost decision on any dipolar
solver (ghost orientation reader).

New cross-check assert at the scatter site pins the relationship:
whenever a pair kernel scatters torques (which also land on ghost
particles), the force-reduce flags must carry GHOSTTRANS_TORQUE, i.e.
every torque-scattering kernel needs its arm in
orientation_ghosts_needed(). Turns a silent whitelist omission into a
loud failure for pair-kernel torque writers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@espresso-ci

espresso-ci commented Aug 3, 2026

Copy link
Copy Markdown

Your pull request does not meet our code documentation rules. The Doxygen documentation generated 3 unique warnings (total: 3, ignored: 0):

src/core/cell_system/CellStructure.hpp:
  line 208: unable to resolve reference to 'm_scatter_torque' for \ref command
  line 218: unable to resolve reference to 'm_torque_replicas_dirty' for \ref command
  line 878: unable to resolve reference to 'reduce' for \ref command

You can generate these warnings with make -t; make doxygen; ../maintainer/CI/dox_warnings.sh using the maxset config. This is the same command that I have executed to generate the log above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@RudolfWeeber @espresso-ci