diff --git a/docs/superpowers/specs/2026-06-23-datlock-contention-fix-design.md b/docs/superpowers/specs/2026-06-23-datlock-contention-fix-design.md new file mode 100644 index 00000000..64e51547 --- /dev/null +++ b/docs/superpowers/specs/2026-06-23-datlock-contention-fix-design.md @@ -0,0 +1,177 @@ +# Design — eliminate `_datLock` contention in the terrain apply (FPS swing fix) + +**Date:** 2026-06-23 +**Branch:** `claude/thirsty-goldberg-51bb9b` (worktree) +**Status:** approved (A1), ready for implementation plan +**Scope:** the FPS 30↔200 swing / GPU rev only. NOT the entity leak / GPU +heat-up (H3) and NOT the invisible-char-on-portal bug — both are separate, +already-scoped follow-ups in the same code area. + +--- + +## 1. Problem (measured, not inferred) + +FPS swings wildly (~30↔200) and the GPU spins up while moving/jumping in +town, during streaming, and after portal-hops. A per-frame cost-split probe +(`[FRAME-DIAG]`, `ACDREAM_WB_DIAG=1`) measured the dominant cost directly: + +| Phase | `apply` (med/p95) | **`lockwait` (med/p95)** | in-lock work | +|---|---|---|---| +| Initial world load | 35 / 100 ms | **24 / 88 ms** | ~6 ms | +| Town, streaming | 0.8 / 35 ms | **~0 / 23 ms** | ~1–13 ms | + +`apply` is the per-`OnUpdate` cost of `ApplyLoadedTerrain`. On idle frames +`lockwait ≈ 0` → 200 fps; on streaming frames the update thread blocks +**23–88 ms** acquiring `_datLock`. The terrain GPU upload is 0.05–0.13 ms and +the entity GPU upload ~1 µs, so the prior "GPU-upload churn" theory is +refuted. **The swing is `_datLock` contention.** + +## 2. Root cause (confirmed by source + the code's own comment) + +`DatCollection` is not thread-safe, so a single global `_datLock` +(`GameWindow.cs:164`) serializes **all** `DatCollection.Get` calls. The +streaming worker holds that lock for the **entire** per-landblock build +(`BuildLandblockForStreaming`, `GameWindow.cs:5910`) — "tens of ms worst +case." The update thread's `ApplyLoadedTerrainLocked` (`GameWindow.cs:6704`) +runs under the **same** lock (`ApplyLoadedTerrain`, `:6485`) because it makes +its own `Get` calls — so it blocks until the worker releases. The existing +comment at `GameWindow.cs:5885` already documents this and names the fix: +*"a future pass can reduce contention by pre-building render-thread work on +the worker."* + +The six `Get` sites inside the apply that force it to take the lock: + +| Line | Dat | Purpose | +|---|---|---| +| 6785 | `LandBlockInfo` | cell count + building list | +| 6793 | `EnvCell` (per cell) | indoor physics cell surfaces | +| 6797 | `Environment` (per cell) | cell vertex/polygon geometry | +| ~6885 | `Setup` (per building) | building shell part-0 for `CacheBuilding` | +| 6988 | `GfxObj` (per entity meshRef) | BSP tree cache for collision | +| 7025 | `Setup` (entity parts) | ShadowObjects registration | + +## 3. Goal / non-goals + +**Goal:** `ApplyLoadedTerrainLocked` makes **zero** `DatCollection` calls, so +its `lock(_datLock)` is removed and the update thread never waits on the +worker. Target: `lockwait` median+p95 → ~0; the 30↔200 swing gone. + +**Non-goals (separate work):** the entity-leak / GPU heat-up over hops (H3); +the invisible-char-on-portal bug; making `DatCollection` itself thread-safe; +moving the per-landblock CPU *build* off-thread (that is variant A2 — see §7). + +## 4. Approach (A1, recommended) — pre-read on the worker, drop the apply lock + +The streaming worker already holds `_datLock` for its build and already reads +many of these dats for rendering. Extend the worker's per-landblock output to +carry a **physics/registration dat bundle**: the parsed dat objects the apply +needs, read by the worker under the lock it already holds. The apply then +reads from the bundle instead of calling `_dats.Get`, makes zero +`DatCollection` calls, and its `lock(_datLock)` is deleted. + +Everything else in the apply stays exactly where it is and runs **on the +update thread, lock-free**: +- the terrain GL upload (`AddLandblockWithMesh`) — must stay on the GL thread; +- the CellSurface/PortalPlane **world-space build** (it needs the apply-time + `origin` = `_liveCenter`-relative; building it here keeps it in the correct + frame and sidesteps the trees-in-sky frame-mismatch class); +- all physics/registry mutation (`_physicsEngine.AddLandblock`, + `_physicsDataCache.CacheGfxObj`/`CacheBuilding`, `ShadowObjects.Register`, + `_envCellRenderer.FinalizeLandblock`). + +These touch only update-thread-owned structures; the worker never touches +them. Reading them from the bundle (instead of `Get`) changes *where the bytes +come from*, not *what is computed* — behavior is identical. This is a +threading change, not a behavior change; it is retail-neutral. + +## 5. Why dropping the lock is safe + +`_datLock`'s only cross-thread responsibility is serializing `DatCollection` +access (the streaming worker vs. the update thread). The apply's other +work — GL upload, physics engine, shadow registry, dat-data cache, +EnvCell renderer — is all update-thread-only; the worker (which only runs the +factory closures that read dats) never touches any of it. Therefore, once the +apply makes no `DatCollection` call, `_datLock` is unnecessary around it and +removing it introduces no new race. The live-spawn handlers +(`OnLiveEntitySpawnedLocked`) that also mutate the registry run on the **same** +update thread (via `_liveSessionController.Tick()` in `OnUpdate`), so they +never raced the apply regardless of the lock. + +**Required correctness check (implementation):** the bundled dat objects are +handed by reference from the worker to the update thread. This is safe iff +`DatCollection.Get` returns objects that are immutable after parse (read- +only DBObjs). The plan must verify this; if any returned object is mutated +post-parse, the bundle must carry a copy or the extracted primitive data +instead of the shared instance. + +## 6. Interface / data-flow changes + +- **`LandblockStreamResult.Loaded`** (and `Promoted`) gains a + `PhysicsDatBundle` payload (a small record holding: the `LandBlockInfo`, a + map of `EnvCell`+`Environment` by cell id, building `Setup`s by id, and the + entity `GfxObj`/`Setup`s by id needed for BSP + ShadowObjects). +- The worker's load closure (`BuildLandblockForStreamingLocked` → + `_loadLandblock`) populates the bundle under the lock it already holds, + reusing reads it already performs where possible. +- `ApplyLoadedTerrainLocked(lb, meshData, bundle)` reads every dat from + `bundle.X[id]` instead of `_dats.Get(id)`. The six `Get` sites in §2 + become bundle lookups. +- `ApplyLoadedTerrain` removes its `lock(_datLock) { … }` wrapper. + +The bundle is built once per landblock on the worker; the apply consumes it +once. No new per-frame allocation on the hot path beyond the bundle hand-off. + +## 7. Rejected / deferred alternatives + +- **A2 — also move the CPU build off-thread.** Build cell surfaces in + landblock-local coords on the worker, add `origin` at apply. Saves ~0.6 ms + of update-thread CPU but adds origin-frame handling and more moved code, for + a benefit dwarfed by the eliminated 88 ms wait. Deferred; revisit only if + residual apply CPU matters after A1. +- **Shrink the worker's lock-hold** (read-then-release inside the worker). The + apply still calls `Get`, still takes the lock, still contends. Partial only. +- **Thread-safe `DatCollection`** (concurrent reads). Deepest change, touches + the core dat reader that memory flags as not-thread-safe and risky. Out of + scope. + +## 8. Verification + +- **Empirical (the proof):** re-run the same repro (town walk + portal-hops, + Release, `ACDREAM_WB_DIAG=1`) with the `[FRAME-DIAG]` apparatus still in the + tree. Acceptance: `lockwait` median+p95 drop from ~24/88 ms to ~0, `apply` + collapses toward its in-lock-body cost, and the visible 30↔200 swing is gone + while streaming/portal-hopping. Read clean FPS with the probe off. +- **Unit tests:** the worker bundle is populated for a landblock with cells + + buildings + entities; the apply produces identical physics surfaces / BSP / + ShadowObjects registrations whether fed via `Get` (old path) or via the + bundle (new path) — a golden equivalence test in + `tests/AcDream.Core.Tests/` (or `AcDream.App.Tests` for the wiring). +- **Build + full `dotnet test` green.** + +## 9. Risks + +- *Bundle misses a dat the apply needs* → a `KeyNotFoundException`/null at + apply. Mitigation: the equivalence test + a fallback assert listing the six + known sites; the apply must consume only ids the worker enumerated. +- *Shared mutable dat object* (the §5 check fails) → cross-thread read race. + Mitigation: verify DBObj immutability; copy/extract if not. +- *Removing the lock exposes a previously-masked race* on a structure I have + not enumerated. Mitigation: the §5 argument enumerates every mutation in the + apply; the plan re-audits each against the worker's touch-set before + deleting the lock. + +## 10. Apparatus / rollout + +The `[FRAME-DIAG]` probe (uncommitted: `GameWindow.cs` + `StreamingController.cs`) +stays in the tree through verification to capture before/after, then is +stripped in a final cleanup commit (mirrors the `92e95be` probe-strip pattern). +The fix commit is separate from the apparatus. + +## 11. References + +- `GameWindow.cs:5885` — the worker's lock-hold comment naming this fix. +- `GameWindow.cs:5910` — worker `lock(_datLock)` (the contended hold). +- `GameWindow.cs:6485/6704` — `ApplyLoadedTerrain` / `…Locked` (the waiter). +- `LandblockStreamer.cs:27` — "_datLock serialises all DatCollection.Get". +- `docs/research/2026-06-23-fps-gpu-churn-handoff.md` — the FPS deep-dive. +- Probe results: `frame-diag.log` / `frame-diag2.log` / `frame-diag3.log`.