perf: add nexus flush() to bound memory on long runs - #990
Conversation
|
It looks like |
A HY_HydroNexus accumulates per-timestep flow state (upstream_flows,
downstream_requests, summed_flows, total_requests) for every time step it
sees. On long CONUS-scale runs that growth is unbounded and dominates memory.
Add a virtual flush(bool clear_completed = false) to the HY_HydroNexus
interface that releases this accumulated state:
- HY_PointHydroNexus::flush clears the four accumulator maps, and the
completed-timestep set when clear_completed is true.
- HY_PointHydroNexusRemote::flush opportunistically reaps completed MPI
sends/receives (non-blocking) and then delegates to the base. It is
deliberately non-blocking: in-flight sends are left to complete naturally
(already bounded by the spinlock in add_upstream_flow, and they carry their
own payload buffers so clearing the accumulators cannot corrupt them), and
no new receives are posted (get_downstream_flow has already drained this
nexus's receives; posting one here would consume a future timestep's
message that the clear would then discard). Blocking on send completion
would turn a per-timestep flush into an inter-rank synchronization point
that can stall progress under load imbalance, so it is avoided.
SurfaceLayer calls flush(true) on each nexus once its contribution for the
current time step has been collected and committed. clear_completed=true is
safe there: in a dendritic network each nexus is requested once at 100%, which
never marks a timestep "completed" (that only happens when multiple partial
requests sum to 100%), so the completed set is empty in normal operation.
Tests:
- NexusTests (serial): flow-state release and both clear_completed branches.
- NexusRemoteTests (mpirun -np 2): the production pattern -- exchange a flow
then flush(true) on every rank, every time step -- asserting flows stay
correct and no deadlock through the final step.
- NexusRemoteTests (mpirun -np 4): a timing-skewed pipeline where a lagging
rank lets its upstream sender race ahead, buffering future-timestep
messages; verifies flush() neither loses that buffered data nor deadlocks.
Tests generated by Claude Opus 4.8 (1M context).
set_mintime(t) advanced a min_timestep watermark, pruned every per-timestep map (and completed) below it, and made add_upstream_flow / get_downstream_flow reject any timestep < min_timestep. Remove set_mintime, the min_timestep member and its two dead guards, and the now-unused invalid_time_step exception, leaving one clear mechanism for releasing nexus state instead of two slightly different variations. The completed set already rejects operations on a fully-drained timestep automatically and per-step, so the min_timestep watermark was a redundant, manually-driven second guard. flush(clear_completed) keeps the one thing set_mintime uniquely did -- pruning completed -- as an explicit flag.
e651858 to
48e1c57
Compare
I just pushed a commit to drop the set_mintime function since it was effectively dead code at this point anyways. It suffered a few issues that made it complicated (and buggy) to use, and for the purpose of reclaiming memory a I also fixed a test case that was causing the mpi nexus tests to hang, so that should be cleared up as well. |
| } | ||
|
|
||
| void HY_PointHydroNexus::set_mintime(time_step_t t) | ||
| void HY_PointHydroNexus::flush(bool clear_completed) |
There was a problem hiding this comment.
Do we have a use case for clear_completed = false? The tests exercise that, but the code doesn't use it. If not, can we just get rid of the parameter and make it unconditional?
There was a problem hiding this comment.
This is currently a defensive guard, and is required to ensure existing semantics of add/get flow are preservable. This is why false is the default. Only callers that guarantee monotonic processing and consuming of a nexus state should use clear_completed=true, which the current surface layer caller does do.
The semantics here to consider:
- clearing the accumulators drops consumed data; (happens on all calls to flush)
- clearing completed drops the guard that makes add/get on an already-drained time step throw an error.
From get_downstream_flow
double HY_PointHydroNexus::get_downstream_flow(std::string catchment_id, time_step_t t, double percent_flow)
{
if ( completed.find(t) != completed.end() ) BOOST_THROW_EXCEPTION(completed_time_step());
.
.
.
if (100.0 - total_requests[t] < 0.00005 )
{
// all water has been requested remove bookeeping
upstream_flows.erase(upstream_flows.find(t));
downstream_requests.erase(downstream_requests.find(t));
summed_flows.erase(summed_flows.find(t));
total_requests.erase(total_requests.find(t));
completed.emplace(t); // <-- T is now "completed"
}
and add_upstream_flow
void HY_PointHydroNexus::add_upstream_flow(double val, std::string catchment_id, time_step_t t)
{
if ( completed.find(t) != completed.end() ) BOOST_THROW_EXCEPTION(completed_time_step());
For any time step that has had its flow completely requested (so it's in completed), a later add_upstream_flow(…, T) or get_downstream_flow(…, T) throws completed_time_step ("Can not operate on a completed time step").
This guard exists independent of the flush semantics, and flush(false) simply preserves it. If any code does try to modify a nexus at an already-drained time, the existing code would throw.
flush(true) removes this guarantee while reclaiming the memory needed to store the information for enforcing this invariant. In this case, the same re-entry silently re-accumulates a fresh T and returns a wrong result instead of erroring.
Time steps that were never fully drained were never in completed, so they're unaffected either way.
There was a problem hiding this comment.
I'm a bit confused about the reasoning here. This is a new function, so there are no callers, besides the one introduced in this PR. What other callers are we worried about?
There was a problem hiding this comment.
Any potential refactor, additional layers, or future uses of the Nexus feature type. As of right now, there are no other callers, hence the defensive posture to make sure any potential caller is explicitly aware of the completed book keeping semantics.
2 participants
A HY_HydroNexus accumulates per-timestep flow state (upstream_flows, downstream_requests, summed_flows, total_requests) for every time step it sees. On long CONUS-scale runs that growth is unbounded and dominates memory.
Add a virtual flush(bool clear_completed = false) to the HY_HydroNexus interface that releases this accumulated state:
SurfaceLayer calls flush(true) on each nexus once its contribution for the current time step has been collected and committed. clear_completed=true is safe there: in a dendritic network each nexus is requested once at 100%, which never marks a timestep "completed" (that only happens when multiple partial requests sum to 100%), so the completed set is empty in normal operation.
Tests: