Optimise runtime with collision, ADAS, div_ops, and state-management improvements - #604
Open
Quarrinexus wants to merge 15 commits into
Open
Optimise runtime with collision, ADAS, div_ops, and state-management improvements#604Quarrinexus wants to merge 15 commits into
Quarrinexus wants to merge 15 commits into
Conversation
bendudson
reviewed
Jul 3, 2026
bendudson
reviewed
Jul 3, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #604 +/- ##
==========================================
+ Coverage 49.09% 49.42% +0.32%
==========================================
Files 96 97 +1
Lines 10037 10113 +76
Branches 1452 1457 +5
==========================================
+ Hits 4928 4998 +70
- Misses 4604 4609 +5
- Partials 505 506 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Quarrinexus
added a commit
to Quarrinexus/hermes-3
that referenced
this pull request
Jul 3, 2026
…e allocation Address review comments from bendudson on PR boutproject#604: - cellAverage now delegates to cellAverageInto instead of duplicating the Simpson's rule integration - Always call result.allocate() before writing, since Field3D fields use copy-on-write and a shared backing array could otherwise be mutated non-locally
Quarrinexus
added a commit
to Quarrinexus/hermes-3
that referenced
this pull request
Jul 4, 2026
…e allocation Address review comments from bendudson on PR boutproject#604: - cellAverage now delegates to cellAverageInto instead of duplicating the Simpson's rule integration - Always call result.allocate() before writing, since Field3D fields use copy-on-write and a shared backing array could otherwise be mutated non-locally
Author
|
I've now run scripts/compare_builds.py between current upstream master and the rebased branch, both built with -DCHECK=1, 5 timed runs per test (1 warmup), across all 10 integrated tests including 2D-production/2D-recycling (nproc=10, MPI). Since this branch is now rebased onto current master, this also removes any potential doubt from the previous comparison having been run against a base several commits behind upstream. This confirms that setting hermes:check_state_values=false produces bitwise-identical output to the default (true) across every test, including under MPI. The results are below.
CHECK≥1 gains: measurable and generally larger than the CHECK=0 numbers in the PR description, since
3 ULP-tolerance failures (1D-recycling, 1D-recycling-dthe, drift-wave): pre-existing, explained in the intial PR-description. 2D production/2D recycling ULP-tolerance failures: Only one variable, |
dschwoerer
reviewed
Jul 6, 2026
…improvements to rest of code
…controller sensitivity - missing_input_files() was flagging collfreq-braginskii-afn and collfreq-multispecies as needing seeded BOUT.restart.*.nc files, because their runtest scripts mention "BOUT.restart" in a startup cleanup step (deleting stale files), not because they actually need one as input. Now only treat it as a real dependency when the runtest also fetches restart data externally (Zenodo/urllib), which is what 2D-production and 2D-recycling actually do. - 1D-recycling and 1D-recycling-dthe have a density/pressure feedback controller that chaotically amplifies any bit-level rounding change late in a run: valid floating-point reordering can flip which side of the controller's threshold the trajectory lands on, after which fields diverge from a different (but not incorrect) trajectory. Comparing the last timestep swamped the diff table with this expected noise. Compare an earlier timestep (empirically the last one before the threshold-driven divergence sets in) for these two tests instead.
Three changes that reduce per-RHS overhead without altering any floating-point arithmetic: * Permissions::bestMatchRights: memoise lookup results, since the permissions are fixed after initialisation but this is called for every state variable access in every RHS evaluation. Also do the section-prefix match without allocating a temporary string per permission entry. The cache is cleared whenever the permissions change (setAccess, substitute, operator>>). * GuardedOptions: replace the two tracker-map heap allocations per construction (per component, per transform) with a session ID from a counter. The maps are now only allocated when CHECKLEVEL >= 999, where they are actually populated and read. Equality semantics are preserved: objects from different constructions still compare unequal, via the session ID. * div_ops: cache the field-aligned (and Field3D-promoted) metric components used by Div_a_Grad_perp_flows and Div_a_Grad_perp_nonorthog. These are constant during a run but were being shifted and promoted on every call. Cached per Coordinates object; the flux loops only read these fields, so sharing the data is safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Instead of destroying and rebuilding the entire state tree with `state = Options()` on every RHS call, clear each value node in place. A cleared node is reset by move-assignment from an empty Options carrying the same full name, which makes it indistinguishable from a newly created node: isSet() returns false, the value and attributes are dropped, and reading it throws. Section nodes, their children maps and their name strings are reused rather than reallocated on every evaluation. Semantics notes: * All state reads go through getNonFinal/GetRef, which throw on cleared nodes, so a value that stops being written cannot be read silently from a previous evaluation. * isSection()/getChildren() can now see (empty) nodes created by a later component in a previous evaluation. All such uses in components either test statically-determined sections (species) or are paired with isSet() checks, so the behaviour is unchanged. Also: * GuardedOptions::GetRef with CHECK >= 1 now throws a clear BoutException when reading a variable with no value set, instead of failing with bad_variant_access (or silently returning false for GetRef<bool>). * Add hermes:check_state_values option (default true) to allow the per-set finiteness sweep over field values to be disabled at runtime, so its cost can be measured or avoided without rebuilding with CHECK=0. Values are unchanged either way; only the failure behaviour on non-finite data differs. * neutral_full_velocity: communicate Dnn, kappa_n and eta_n in a single call, reducing three MPI guard-cell exchanges to one. All files pass g++ -fsyntax-only against the BOUT++ submodule headers at CHECK=0/1/2/999; needs a full build and compare_builds.py run for verification. Field differences should be exactly zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The performance branch now contains two classes of change with different verification requirements: * Float-reordering optimisations (earlier commits): differences up to a few ULPs are expected. The existing masked relative/ULP comparison at a single timestep (--expect ulp, the default) covers these. * Bit-identical framework changes (0420748, 3bb5bab): permissions caching, GuardedOptions session IDs, cached aligned metrics, persistent state tree, merged communication. These must produce exactly the same output. --expect identical compares every common variable at every timestep bit-for-bit (via int64 views), which also catches what the masked comparison cannot: differing zeros, signed zeros, NaN payloads and infinities. Because truly identical builds cannot diverge, the feedback-controller timestep cap is skipped in this mode and a mismatch in the number of output steps is itself a failure. --verify-check-state-values additionally runs the NEW binary once per test with hermes:check_state_values=false, requires bitwise-identical output, and reports the time saved by skipping the per-set finiteness sweeps (only meaningful for CHECK >= 1 builds). The module docstring now maps each change to the tests that exercise it, and records that neutral_full_velocity (merged communicate call) has no integrated-test coverage. Comparison logic verified with stub datasets: identical data passes; single-ULP flips, signed-zero and NaN-payload differences, first-timestep-only differences and timestep count mismatches all fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hermes-3 never writes the x-boundary guard cells of diagnostic fields (sources, ddt(...)); their output contents are whatever the recycled Array allocation last held. The div_ops aligned-metric cache changes the allocation sequence in shifted-metric runs, so 2D-production and 2D-recycling showed bitwise diffs confined to x-guard columns of 7 diagnostics (with junk-scale values like 1.9e+219 in the OLD build) while all interior values and all evolved fields, guard cells included, are bit-identical. --expect identical now rechecks each differing variable with the x-guard columns stripped (using MXG from the dataset metadata) and reports guard-only diffs as a note instead of a failure. Diffs in any written cell still fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LFRUq6uiGX3UvZJYZhjjt
The x-guard-cell false-positive fix (2191c45) only applied to --expect identical. A master-vs-final-branch comparison uses the default ULP-tolerant path instead, and hits the same issue: master's allocation sequence for never-written diagnostic guard cells differs from any build including the div_ops metric cache (0420748), so those cells show large spurious relative/ULP diffs unrelated to correctness. Reuse the same guard-cell recheck there.
Replace fork-specific commit hashes (b8d46fb, 0420748, 3bb5bab, etc.) and "the performance branch" framing with generic descriptions of the two change classes (FP-reordering vs. bit-identical caching/refactor) the tool is meant to distinguish. The tool itself was always generic; only the documentation referenced this fork's specific history.
…e allocation Address review comments from bendudson on PR boutproject#604: - cellAverage now delegates to cellAverageInto instead of duplicating the Simpson's rule integration - Always call result.allocate() before writing, since Field3D fields use copy-on-write and a shared backing array could otherwise be mutated non-locally
No functional changes.
…before mesh->communicate
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
3 participants
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.
Description
Purpose
To reduce hermes-3 runtime cost via a set of internal optimisations to the collision-frequency calculation, ADAS reaction-rate evaluation, div_ops metric handling, and per-RHS-evaluation state management — with no change to simulation physics or user interfaces. No new physics capability.
Change Summary
Two classes of change, verified separately (different correctness bar for each, see Validation):
1. Floating-point reordering (mathematically identical, not bit-identical):
braginskii_collisions.cxx/braginskii_ion_viscosity.cxx: hoists scalar (per-species) constants out of the collision-frequency loops; replacespow(x, 1.5)withx * sqrt(x).adas_reaction.cxx/integrate.hxx: Introduces cellAverageInto() so that OpenADAS and OpenADASChargeExchange reuse a persistent Field3D workspace instead of constructing a fresh one on every evaluation call.2. Framework changes (no algebraic reordering at all, bit-identical):
div_ops.cxx: Replaced an expensive modulo operator (%) used for boundary wraparound with a standard if/else conditional branch. Because this logic only handles integer grid indices, the output is identical.permissions.cxx/guarded_options.*: Optimises access-rights lookup and memory tracking. It now remembers the results of bestMatchRights and replaces high-overhead, per-construction tracking maps with a more lightweight session-ID scheme.div_ops.cxx: Caches grid metric calculations within theCoordinatesobject, the dominant performance gain in this PR, avoiding repeated recalculation and shifting of field-aligned components on every flow evaluation.hermes-3.cxx: the stateOptionstree persists between RHS evaluations instead of being rebuilt every call.neutral_full_velocity.cxx: merges threemesh->communicate()calls into one.hermes:check_state_valuesoption (default true) to skip the per-set()finiteness sweep when unneeded.Validation
Verified on Imperial College London's CX3 HPC across all 10 integrated tests using
scripts/compare_builds.py(new file, included in this pull request), which runs both binaries simultaneously and reports per-variable field differences as well as wall-clock timing.Master vs. this branch (commit 7f0ae4d), single PBS job across all 10 tests, 5 timed runs per binary per test (note compare_builds.py defaults to 3):
neutral_mixed1D-recyclingdrift-wave1D-recycling-dthe2D-recyclingalfven-wavecollfreq-braginskii-afnvorticitycollfreq-multispecies2D-production¹ One of the 5 timed runs for the new binary spiked to 121s against the other 4's tight 29.6-29.9s (scheduler/network hiccup, not reproducible). NEW (mean) above and the speedup figure both exclude that outlier.
Caching the grid metrics in div_ops provides the largest performance boost. Diffusion-heavy simulations (like
neutral_mixed) show a ~25-30% speedup in production (CHECK=0): the exact figure has varied between separate job submissions on this shared HPC queue (11.8% in an earlier 3-run job vs. 30.2% here), but this test is consistently the largest and most reproducible win across every comparison in this investigation. The speedup will be even greater in development builds (CHECK≥1) because this cache also eliminates a repetitive string scan and a full-field validation sweep that are normally compiled out of production runs.No correctness bug was found in the actual code changes.
compare_builds.pydid surface differences on some tests, but each traces to one of two known, explained causes rather than being hidden; see Review Notes for the full details on these and other caveats.Run it yourself:
AI Assistance
scripts/compare_builds.pyverification tool.Documentation
scripts/compare_builds.pydocuments itself via its module docstring and--help(meaning users can view usage instructions directly in the terminal).docs/sphinx/) was not changed for the following reasons:hermes:check_state_valuesis self-documenting. It hooks into the codebase's existing.doc()feature to automatically display its own description text to users when requested.Review Notes
1D-recycling/1D-recycling-dthehave a density-feedback controller that chaotically amplifies any bit-level rounding change late in the run; a valid FP reordering can land the density trajectory on the opposite side of the controller's threshold, after which the two runs integrate different (both valid) trajectories.compare_builds.pycompares an earlier timestep for these two tests specifically, and still reports the residual differences.SNVd+,ddt(Nd), etc., on2D-production/2D-recycling) are never written by Hermes-3, so their output is whatever stale allocation content happens to be there; this differs between builds whenever the allocation sequence changes (e.g. the div_ops metric cache), unrelated to correctness.compare_builds.pydetects and classifies this rather than reporting a false failure. Might be worth a follow-up to zero those cells on write so output is byte-reproducible in general.master. The diff still applies and merges without conflict, but the correctness/performance verification above is relative to the fork point, not currentmaster. (tested in later comment)neutral_full_velocity's mergedcommunicate()has no integrated-test coverage; its correctness argument is structural only (guard-cell exchanges of independent fields commute), not verified by a test in this PR.hermes:check_state_values=falseis untested.compare_builds.pyhas a dedicated--verify-check-state-valuesflag for exactly this (runs the NEW binary once more with the option disabled and requires bitwise-identical output), but it was never invoked in this investigation, it's only meaningful on a CHECK≥1 build, and every build here used CHECK=0, where the finiteness sweep the option skips is already compiled out. So beyond ensuring the change's existence doesn't cause any issues, the option's actual claim (disabling it is safe and saves time) has no test coverage yet; verifying it would need a CHECK≥1 build. (tested in later comment)