Skip to content

Use mapreduce_neighbor and remove Ref values#1211

Draft
efaulhaber wants to merge 3 commits into
trixi-framework:mainfrom
efaulhaber:mapreduce-neighbor
Draft

Use mapreduce_neighbor and remove Ref values#1211
efaulhaber wants to merge 3 commits into
trixi-framework:mainfrom
efaulhaber:mapreduce-neighbor

Conversation

@efaulhaber
Copy link
Copy Markdown
Member

@efaulhaber efaulhaber commented May 25, 2026

Based on trixi-framework/PointNeighbors.jl#158.

This PR improves two patterns I introduced in #1124, #1125, #1130, #1116 and #1139.
I changed the RHS code to work on Ref values, but this is suboptimal both aesthetically (is ugly) and functionally (prevents SIMD vectorization on CPUs). This PR changed many lines, but most of these are boilerplate changes. The actual change is very simple as explained below.

Pattern 1

main:

dv_particle = Ref(zero(v_a))

foreach_neighbor(...) do particle, neighbor, pos_diff, distance
    ...
    dv_particle[] += result
end

Since this prevents SIMD vectorization, I introduced a clean mapreduce function in trixi-framework/PointNeighbors.jl#158.

dv_particle = mapreduce_neighbor(+, ..., init=zero(v_a)) do particle, neighbor, pos_diff, distance
    ...
    return result
end

Pattern 2

main:

dv_viscosity!(dv_particle, ...)

This pattern was consistent with pattern 1 on main and successfully removed adds of zero SVectors to improve GPU performance, but now with pattern 1 changed, it became

dv_ref = Ref(dv_particle)
dv_viscosity!(dv_ref, ...)
return dv_ref[]

which is ugly and now boxes the value in a Ref for no reason.

All of these I changed to

dv_particle = dv_viscosity(dv_particle, ...)

So all functions that previously inplace modified Ref values now just return the new value.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors neighbor-interaction kernels to avoid Ref-based accumulation and to use mapreduce_neighbor, improving CPU SIMD/vectorization opportunities while keeping the “accumulate then write once” pattern for GPU performance.

Changes:

  • Replace foreach_neighbor + Ref accumulators with mapreduce_neighbor reductions in selected RHS/structure kernels.
  • Convert several previously Ref-mutating helpers (e.g., viscosity, shifting, surface tension, continuity/density diffusion) into pure functions returning the updated value.
  • Update affected call sites and viscosity tests to match the new return-value APIs.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/schemes/fluid/viscosity.jl Updates viscosity tests to use return-value API instead of Ref mutation.
src/schemes/structure/total_lagrangian_sph/system.jl Switches neighbor accumulation to mapreduce_neighbor for deformation gradient assembly.
src/schemes/structure/total_lagrangian_sph/rhs.jl Refactors TLSPH structure-structure RHS accumulation to mapreduce_neighbor.
src/schemes/structure/structure.jl Updates structure–fluid interaction helpers to use return-value APIs (viscosity/adhesion/continuity).
src/schemes/structure/rigid_body/system.jl Converts rigid-body adhesion force helper to return-value style.
src/schemes/fluid/weakly_compressible_sph/rhs.jl Uses mapreduce_neighbor to accumulate (dv, drho) contributions without Refs.
src/schemes/fluid/weakly_compressible_sph/density_diffusion.jl Converts density diffusion kernel to return updated drho rather than mutating a Ref.
src/schemes/fluid/viscosity.jl Converts viscosity dispatch and implementations to return updated dv_particle.
src/schemes/fluid/surface_tension.jl Converts surface tension + adhesion force helpers to return updated dv_particle.
src/schemes/fluid/shifting_techniques.jl Converts shifting term helpers to return updated dv_particle.
src/schemes/fluid/implicit_incompressible_sph/system.jl Updates viscosity usage to return-value API.
src/schemes/fluid/implicit_incompressible_sph/rhs.jl Updates viscosity usage to return-value API.
src/schemes/fluid/fluid.jl Converts continuity equation + density diffusion application to return-value API.
src/schemes/fluid/entropically_damped_sph/rhs.jl Updates viscosity/shifting/surface tension/adhesion/continuity usage to return-value APIs.
src/schemes/boundary/wall_boundary/rhs.jl Converts boundary continuity equation helpers to return-value API.
src/schemes/boundary/open_boundary/rhs.jl Updates viscosity/continuity usage to return-value APIs.
src/io/write_vtk.jl Updates surface tension postprocessing to use return-value API.
src/general/neighborhood_search.jl Adds TrixiParticles wrapper(s) for mapreduce_neighbor, including a GPU unsafe fast path.
Comments suppressed due to low confidence (1)

src/schemes/structure/total_lagrangian_sph/rhs.jl:86

  • mapreduce_neighbor is used to avoid Ref-based accumulation, but this closure still allocates/uses a Ref per neighbor to call dv_penalty_force! / dv_viscosity_tlsph!. This undermines the stated goal of removing Ref boxing and may add per-neighbor overhead. Consider providing non-mutating variants (e.g., dv_penalty_force / dv_viscosity_tlsph returning the updated value) so the closure can stay allocation-free and just return dv_particle.
            dv_particle = Ref(m_b * (pk1_rho2_a + pk1_rho2_b) * grad_kernel)

            @inbounds dv_penalty_force!(dv_particle, penalty_force, particle, neighbor,
                                        initial_pos_diff, initial_distance,
                                        current_pos_diff, current_distance,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants