acdream/docs/superpowers/specs/2026-06-23-datlock-contention-fix-design.md

# 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<T>` 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<T>` 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<T>`, 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<T>` 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<X>(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<T>".
- `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`.