Particle Dual Memory Spaces - #2136
Conversation
c966067 to
bb42111
Compare
|
Ok, fixed some bad errors, these two commits should be clean now (of course I'll find ways they aren't going forward, I'm sure) |
80445ae to
003e70b
Compare
|
ack, I have misunderstood how much raw manipulation of the |
2d68d6a to
4835c41
Compare
|
I haven't tried the latest commit yet, but I think it might be the best way to go, all things considered.Will need to think, write up, and discuss (and actually test and debug to make sure it works) |
5520ee7 to
2818c32
Compare
|
Ok, got the pieces of the design I want in place. Detailed rewrite of the PR description and some questions coming tomorrow. |
|
@mayrmt perhaps also of interest to you for review? Not sure who all would be most interested |
There was a problem hiding this comment.
🟡 Not ready to approve
The new “host” storage is currently a default-memory-space Kokkos view (can become device memory on GPU builds), which would break host-side pointer access and MPI assumptions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR introduces dual host/device memory handling for particle state storage to enable incremental GPU porting, primarily by upgrading ParticleContainer to manage host/device synchronization and by splitting read-only vs writable data access.
Changes:
- Added
ParticleSpace(Host/Device) to distinguish memory spaces when requesting particle state pointers. - Reworked
ParticleContainerstate storage to use Kokkos views/dual views with lazy device initialization and explicit sync/modify behavior. - Updated particle algorithms/interactions to use the new writable state accessors where mutation occurs.
File summaries
| File | Description |
|---|---|
| src/particle/tests/4C_particle_container_test.cpp | Updates tests to use new pointer access APIs (needs adjustment to keep read-only API coverage). |
| src/particle/src/rigidbody/4C_particle_rigidbody.cpp | Switches state writes to get_ptr_to_state_writable / cond_get_ptr_to_state_writable. |
| src/particle/src/interaction/4C_particle_interaction_sph_temperature.cpp | Uses writable conditional access for temperature derivative updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_surface_tension.cpp | Switches multiple state updates to writable accessors. |
| src/particle/src/interaction/4C_particle_interaction_sph_surface_tension_recoilpressure_evaporation.cpp | Uses writable accessor for acceleration updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_surface_tension_interface_viscosity.cpp | Uses writable accessor for acceleration updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_surface_tension_barrier_force.cpp | Uses writable accessor for acceleration updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_rigid_particle_contact.cpp | Uses writable conditional accessor for force updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_pressure.cpp | Uses writable accessor for pressure assignment. |
| src/particle/src/interaction/4C_particle_interaction_sph_peridynamic.cpp | Uses writable accessors for bond/force/acceleration/damage updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_open_boundary.cpp | Uses writable accessors for boundary state updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_momentum.cpp | Uses writable accessors for acceleration/modified acceleration/force updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_heatsource.cpp | Uses writable accessor for temperature derivative updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_heatloss_evaporation.cpp | Uses writable accessor for temperature derivative updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_density.cpp | Uses writable accessors for density/colorfield accumulation and updates. |
| src/particle/src/interaction/4C_particle_interaction_sph_boundary_particle.cpp | Uses writable accessors for boundary particle state initialization. |
| src/particle/src/interaction/4C_particle_interaction_dem.cpp | Uses writable accessors for initialization and acceleration computation. |
| src/particle/src/interaction/4C_particle_interaction_dem_contact.cpp | Uses writable accessors for force/moment updates in contacts. |
| src/particle/src/interaction/4C_particle_interaction_dem_adhesion.cpp | Uses writable accessors for force updates in adhesion. |
| src/particle/src/engine/4C_particle_engine.cpp | Uses writable accessors where engine mutates particle states. |
| src/particle/src/engine/4C_particle_engine_typedefs.hpp | Adds a typedef alias for the new particle memory space enum. |
| src/particle/src/engine/4C_particle_engine_enums.hpp | Introduces ParticleSpace and declaration for enum-to-name conversion (has a Doxygen grouping issue). |
| src/particle/src/engine/4C_particle_engine_enums.cpp | Implements enum_to_space_name. |
| src/particle/src/engine/4C_particle_engine_container.hpp | Core API change: read-only vs writable accessors + host/device selection + DualView management (needs a HostSpace fix and const-cast cleanup). |
| src/particle/src/engine/4C_particle_engine_container.cpp | Implements Kokkos-backed allocation/resizing and updated add/remove particle logic (has a signed/unsigned warning risk). |
| src/particle/src/algorithm/4C_particle_algorithm_timint.cpp | Uses writable accessor for position perturbation. |
| src/particle/src/algorithm/4C_particle_algorithm_temperature_bc.cpp | Uses writable accessor for temperature updates. |
| src/particle/src/algorithm/4C_particle_algorithm_initial_field.cpp | Uses writable accessor for initial field assignment. |
| src/particle/src/algorithm/4C_particle_algorithm_dirichlet_bc.cpp | Uses writable accessors for applying Dirichlet BCs. |
| src/particle/src/algorithm/4C_particle_algorithm_constraints.cpp | Simplifies optional-state handling via conditional writable accessors. |
Review details
Comments suppressed due to low confidence (3)
src/particle/src/engine/4C_particle_engine_container.hpp:216
- Same as host sync: the C-style cast to call sync_device() is unnecessary with the current mutable members; removing it avoids misleading const-casting and simplifies the internal-mutability approach.
// casting away const-ness because this only changes the representation of the data
(*(Kokkos::DualView<double*>*)&states_dual_[state]).sync_device();
return (const double*)&(states_dual_[state].view_device().data()[index * statedim_[state]]);
src/particle/tests/4C_particle_container_test.cpp:345
- Same issue as above: this should use the read-only conditional accessor and a const pointer to match the test intent.
double* currvel = container_->cond_get_ptr_to_state_writable(Particle::Velocity, index);
src/particle/tests/4C_particle_container_test.cpp:348
- Same issue as above: keep this as a read-only conditional accessor so the test continues to validate the const API.
double* currmass = container_->cond_get_ptr_to_state_writable(Particle::Mass, index);
- Files reviewed: 30/30 changed files
- Comments generated: 7
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
e267518 to
0025fc0
Compare
|
Sorry for all the noise - all enums should be swapped to |
|
Last push was just a small tweak to improve debugging/logging output for the future, no logical changes to this PR that effect reviewability |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/particle/src/engine/4C_particle_engine_container.cpp:307
- New cross-space access/sync behavior is introduced (Host/Device branches with lazy DualView init), but the existing
ParticleContainerunit tests only exercise the default Host path. Please add/extend a unit test to call the new APIs withParticle::Space::Device(without requiring GPU kernels) to catch regressions like incorrect space forwarding and to validate sync/modify semantics.
else if (space == Particle::Space::Device)
{
if (!states_->is_dual_valid_[state_idx]) init_state_dual(state);
states_->dual_[state_idx].sync_device();
return &(states_->dual_[state_idx].view_device().data()[index * statedim_[state_idx]]);
src/particle/src/engine/4C_particle_engine_container.hpp:261
try_get_ptr_to_state_writable(..., space)does not forwardspacetoget_ptr_to_state_writable, so it always returns the Host pointer path even when Device is requested. It also leavesspaceunused (can trigger -Wunused-parameter with warnings-as-errors).
inline double* try_get_ptr_to_state_writable(
Particle::State state, int index, Particle::Space space = Particle::Space::Host)
{
FOUR_C_ASSERT(index >= 0 and index < particlestored_,
"can not return pointer to state of particle as index {} out of bounds!", index);
src/particle/src/engine/4C_particle_engine_container.hpp:43
- Adding an out-of-line destructor makes the type non-movable (move ctor/assignment are no longer implicitly declared when a destructor is user-declared). Since the class already can’t be copied due to the
std::unique_ptr, it’s likely still intended to be movable; consider explicitly defaulting move operations to avoid an accidental API regression.
//! constructor
explicit ParticleContainer();
//! destructor
~ParticleContainer();
ppraegla
left a comment
There was a problem hiding this comment.
Thanks for splitting the PR. It is much more reviewable now.
Could you post some performance benchmarks on how these changes affect the performance? Maybe the benchmark from @vovannikov is a good start.
In this #2136 (comment) you wrote about testing the changes. It is true that we do not have the capabilities to test this at the moment. But it would be good to start writing tests for it, so we do not forget them and have them ready once the testing infrastructure is in place. We cannot run the tests in the GitHub actions. But people can run them locally if they want. We can tackle this in a follow-up PR.
|
Ok, I have swapped this over to draft for now. It occurred to me that I could test this functionality via changes to the methods to manipulate the particle state values that I will need anyways |
|
I added some tests. Unfortunately I need to fix the delicate Trillinos-Kokkos-Cuda-Clang interplay on my local machine to try to runs those tests with Cuda enabled |
|
FYI @jeremylt , we have to temporarily disable the Kokkos CUDA buildtest due to issues with ccache (you may have noticed the kokkosparallel action consistently takes over an hour to run, whereas others make use of ccache). I am fixing it currently. And, yes, the actual installation and general environment interplay is extremely delicate unfortunately. Feel free to reach out to me if you get stuck on anything particular, though I'm sure you've become quite familiar with it by now |
|
Yea, these tests (modulo a typo) work correctly in a CPU only build, so that's good, but I'm going to have to play the conflicting CUDA+Clang versions song and dance for a bit to confirm that the changes do what they should on device. Keeping the PR as draft for now |
|
rebuilt and failure again, so something is screwy and I will have to figure out what exactly is broken that isn't broken for my sandbox project that's using the same Kokkos functions but with a simpler build env |
This PR adds dual memory spaces so that we can start implementing GPU porting for the particle code.
This dual memory space setup is standard for GPU enabled computation, but there are a few things I want to call out in my implementation.
Split read-only and writable access to underlying data - In order to reduce the number of memory transfers between the host and device (very expensive), we need to know every time writable access is requested, as this is the only time a synchronization between host and device is needed.
Lazy initialization of device memory - We will not need to carry all values to the device at first, so I set up lazy allocation of device memory. The default is host-only memory, but then a device side mirror of this memory is allocated the first time device side memory is requested. This will be the exact same space if Kokkos is only build with CPU support and not GPU support, but we let Kokkos manage that logic via its DualView object.
Only touching
ParticleContainer- It looked to me as though theParticleStatesobject was deeply connected with MPI communication and theParticleContainerholds the contiguous arrays of particle data that are used in the computationally heavy portions of the code. To that end, I only ported theParticleContainerdata to the dual memory space layout, as MPI communication is set up on the host. This can be revisited at a later time to enable GPU aware MPI, but that would be much more invasive of a change.Minimal dev facing changes - Due to the split of pointer accessors to readable and writable, with and additional optional argument for memory space, developers working in a file not yet converted to GPU computation do not have to think about any new loop constructs or anything. Even once the file is converted, all that should change is writing the loop heads via Kokkos syntax and adding an extra argument to the pointer accessors to request device memory.
Internal mutability pattern - I am doing something a bit strange that I want to call out and need the opinion of someone with more C++ experience. The read-only accessors are
const, but the Kokkos functions to sync the DualViews and the helper function I have to initialize the device side memory the first time it is requested are non-constfunctions. I would argue that this sort of internal mutation is not violatingconstness in any way the caller cares about - the underlying data is not changing, only the location (host or device). I havecast away themarked the relevant private object members asconstness of these DualViews inside of the read-only pointer acessors andmutable(seehere, here, andhere). I modeled this based upon how I would approach this in C, but I don't know if there is a more idiomatic/correct way to do this in C++.Edit: This has been updated to use "pointer to impl" instead - it has the same effects on const-correctness but also allows us to keep the Kokkos headers out of the hpp file and keep them only in the cpp files as they get ported to Kokkos.
Note: Does compile successfully with #2012 (expected as these files still all are compiled for the CPU)
LLM Disclosure: Used Copilot plugin in VSCode to prompt for review the commits a few times. Because of the LLM output, I added commit 93baac7 which moved the checks earlier before the count is incremented. With some prodding (because I have discomfort with it), Copilot commented on the internal mutability pattern I'm using for the Views, and I'm not entirely satisfied with it myself.
I ended up replacing it with PImp as that also allowed me to move the Kokkos headers out of the .hpp file into the .cpp file, so that dependency isn't added to files that do not strictly need it.
Original description for posterity: