From 4ef79b6b45ca836ed59819f2fec6efeb369775ae Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sat, 25 Jul 2026 02:41:55 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#200=20review=20round?= =?UTF-8?q?=20=E2=80=94=20cache=20sizing=20truth=20+=20endorheic=20asserti?= =?UTF-8?q?on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoshe finding 1: dropping HydrologySample.elevation was PROVEN unsafe (TerrainAnalysis.elev_pct is a rank percentile, not raw elevation; HydrologyResult carries no elevation) — the copy stays, and the truth moves into the docs instead: gen_queue's TerrainAnalysisCache sizing comment corrected to real 512x256 working-grid numbers (~1.57 -> ~2.62 MB/entry, capacity-8 worst case ~21 MB), clone-on-hit cost documented, byte-safety proof recorded on HydrologySample itself. Arc follow-up filed as T-1187. Hoshe finding 2: the endorheic-split test now asserts the bowl basin's BasinOutcome actually diverges (Endorheic at moisture 0, Overflow at 100, straddling ENDORHEIC_MOISTURE_CEILING=60) plus basin-count sanity — mutation-verified by stubbing is_endorheic and watching it fail. Doc/test-only round: goldens byte-unchanged, full suite 2114 green. Co-Authored-By: Claude Fable 5 --- server/src/atlas/features.rs | 21 +++++++++++ server/src/atlas/gen_queue.rs | 66 +++++++++++++++++++++++++++-------- server/src/atlas/layer1.rs | 63 +++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 14 deletions(-) diff --git a/server/src/atlas/features.rs b/server/src/atlas/features.rs index f7c985e8e..2a7730ecd 100644 --- a/server/src/atlas/features.rs +++ b/server/src/atlas/features.rs @@ -98,6 +98,27 @@ pub struct TerrainAnalysis { /// `[0.0, 1.0]` normalized domain the raw heightmap and `sea_level` already /// share — so a bilinear sample of one is directly comparable to a bilinear /// sample of the other, no rescaling at the call site. +/// +/// **Size + clone cost (PR #200 review, Hoshe finding 1):** two `Vec` at +/// the real 512×256 working grid = ~1.05 MB/entry, added on top of +/// `TerrainAnalysis`'s pre-existing ~1.57 MB of dense fields (~2.62 MB total, +/// ×1.67 growth, not quite a doubling) — see the corrected sizing comment on +/// `GenWorkItem::DeriveWindow` (`gen_queue.rs`) for the full accounting and +/// the `TerrainAnalysisCache` cache-HIT clone-cost note (every hit +/// deep-copies both these `Vec`s, not just the first miss/insert). +/// +/// **`elevation` is a deliberate, provably-necessary redundant copy, not an +/// oversight.** `TerrainAnalysis` has no OTHER field that retains the raw +/// `[0,1]` heightmap: `elev_pct` is a RANK PERCENTILE (`rank(elev[i]) / +/// (land_cell_count - 1)`, `compute_elev_percentile`'s own doc/impl) — +/// mathematically a different quantity from absolute elevation, and NOT +/// safe to compare against `filled` (two cells at different true elevations +/// can share adjacent ranks; ocean cells are forced to `0.0` regardless of +/// their real depth). `HydrologyResult` itself carries no elevation field +/// either (`hydrology_equilibrium.rs`: `basins`, `filled_scaled`, +/// `channel_depth_scaled`, `cliff_edge` — no `original`/`elevation` member). +/// So there is no existing bit-identical grid this field could point at +/// instead — carrying its own copy is the only byte-safe option today. #[derive(Debug, Clone)] pub struct HydrologySample { /// The original (unfilled) heightmap elevation, `[0.0, 1.0]`. Not stored diff --git a/server/src/atlas/gen_queue.rs b/server/src/atlas/gen_queue.rs index 6bb45f9fc..5196633e1 100644 --- a/server/src/atlas/gen_queue.rs +++ b/server/src/atlas/gen_queue.rs @@ -197,20 +197,58 @@ pub enum GenWorkItem { /// `drain_generation_completions` documents). /// /// **TerrainAnalysis availability (T-1137 binding decision, with numbers; - /// corrected 2026-07-21 per PR #187 review — Tyre C1):** `BodyWorldState` - /// does NOT retain `TerrainAnalysis` after cascade completion (T-1044 - /// scoped its transient-carry fix to *within-cascade* reuse only — - /// `cascade.rs` drops it once `DistrictProfile`+`RoadGraph` finish; see the - /// doc on `CascadeSnapshot::terrain_analysis`). Caching it alongside every - /// `BodyWorldStateCache` entry would cost ~2 MB × 50-body capacity ≈ - /// 100 MB of PERMANENT resident cost, paid by every cached body whether or - /// not a window is ever requested for it — the exact D-203 budget concern - /// T-1044's own ticket text guarded against. So `run_work_item` re-derives - /// via `run_layer1` (matching `aliveness_probe`'s existing `--render` - /// workaround) rather than persisting a field on `BodyWorldState` — but - /// NOT unconditionally on every work item: the actual model is a small - /// **per-body LRU** (`TerrainAnalysisCache`, capacity 8, ~16 MB worst - /// case), consulted before every re-derive. The FIRST `DeriveWindow` on a + /// corrected 2026-07-21 per PR #187 review — Tyre C1; RE-CORRECTED + /// 2026-07-25 per PR #200 review — Hoshe finding 1, T-1184's hydrology + /// field addition):** `BodyWorldState` does NOT retain `TerrainAnalysis` + /// after cascade completion (T-1044 scoped its transient-carry fix to + /// *within-cascade* reuse only — `cascade.rs` drops it once + /// `DistrictProfile`+`RoadGraph` finish; see the doc on + /// `CascadeSnapshot::terrain_analysis`). Caching it alongside every + /// `BodyWorldStateCache` entry would cost **~2.62 MB × 50-body capacity ≈ + /// 131 MB** of PERMANENT resident cost (real per-entry figure below — + /// this multiplier is unchanged, only the per-entry base moved), paid by + /// every cached body whether or not a window is ever requested for it — + /// the exact D-203 budget concern T-1044's own ticket text guarded + /// against. So `run_work_item` re-derives via `run_layer1` (matching + /// `aliveness_probe`'s existing `--render` workaround) rather than + /// persisting a field on `BodyWorldState` — but NOT unconditionally on + /// every work item: the actual model is a small **per-body LRU** + /// (`TerrainAnalysisCache`, capacity 8), consulted before every + /// re-derive. + /// + /// **Real per-entry size (T-1184, corrected from the stale "~2 MB" + /// figure the PR #187 pass computed before hydrology existed):** at the + /// real `GRID_W × GRID_H = 512 × 256` working grid (131,072 cells, the + /// SAME grid every `TerrainAnalysisCache` entry is built at — NOT the + /// 1024×512 stored-heightmap-PNG resolution, D-202, which is downsampled + /// away before `run_layer1` ever runs) — pre-T-1184 dense fields + /// (`ocean_mask`/`lake_mask`: `Vec`, 1 B/cell each; `water_dist`: + /// `Vec`, 2 B/cell; `slope_deg`/`elev_pct`: `Vec`, 4 B/cell + /// each) sum to **~1.57 MB**. T-1184's `HydrologySample` (`elevation` + + /// `filled`, both `Vec`, 4 B/cell) adds **~1.05 MB** — **not quite a + /// doubling** (×1.67, not ×2), but a real, non-trivial per-entry growth: + /// **post-T-1184 total ~2.62 MB/entry.** At capacity 8 that is **~21.0 MB + /// worst case** (was ~12.6 MB pre-T-1184; the old "~16 MB" comment was + /// itself a round-up of the pre-T-1184 figure, not a post-hydrology one). + /// Both this cache and `BodyWorldStateCache`'s counterfactual above use + /// the corrected ~2.62 MB base. + /// + /// **Clone-on-HIT, not just insert (Hoshe finding 1, binding for the next + /// sizing decision):** `get_or_derive`'s cache-hit branch + /// (`return (l1.clone(), ta.clone())`) deep-copies the FULL + /// `TerrainAnalysis` — including both new `HydrologySample` `Vec` + /// fields — on EVERY hit, not merely on the first miss/insert. A + /// pan-heavy session hitting the same body's cache entry repeatedly pays + /// the ~2.62 MB clone cost per `DeriveWindow` work item, not once per + /// body. This was already true pre-T-1184 for the smaller ~1.57 MB + /// struct; T-1184 makes the per-hit clone cost ~1.67× larger, not a new + /// category of cost. **Not fixed this round** (an `Arc` + /// refactor — sharing one heap allocation across hits instead of + /// deep-copying — is the obvious next step if hit-heavy clone cost ever + /// shows up in a profile, but is out of scope for T-1184; flagged for a + /// follow-up ticket rather than done speculatively here). + /// + /// The FIRST `DeriveWindow` on a /// body pays the full ~45 ms `run_layer1` cost and populates that body's /// cache entry; EVERY SUBSEQUENT window on the SAME body (any `center`/`n`, /// not just an exact repeat — that narrower case is what diff --git a/server/src/atlas/layer1.rs b/server/src/atlas/layer1.rs index 5f365c5ae..ac8ce5d26 100644 --- a/server/src/atlas/layer1.rs +++ b/server/src/atlas/layer1.rs @@ -505,5 +505,68 @@ mod tests { "lake extent (filled_scaled) must be moisture-independent — only \ the endorheic/overflow split may vary with moisture_q" ); + + // PR #200 review, Hoshe finding 2: the assertion above only proves + // EXTENT is moisture-independent — it says nothing about whether the + // endorheic/overflow split (this test's OWN namesake claim) actually + // moves at all. `TerrainAnalysis.hydrology` doesn't carry + // `BasinOutcome` (only the continuous `elevation`/`filled` fields + // `derive_morphology_zone`'s lake gate needs — see `HydrologySample`'s + // doc), so re-solve directly via `hydrology_equilibrium::solve` to + // reach `Basin::outcome`. `moisture_q: 0` and `moisture_q: 100` + // straddle `ENDORHEIC_MOISTURE_CEILING = 60` on either side, so the + // SAME basin (same elevation geometry, same `bowl_hm`) must classify + // differently — a regression that makes the endorheic split dead code + // (e.g. `is_endorheic` always returning `false`, or `moisture_q` never + // threading through to it) would leave the `filled` assertion above + // green but SHOULD fail here. + let dry_result = crate::atlas::hydrology_equilibrium::solve( + &h.data, + h.width, + h.height, + h.sea_level, + crate::atlas::hydrology_equilibrium::ClimateInputs { moisture_q: 0 }, + ); + let wet_result = crate::atlas::hydrology_equilibrium::solve( + &h.data, + h.width, + h.height, + h.sea_level, + crate::atlas::hydrology_equilibrium::ClimateInputs { moisture_q: 100 }, + ); + assert_eq!( + dry_result.basins.len(), + 1, + "fixture sanity: bowl_hm(64, 32, ...) must produce exactly one basin" + ); + assert_eq!(wet_result.basins.len(), 1, "fixture sanity: same on the wet solve"); + let dry_outcome = &dry_result.basins[0].outcome; + let wet_outcome = &wet_result.basins[0].outcome; + assert_ne!( + dry_outcome, wet_outcome, + "the SAME basin (identical elevation geometry) must classify \ + differently at moisture_q=0 vs moisture_q=100 — these straddle \ + ENDORHEIC_MOISTURE_CEILING=60, so a regression making the \ + endorheic/overflow split dead code would collapse both to the \ + same outcome here" + ); + // Non-vacuous on BOTH halves of this test's own name: dry actually + // IS Endorheic, wet actually IS Overflow — not just "different from + // each other" (which alone couldn't rule out both being some other + // unrelated pair of values). + assert!( + matches!( + dry_outcome, + crate::atlas::hydrology_equilibrium::BasinOutcome::Endorheic { .. } + ), + "moisture_q=0 (well below the ceiling) must classify Endorheic; got {dry_outcome:?}" + ); + assert!( + matches!( + wet_outcome, + crate::atlas::hydrology_equilibrium::BasinOutcome::Overflow { .. } + ), + "moisture_q=100 (well above the ceiling) must classify Overflow; got {wet_outcome:?}" + ); } }