WIP: particel storage using Kokkos views#5393
Open
RudolfWeeber wants to merge 148 commits into
Open
Conversation
Approved brainstorming outcome: replace the Particle struct (AoS) with a cell-sorted flat ParticleStore of component-major Kokkos View columns (per-field dual residency, ScatterView force/torque accumulation), with state, parameters, and observables in separate containers. Migration is incremental: fields are evicted from the struct group-by-group behind proxy accessors, with always-green tests and a 5% cumulative budget on LJ/P3M benchmarks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- run_benchmarks: catch CalledProcessError from subprocess.run, print a clear one-line error naming the failed configuration, and return exit code 3 instead of letting the exception propagate (which would have aliased the compare regression exit code 1). - compare: append label= component to the printed configuration name when the label field is non-empty, so two groups differing only by label print distinct names. - module docstring: add exit-code table documenting codes 0/1/2/3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The load-average check self-tripped: the gate's own 4-rank MPI configurations drive the 1-minute load average to ~4, which takes minutes to decay, so 'run' aborted after its first multi-rank configuration. Replace the /proc/loadavg reading with a two-sample measurement of 'foreign' CPU usage: utime+stime ticks from /proc/<pid>/stat summed over processes whose owner uid differs from os.getuid(), expressed as a percentage of one core. This ignores our own benchmark load and only reacts to other users on the shared machine. CLI flag --max-load becomes --max-foreign-cpu (default 50.0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…torage Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cleStorage Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- check_cell_storage_mutations.sh: rewrite as an embedded python3 scanner that reads each src/core file as one string and matches the mutation pattern across newlines, catching clang-format-wrapped calls that the old line-based grep missed. Document the alias limitation (mutation via a local reference is invisible; a green run is a tripwire, not a proof) and the decomposition swap/teardown exception. - ParticleListOperations.hpp: name the decomposition swap/teardown exception in the namespace doc (old cell storage is destroyed wholesale by destructors; phase 2 rebuilds the full store via mark-dirty). - benchmark_gate.py: 'run' refuses (exit 4) if the output CSV already exists, so stale rows cannot pollute the min-of-means comparison; clarify the exit-code table (exit 1 also covers missing configurations). - ParticleStore.hpp: spell accessor return types as std::size_t. - CellStructure.cpp: add the direct ParticleListOperations.hpp include. - ParticleListOperations_test.cpp: add extract-from-single-element and resize-ghost-storage-shrink test cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace reference bindings (auto &force = p.force()) with local copy + write-back, eliminate all force_and_torque() uses outside ghosts.cpp, and make binary-arithmetic uses of force()/torque() proxy-safe via explicit Utils::Vector3d conversions. Zero behavior change: today's accessors still return Utils::Vector3d&. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bug 1: serialize_and_reduce FORCE branch gated on both policy==UPDATE and
direction==LOAD, so a plain MOVE+LOAD (e.g. lb_tracers' force ghost update)
fell into the else branch and serialized FROM the particle on an input
archive, discarding the received force. Fix by branching on direction first,
then on policy for the accumulate vs. assign distinction.
Bug 2: calc_transmit_size early-returned for FORCE with an assert that
data_parts==GHOSTTRANS_FORCE (false for mixed updates); in Release the assert
is a no-op and all non-FORCE parts were silently dropped from the size.
Replace with compositional sizing: compute force_size separately, mask FORCE
out of data_parts, run the Particle{}-based sizer only for remaining parts,
and return the sum.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add get_particle_force() and get_particle_torque_lab() in particle_node, each using a collective all_reduce over the live particle data on the owning rank. Route the Python-facing 'f' and 'torque_lab' getters in ParticleHandle through these new fetch functions instead of the fetch-cache detached Particle copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ParticleForce member leaves the Particle struct in the same commit that makes the columns authoritative (spec section 4, single ownership). Non-const accessors return a write-through VectorReference; const accessors return values. Python getters fetch from the owning rank. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Phase-2 T7 moved force and torque out of the Particle struct into ParticleStore columns. Those Kokkos columns are rank-local and are not carried by Particle::serialize, which is used for whole-particle migration across MPI ranks during a global resort (AtomDecomposition and RegularDecomposition). A particle that migrated therefore arrived detached from the store, and the store rebuild zero-filled its new row, losing the force. This was invisible until a decomposition switch (global resort that moves particles) was followed by a force-reusing integrator step (run(0), which does not recompute forces): the reused force read 0. This surfaced as hybrid_decomposition::test_against_nsquare failing deterministically at 4 ranks (the n_square reference forces were wrong, not the hybrid ones); serial and regular-decomposition identity were unaffected because neither migrates a particle and then reuses forces. Fix: ferry force/torque with the migrating particle through a small transitional carrier on Particle (m_detached_force/m_detached_torque), serialized alongside the other fields; ParticleStore::assign_row seeds a detached (migrated or new) particle's row from the carrier. The column remains the source of truth once attached; brand-new particles keep the zero default. This restores the exact pre-migration behavior. The carrier is transitional and is removed in phase 7 when inter-rank exchange moves to per-field column packing. Adds a ParticleStore unit-test regression guard that serialize-roundtrips a particle and asserts assign_row seeds the migrated force/torque. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final-review fixes for phase 2 of the ParticleStore migration: - VectorReference: add value-copying copy-assignment (write-through, like std::vector<bool>::reference); the copy constructor still rebinds. Add unit tests for value-copy and chained assignment. - particle_management test helper: reattach() re-points the moved particle at the store via attach_to_store instead of assign_row, so a real move no longer re-seeds from the stale migration carrier. - ShapeBasedConstraint: co-own the Kokkos runtime (shared_ptr<KokkosHandle>) captured in the lazy attach path, mirroring CellStructure, so the constraint's ParticleStore Views can be released before Kokkos::finalize even if it outlives the last CellStructure. - ParticleStore::assign_row: assert the row index is within bounds; record the adjudicated deviation in the design spec (id-based cross-check deferred to phase 5 when the id column exists). - Comment fixes: ParticleStore rebuild seeds new rows from the migration carrier (not zero-init); Particle_test clarifies force/torque ARE serialized via carriers but not applied to an already-attached particle. - Remove dead force_range() from ParticlePropertyIterator (no consumers). - trajectory_identity benchmark: clarify that P3M's required accuracy argument is unused with tune=False (kept: it is a required SI key; removing it breaks construction and the identity hashes are unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The H5MD "write" method reads local particle forces directly, but forces now live in the ParticleStore columns (migration phase 2) which are rebuilt lazily. Every other force-reading script-interface entry point already calls ensure_particle_store_synchronized(); the H5MD writer did not, so a write issued while the store is dirty (before the first integrator step, or right after a resort/particle addition with no intervening force read) could read stale/detached rows. Add the sync as the first step of the write branch. Also link the espresso_hdf5 target against Kokkos/Cabana: since force/torque moved into the ParticleStore, h5md_core.cpp transitively includes Kokkos_Core.hpp and no longer builds without those include paths. Add a Python regression test (testsuite/python/h5md.py) that marks the store dirty via a resort after adding a particle and then writes without any integrator step, checking the write does not crash and the fresh particle's force is zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ionReference (phase 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add explicitly defaulted copy constructors to BasicVectorReference<T> and QuaternionReference next to their write-through copy assignments. The comment clarifies the asymmetry: the copy constructor rebinds the proxy (default pointer/stride copy), while copy assignment writes values through to the underlying storage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ge test The cuda12-maxset and cuda12-coverage jobs build with g++-12, which rejects `scratch += i` on a volatile-qualified operand under -Werror (compound assignment to a volatile is deprecated in C++20, -Werror=volatile). The volatile scratch loop only exists to defeat dead-code elimination so the "unrelated work" between binding and reusing a RowParticleRange reference is actually emitted. Rewrite it as an explicit `scratch = scratch + i`, which keeps the volatile read+write per iteration without the deprecated compound form. Test still passes (6/6). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
UBSan (clang-sanitizer job) reports a nonnull-argument violation at MigrationPack.cpp WriteCursor::put_bytes -> std::memcpy(dst, src, 0): a particle with no bonds (or no exclusions) has an empty ragged-run sidecar whose data() is nullptr, and append_row_bonds/exclusions call put_bytes(run.data(), 0). memcpy declares both pointer operands nonnull, so memcpy(dst, nullptr, 0) is undefined behavior even though it copies nothing. read_row_* has the symmetric issue via get_bytes. Skip the memcpy when bytes == 0 in both cursors. Behaviorally identical (m_at += 0 is a no-op, so cursor positions and the packed byte stream are unchanged -> migration identity stays bitwise); it only removes the UB. Fixes 6 UBSan unit-test failures (MigrationPack_test, Verlet_list_test, EspressoSystemStandAlone_test x4 rank/thread variants), all of which migrate particles that carry empty bond/exclusion runs. Verified: the 8 affected ctest cases pass clean under UBSan after the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Zero-initialize the ghost PROPRTS archive locals (the analyzer cannot see through the archive operator& and flags the assignments as garbage), fold the RATTLE/BONDS accounting into a mutation-free assert (the masks were dead stores in release builds), and compile the GlueToSurface generation re-stamp only in debug builds (its only reader is the debug generation guard). Behavior unchanged; lj/p3m identity bitwise-identical. The clang-19 + ESPRESSO_BUILD_WITH_CLANG_TIDY configuration is what the ICP GitLab empty and clang-sanitizer jobs run and the previous local gates missed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three fixes found with perf on the lj/p3m benchmarks (steady-state sampling, instruction-level annotation): - [[gnu::flatten]] on init_forces_and_thermostat: the per-particle lambda exceeded gcc's inlining budget, turning Utils::hadamard_product and Vector constructors into per-particle PLT calls inside the Langevin friction path (15% of core time on lj at 1000 particles). - O(1) ParticleStore::has_pending_removals() fast path: iterators and live-counting consulted the pending-removal byte mask per element; the mask can only be set between a removal and the next rebuild, so the steady state now pays one predictable branch instead of a memory load per element. - Verlet-build kernels read id/position through raw column pointers hoisted once per work item instead of per-candidate Particle accessor derefs (the two hottest loads of the link-cell pass); the Particle views stay attached for the verlet criterion; iteration order and arithmetic unchanged. lj/p3m trajectories stay bitwise-identical to the phase-1 reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-evaluates the phase-3.5 layout decision on the finished migration with hardware profiling, per-config order-balanced A/B on identical sources (a compile-time toggle selects the layout; trajectories are bitwise-identical either way): - component-major wins p3m 4-rank (-1.7%), p3m 1-rank (-1.3%), lj 1-rank and 4-rank (-0.5% each) - lj-omp recovered from +1.0% to +0.5% (noise) by gathering positions into an interleaved scratch once per Verlet build: under component-major the build's per-candidate position reads touched three far-apart component streams, roughly doubling its cost at 4000 particles per core (perf-measured) - p3m-omp is inconclusive under P3M auto-tuning variance The phase-3.5 preference for particle-major came from gather-dominated paths (struct-era pair gathers, whole-particle ghost packing) that no longer exist; per-component ghost legs now benefit from contiguity, and component-major is the coalescing-friendly layout for device-resident GPU work. Particle-major stays selectable with -DESPRESSO_PARTICLE_STORE_PARTICLE_MAJOR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The user-provided raw log of GitLab job 423747 (clang-sanitizer) showed the job failing on clang-tidy errors in migration headers that the local tidy gate had missed: incremental clang-tidy builds only lint TUs that recompile, so header findings in up-to-date TUs go unreported -- standalone clang-tidy runs over representative TUs are the reliable gate form. - eight modernize-return-braced-init-list sites (Cell::particles/rows, CellRows iterators, the ParticleStore proxy factories) - bugprone-unhandled-self-assignment in the write-through proxy copy-assignments (BasicVectorReference, QuaternionReference): self-assignment is benign for a value-writing proxy, the explicit guard documents it Also supersedes the stale phase-3.5 layout comment left above the phase-8 decision block. lj/p3m identity bitwise-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The raw logs of GitLab jobs 423797 (empty) and 423791 (clang-sanitizer) show two remaining groups the previous sweep missed because it only covered production TUs: - three modernize-return-braced-init-list sites in the VectorReference unit test fixtures (the CI jobs lint test TUs too) - the GlueToSurface generation re-stamp was a dead store in the sanitizer config (built without NDEBUG, so the earlier NDEBUG guard did not apply; the analyzer is right that nothing reads the final stamp). The re-stamp is deleted: the re-resolved views are used immediately with no topology change in between, so only the const per-iteration stamp guarding the views held ACROSS the add is needed. lj/p3m identity bitwise-identical; standalone clang-tidy is clean over the production and unit-test TUs of the migration surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…inal-state terms Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tate terms Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ate terms Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… warning Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…el (simd ws1) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…air kernel (simd ws1)" This reverts commit da29efd.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 0ec0035.
Restructure init_forces_and_thermostat into contiguous component sweeps under the component-major ParticleStore columns: - Force/torque init is now three (x,y,z) contiguous column copies (force = ext_force, torque = ext_torque) over [0, n_local), which auto-vectorize into packed AVX2 ymm moves, replacing the per-row Particle-proxy writes. External-force-disabled builds zero-fill the columns. The engine swim term (needs the quaternion-derived director) stays a per-row scalar update applied only when a swimmer exists. - The Langevin friction+noise pass is split off into a force-inlined per-row helper (apply_langevin_row). The id-keyed Philox draw stays scalar by design; per-particle arithmetic order is preserved (force = ext_force; force += (friction + noise)), so trajectories are bitwise-identical. always_inline (not [[gnu::flatten]]) is used because flattening this path fuses pref*v into the accumulate under -march=native and would break bitwise identity. Same-flags identity (native): lj/p3m/langevin all unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…simd ws3)" This reverts commit cccccc0.
This reverts commit f991360.
The z-line gather (simd ws2) wins single-threaded (-7.5% on the p3m benchmark at 1 rank, -3.8% at 4 MPI ranks, 100k particles) but measures slower when the assignment loop is OpenMP-threaded (+4.4% at 4 threads, 160k particles); multi-threaded execution keeps the per-point interpolate form. Same dispatch idiom as kokkos_parallel_range_for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lation units Move init_forces_and_thermostat (with its [[gnu::flatten]] Langevin tree) and the external_force helper out of forces.cpp into forces_init.cpp, and introduce a non-template update_verlet_state wrapper in short_range_verlet.cpp that is the sole instantiation site of update_cabana_state<VerletCriterion<>>. forces.cpp, energy.cpp and pressure.cpp now call the wrapper, so the AoSoA-commit / Verlet-list-build giant is deduped 3->1 and forces.cpp's per-TU inline-growth budget is no longer shared with the init tree. Pure code motion: bitwise identity holds on both canonical and native builds (lj + p3m). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wave 1 of the short-range hot-path optimization: - Branchless cuboid minimum-image fold. Encode periodicity into a masked inverse box length (0 for non-periodic directions) so the per-component fold reduces to `dx - rint(dx * inv_masked) * L` with no branch. `rint` maps to a single rounding instruction. Results match the previous round-based fold across the pair-loop input domain (separations below 1.5 box lengths); confirmed bitwise-identical on the canonical lj and p3m trajectories. - Flat per-type-pair squared-cutoff table in VerletCriterion. The per-candidate cutoff query in the Verlet-list build becomes a dense table load instead of walking the InteractionsNonBonded pointer table; inactive pairs store a negative sentinel so the distance comparison rejects them without a separate activity check. - Hoist cuboid box parameters by value into the Verlet-build kernels via CuboidMinimumImage, instead of chasing the BoxGeometry reference for the box lengths on every candidate pair. - Force-inline Utils::Vector operator+= / operator-= (previously outlined as .isra clones called from inside the pair kernel). Canonical identity bitwise-preserved (lj, p3m); unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The `#pragma omp simd` on the fixed-trip charge-scatter and force-gather z-lines makes clang emit `-Wpass-failed=transform-warning` when it declines to vectorize the loop at the sanitizer build's `-O1`, which `-Werror` promotes to a hard failure. Restrict the pragma to non-clang compilers. The scatter loop has independent stores, so vectorization never affects its result. For the gather reduction, the pragma stays enabled on gcc (where it authorizes the z-line tree accumulation that fixes the canonical single-threaded gather); on clang the loop keeps the sequential order it already used, since the pragma was failing to apply there anyway. Results differ only at the rounding level, as the gather already does across compilers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wave 2 of the short-range hot-path optimization. Add SpecializedForcesKernel<HasCoulomb>, which owns the Verlet-list loop and runs once per particle instead of being invoked per pair by Cabana::neighbor_parallel_for. This lets it obtain the ScatterView accessor once per particle rather than once per pair -- the per-pair access() is an omp_get_thread_num call that the profile showed dominating the LJ pair cost -- and hoist the per-particle position, type and charge and the cuboid box parameters out of the neighbor loop. `if constexpr (HasCoulomb)` compiles the real-space electrostatics path in or out. forces.cpp installs it through a new optional ShortRangeVerletPairLoop hook on cabana_short_range, dispatched by create_specialized_verlet_pair_loop, which selects it only for a cuboid box with no NPT virial, dipolar or ELC kernel, DPD thermostat, Thole or Gay-Berne pair, or particle exclusion, and falls back to ForcesKernel otherwise. Two instantiations cover the hot cases: pure central radial forces and central-plus-real-space-electrostatics. The kernel keeps the generic per-pair scatter order (i before j, both scaled from the same pair force; no register accumulation), so results are bitwise-identical on a single thread. Canonical identity preserved (lj, p3m); unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Doxygen cannot resolve a \ref to the namespaced free function template detail::get_mi_coord_masked; render it as a code span instead. Comment-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a highly experimental agent run push to switch the particle storage to Kokkos views entirely, eliminating the Particle struct in the core.
Python interface stays as it is.
We use Fable for planning/design (while we have it) and Opus 4.8 for the actual coding tassks.