Repository navigation
r.sim: Add walker weights to the water depth atomically - #7974
Merged
Merged
Conversation
With nprocs > 1, walkers on different threads added their weights to the same cells of the water depth (or sediment concentration) grid without synchronization. This is a data race: of two concurrent additions to a cell, one can be lost, so parallel runs lose water. On a small bowl with 2000 walkers, the total was 13% to 21% lower at nprocs=4 than at nprocs=1. Add the weight with an atomic capture, so that no addition is lost and the diffusion decision uses the updated sum, as in the serial code. Parallel results still depend on the order in which threads add the weights, as before, but no water is lost. Single-threaded results do not change: the depth, discharge, sediment flux and erosion/deposition maps computed from the nc_spm elevation, with and without infiltration, are byte-identical to those before this change. Written with the help of Claude Code.
Compare the total water accumulated with 4 threads against a single-threaded run on a bowl which keeps all walkers in the region. Without diffusion, the random numbers do not affect the walkers, and all of them end up in the few cells at the bottom of the bowl, so unsynchronized updates of the same cell are likely. Also test a walker count which is not a multiple of the thread count, which lost the last walker before the chunk size was rounded up. Without the atomic addition of walker weights, both tests fail with totals 12% to 14% short at nprocs=4 in every run. The tests take about 2 s. Written with the help of Claude Code.
wenzeslaus
force-pushed
the
simwe-depth-atomic
branch
from
September 30, 2026 10:36
46f7570 to
408dc9b
Compare
petrasovaa
approved these changes
Sep 30, 2026
This was referenced Sep 30, 2026
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
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.
With nprocs > 1, walkers on different threads add their weights to the same cells of the r.sim water depth (or sediment concentration) grid without synchronization, so additions are lost and parallel runs lose water. This makes the addition atomic. Single-threaded results do not change.
This adds 6-7% of runtime for nprocs=1 and 5-8% for 4 and 8 threads for small raster (200×200). An alternative could have a branch for nprocs=1 which would not use the atomic. Notably, this additional cost will not go away with the new random numbers (WIP), but the new RNG code should make the parallel code faster (much faster comparing to what is now on main which is RNG with atomics, and comparing to state before atomics with shared generator without locking, the new RNG code should be somewhat faster in serial, and in parallel, it should be actually faster parallel then serial as in #7884).
So, while the atomics slightly decrease performance comparing to hitting same variables with race condition, they add correctness and a better parallel implementation will outweigh the cost of atomics. The choice is between one simple code with slight cost for single-threaded run (which will still do part of the atomics operations; cost 6% on 4 s run) and duplicating the code slightly to most out of the single-threaded run and keeping the atomics to parallel runs (with <1% cost comparing to main for a single-thread).
Locally, the tests run about 2 s adding to the 11.5 s of other pytest tests (so maybe 3 s in CI).
Details (AI generated)
grids->gama[k][l] +=inmain_loop()(raster/r.sim/simlib/hydro.c) becomes anomp atomic capture. The captured sum is what the diffusion decision uses, as in the serial code.raster/r.sim/r.sim.water/tests/r_sim_water_nprocs_test.py:Cost of the atomic, and an alternative
Median of 7 interleaved runs, 200x200 cells, 200k walkers, 10 min simulated. The third column is an experiment, not in this PR: it reads the thread count once per parallel region and uses a plain
+=when only one thread runs.Squash-merge commit message
The analysis, code and tests were done with Claude Code (Opus) and reviewed by me.