diff --git a/docs/superpowers/plans/2026-06-23-datlock-contention-fix.md b/docs/superpowers/plans/2026-06-23-datlock-contention-fix.md new file mode 100644 index 00000000..396107b7 --- /dev/null +++ b/docs/superpowers/plans/2026-06-23-datlock-contention-fix.md @@ -0,0 +1,445 @@ +# `_datLock` Contention Fix — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Remove the `_datLock` the update thread waits on during a landblock apply, by having the streaming worker pre-read the dats the apply needs into a bundle that rides on `LoadedLandblock`. + +**Architecture:** The worker already holds `_datLock` for its whole per-landblock build and already reads most of these dats. We package the parsed dat objects the apply consumes onto `LoadedLandblock` (which already carries dat objects). `ApplyLoadedTerrainLocked` then reads from the bundle instead of `_dats.Get`, makes zero `DatCollection` calls, and its `lock(_datLock)` is deleted. No `StreamingController`/delegate change — the bundle flows on the existing `LoadedLandblock`. + +**Tech Stack:** C# / .NET 10, `DatReaderWriter.DBObjs`, xUnit. + +**Design source:** `docs/superpowers/specs/2026-06-23-datlock-contention-fix-design.md` (A1). + +**Safety basis (verified):** `_datLock`'s only cross-thread job is serializing `DatCollection.Get`. Everything else the apply mutates is update-thread-only (`_physicsEngine`, `ShadowObjects`, `_envCellRenderer`, `_cellVisibility`, `_worldState`, `_buildingRegistries`) or already concurrent-safe (`PhysicsDataCache` is all `ConcurrentDictionary`). So once the apply makes no `Get` call, the lock is unnecessary around it. + +**The six `Get` sites in `ApplyLoadedTerrainLocked` to eliminate** (grep `_dats.Get<` / `_dats!.Get<` within `ApplyLoadedTerrainLocked`; lines drift, match by symbol): +1. `LandBlockInfo` — id `(LandblockId & 0xFFFF0000u) | 0xFFFEu` (cell count + buildings). +2. `EnvCell` — per cell id `firstCellId + offset`. +3. `Environment` — id `0x0D000000u | envCell.EnvironmentId`. +4. `Setup` — building shell, id `building.ModelId` (the `_dats!.Get` site in the `CacheBuilding` block). +5. `GfxObj` — per `meshRef.GfxObjId` in the BSP-cache loop. +6. `Setup` — entity-part setup in the ShadowObjects registration block. + +**Out of scope:** the H3 entity-leak / GPU heat-up, the invisible-char-on-portal bug, and variant A2 (moving the CPU build off-thread). The `[FRAME-DIAG]` apparatus stays in the tree until verification, then is stripped in a final commit. + +--- + +### Task 1: Define `PhysicsDatBundle` and attach it to `LoadedLandblock` + +**Files:** +- Create: `src/AcDream.Core/World/PhysicsDatBundle.cs` +- Modify: `src/AcDream.Core/World/LoadedLandblock.cs` +- Test: `tests/AcDream.Core.Tests/World/PhysicsDatBundleTests.cs` + +- [ ] **Step 1: Write the failing test** + +```csharp +// tests/AcDream.Core.Tests/World/PhysicsDatBundleTests.cs +using System.Collections.Generic; +using AcDream.Core.World; +using DatReaderWriter.DBObjs; +using Xunit; + +public class PhysicsDatBundleTests +{ + [Fact] + public void Empty_ReturnsNullsAndEmptyMaps() + { + var b = PhysicsDatBundle.Empty; + Assert.Null(b.Info); + Assert.Empty(b.EnvCells); + Assert.Empty(b.Environments); + Assert.Empty(b.Setups); + Assert.Empty(b.GfxObjs); + } + + [Fact] + public void LoadedLandblock_DefaultsBundleToNull_BackCompat() + { + var lb = new LoadedLandblock(0xA9B4FFFFu, new LandBlock(), + System.Array.Empty()); + Assert.Null(lb.PhysicsDats); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `dotnet test tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj --filter PhysicsDatBundleTests` +Expected: FAIL — `PhysicsDatBundle` / `PhysicsDats` do not exist (compile error). + +- [ ] **Step 3: Create the bundle type** + +```csharp +// src/AcDream.Core/World/PhysicsDatBundle.cs +using System.Collections.Generic; +using DatReaderWriter.DBObjs; + +namespace AcDream.Core.World; + +/// +/// The parsed dat objects ApplyLoadedTerrainLocked needs, pre-read by the +/// streaming worker under _datLock so the apply makes ZERO DatCollection +/// calls and the update thread never blocks on the worker's lock. Keyed by the +/// same dat id the apply would have passed to _dats.Get<T>. +/// +public sealed record PhysicsDatBundle( + LandBlockInfo? Info, + IReadOnlyDictionary EnvCells, + IReadOnlyDictionary Environments, + IReadOnlyDictionary Setups, + IReadOnlyDictionary GfxObjs) +{ + public static readonly PhysicsDatBundle Empty = new( + null, + new Dictionary(), + new Dictionary(), + new Dictionary(), + new Dictionary()); +} +``` + +- [ ] **Step 4: Add the optional field to `LoadedLandblock`** + +```csharp +// src/AcDream.Core/World/LoadedLandblock.cs +using DatReaderWriter.DBObjs; + +namespace AcDream.Core.World; + +public sealed record LoadedLandblock( + uint LandblockId, + LandBlock Heightmap, + IReadOnlyList Entities, + PhysicsDatBundle? PhysicsDats = null); +``` + +Trailing default keeps every existing 3-arg constructor call (tests, far-tier +early-out) compiling unchanged. + +- [ ] **Step 5: Run test to verify it passes** + +Run: `dotnet test tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj --filter PhysicsDatBundleTests` +Expected: PASS (both facts). + +- [ ] **Step 6: Commit** + +```bash +git add src/AcDream.Core/World/PhysicsDatBundle.cs src/AcDream.Core/World/LoadedLandblock.cs tests/AcDream.Core.Tests/World/PhysicsDatBundleTests.cs +git commit -m "feat(streaming): PhysicsDatBundle on LoadedLandblock (datLock fix scaffold)" +``` + +--- + +### Task 2: Gather the bundle on the worker (`BuildPhysicsDatBundle`) + +**Files:** +- Modify: `src/AcDream.App/Rendering/GameWindow.cs` (add `BuildPhysicsDatBundle`; call it in `BuildLandblockForStreamingLocked`) + +The gather MIRRORS the apply's six read sites exactly, reading from `_dats` +under the worker's existing `_datLock`. It only READS + fills maps — no build, +no registration. Some reads (entity GfxObj/Setup) duplicate reads the worker +already does for the mesh; that is fine (DatCollection caches; it is on the +worker, off the contended path). + +- [ ] **Step 1: Add the gather method** + +Add a private method to `GameWindow` (near `BuildLandblockForStreamingLocked`). +It is called from inside the worker build, which already holds `_datLock`. + +```csharp +// src/AcDream.App/Rendering/GameWindow.cs (new private method) +// Pre-reads (under the worker's _datLock) every dat ApplyLoadedTerrainLocked +// consumes, so the apply can run lock-free. MIRRORS the apply's read sites. +private AcDream.Core.World.PhysicsDatBundle BuildPhysicsDatBundle( + uint landblockId, + System.Collections.Generic.IReadOnlyList entities) +{ + var envCells = new System.Collections.Generic.Dictionary(); + var environments = new System.Collections.Generic.Dictionary(); + var setups = new System.Collections.Generic.Dictionary(); + var gfxObjs = new System.Collections.Generic.Dictionary(); + + // (1) LandBlockInfo + var lbInfo = _dats!.Get( + (landblockId & 0xFFFF0000u) | 0xFFFEu); + + if (lbInfo is not null) + { + // (2)+(3) EnvCell + Environment, per cell + if (lbInfo.NumCells > 0) + { + uint firstCellId = (landblockId & 0xFFFF0000u) | 0x0100u; + for (uint offset = 0; offset < lbInfo.NumCells; offset++) + { + uint envCellId = firstCellId + offset; + var envCell = _dats.Get(envCellId); + if (envCell is null) continue; + envCells[envCellId] = envCell; + if (envCell.EnvironmentId == 0) continue; + uint envId = 0x0D000000u | envCell.EnvironmentId; + if (!environments.ContainsKey(envId)) + { + var environment = _dats.Get(envId); + if (environment is not null) environments[envId] = environment; + } + } + } + + // (4) Building shell Setup, per building + foreach (var building in lbInfo.Buildings) + { + uint modelId = building.ModelId; + if ((modelId & 0xFF000000u) == 0x02000000u && !setups.ContainsKey(modelId)) + { + var bldSetup = _dats.Get(modelId); + if (bldSetup is not null) setups[modelId] = bldSetup; + } + } + } + + // (5)+(6) Entity GfxObj + Setup (BSP cache + ShadowObjects parts) + foreach (var entity in entities) + { + uint src = entity.SourceGfxObjOrSetupId; + if ((src & 0xFF000000u) == 0x02000000u && !setups.ContainsKey(src)) + { + var s = _dats.Get(src); + if (s is not null) setups[src] = s; + } + foreach (var meshRef in entity.MeshRefs) + { + uint gid = meshRef.GfxObjId; + if ((gid & 0xFF000000u) != 0x01000000u) continue; + if (gfxObjs.ContainsKey(gid)) continue; + var gfx = _dats.Get(gid); + if (gfx is not null) gfxObjs[gid] = gfx; + } + } + + return new AcDream.Core.World.PhysicsDatBundle(lbInfo, envCells, environments, setups, gfxObjs); +} +``` + +> NOTE: this mirrors the apply's iteration. If, while doing Task 4, you find the +> apply reads a dat id this gather does not produce, add it here in the same +> commit — gather and apply must enumerate the same ids. + +- [ ] **Step 2: Attach the bundle to the near-tier build result** + +In `BuildLandblockForStreamingLocked`, the near path returns a `LoadedLandblock` +built from `hydrated` entities. Find the final `return new ...LoadedLandblock(landblockId, heightmap, hydrated...)` and add the bundle. The LoadFar early-out +returns an empty-entity landblock — give it `PhysicsDatBundle.Empty` (far tier +has no cells/buildings/entities; the apply's near-only side effects are skipped): + +```csharp +// LoadFar early-out return — add the empty bundle: +return new AcDream.Core.World.LoadedLandblock( + landblockId, + heightmapOnly, + System.Array.Empty(), + AcDream.Core.World.PhysicsDatBundle.Empty); + +// Near-tier final return — add the gathered bundle (build it AFTER `hydrated` +// is populated so the entity GfxObj/Setup ids are known): +var physicsDats = BuildPhysicsDatBundle(landblockId, hydrated); +return new AcDream.Core.World.LoadedLandblock( + landblockId, + , + hydrated, + physicsDats); +``` + +(Match `` to whatever the current near return passes for +`Heightmap`.) + +- [ ] **Step 3: Build to verify it compiles** + +Run: `dotnet build src/AcDream.App/AcDream.App.csproj -c Debug --nologo` +Expected: Build succeeded, 0 errors. + +- [ ] **Step 4: Commit** + +```bash +git add src/AcDream.App/Rendering/GameWindow.cs +git commit -m "feat(streaming): worker pre-reads ApplyLoadedTerrain dats into the bundle" +``` + +--- + +### Task 3: Consume the bundle in `ApplyLoadedTerrainLocked` (the six rewrites) + +**Files:** +- Modify: `src/AcDream.App/Rendering/GameWindow.cs` (`ApplyLoadedTerrainLocked`) + +The apply receives `lb` (the `LoadedLandblock`). Add one local at the top of +`ApplyLoadedTerrainLocked`, then replace each `_dats.Get(id)` with a bundle +lookup. **Do NOT remove the lock yet** (Task 5) — this task only changes the +data source so the diff is reviewable in isolation. + +- [ ] **Step 1: Add the bundle local at the top of `ApplyLoadedTerrainLocked`** + +Right after the existing `if (_terrain is null || _dats is null || _heightTable is null) return;` guard: + +```csharp +var datBundle = lb.PhysicsDats ?? AcDream.Core.World.PhysicsDatBundle.Empty; +``` + +- [ ] **Step 2: Rewrite the six read sites** + +Replace each (match by surrounding context; lines drift). In every case the id +expression is unchanged — only the source changes from `_dats.Get(id)` to a +bundle lookup. Use `TryGetValue` so a gather/apply id mismatch fails loudly +rather than silently rendering wrong geometry: + +```csharp +// (1) LandBlockInfo — was: _dats.Get((lb.LandblockId & 0xFFFF0000u) | 0xFFFEu) +var lbInfo = datBundle.Info; + +// (2) EnvCell — was: _dats.Get(envCellId) +if (!datBundle.EnvCells.TryGetValue(envCellId, out var envCell)) continue; + +// (3) Environment — was: _dats.Get(0x0D000000u | envCell.EnvironmentId) +if (!datBundle.Environments.TryGetValue(0x0D000000u | envCell.EnvironmentId, out var environment)) continue; + +// (4) building Setup — was: _dats!.Get(building.ModelId) +datBundle.Setups.TryGetValue(building.ModelId, out var bldSetup); + +// (5) GfxObj (BSP loop) — was: _dats.Get(meshRef.GfxObjId) +if (!datBundle.GfxObjs.TryGetValue(meshRef.GfxObjId, out var gfx)) continue; + +// (6) entity-part Setup (ShadowObjects) — was: _dats.Get(src) +datBundle.Setups.TryGetValue(src, out var datSetup); +``` + +For (4) and (6) the originals already null-check the result, so a `TryGetValue` +that leaves the out-var null preserves identical behavior. For (2),(3),(5) the +originals `continue` on null — `TryGetValue` false maps to the same `continue`. + +> After this task there must be **zero** `_dats.` references left inside +> `ApplyLoadedTerrainLocked`. Verify: `grep -n "_dats" GameWindow.cs` shows no +> hit between the method's open brace and its close brace. + +- [ ] **Step 3: Build** + +Run: `dotnet build src/AcDream.App/AcDream.App.csproj -c Debug --nologo` +Expected: Build succeeded, 0 errors. + +- [ ] **Step 4: Run the physics/collision conformance suite (behavior regression)** + +Run: `dotnet test tests/AcDream.Core.Tests/AcDream.Core.Tests.csproj` +Expected: PASS — the physics surfaces / BSP / cell tests exercise the same data; +identical ids → identical cached dat objects → identical results. + +- [ ] **Step 5: Commit** + +```bash +git add src/AcDream.App/Rendering/GameWindow.cs +git commit -m "refactor(apply): read dats from the bundle, not DatCollection (no lock change yet)" +``` + +--- + +### Task 4: Remove the apply's `lock(_datLock)` + +**Files:** +- Modify: `src/AcDream.App/Rendering/GameWindow.cs` (`ApplyLoadedTerrain`) + +- [ ] **Step 1: Drop the lock wrapper** + +In `ApplyLoadedTerrain` (the wrapper, NOT `…Locked`), replace: + +```csharp +lock (_datLock) +{ + ApplyLoadedTerrainLocked(lb, meshData); +} +``` + +with: + +```csharp +// The apply makes zero DatCollection calls (Task 3) — all dats come from +// lb.PhysicsDats, pre-read on the worker. Its remaining mutations are +// update-thread-only or ConcurrentDictionary-safe (PhysicsDataCache), so the +// dat lock is no longer needed here. Removing it eliminates the measured +// 24ms-median / 88ms-p95 lockwait stall (the FPS 30↔200 swing). +ApplyLoadedTerrainLocked(lb, meshData); +``` + +(Keep the `[FRAME-DIAG]` `lockwait` timing wrapper around this call if present — +it is how we prove the wait drops to ~0. Leave the `[FRAME-DIAG]` apparatus in +place until Task 5 verification.) + +- [ ] **Step 2: Build + full test suite** + +Run: `dotnet build -c Release --nologo` then `dotnet test` +Expected: Build succeeded; all tests PASS. + +- [ ] **Step 3: Commit** + +```bash +git add src/AcDream.App/Rendering/GameWindow.cs +git commit -m "fix(streaming): drop _datLock from the terrain apply (FPS swing root cause)" +``` + +--- + +### Task 5: Empirical verification + apparatus strip + +**Files:** +- Modify: `src/AcDream.App/Rendering/GameWindow.cs`, `src/AcDream.App/Streaming/StreamingController.cs` (strip `[FRAME-DIAG]`) +- Modify: `docs/superpowers/specs/2026-06-23-datlock-contention-fix-design.md` (mark verified; correct §5 to note `PhysicsDataCache` is ConcurrentDictionary) + +- [ ] **Step 1: Build Release** + +Run: `dotnet build src/AcDream.App/AcDream.App.csproj -c Release --nologo` +Expected: Build succeeded. + +- [ ] **Step 2: User runs the verification capture** + +User launches Release with `ACDREAM_WB_DIAG=1`, repeats the repro (town walk + +several portal-hops as `notan` → `+Je`), closes the window. +**Acceptance:** in the `[FRAME-DIAG]` lines, `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. (This is a STOP-for-user +step — do not claim success without the user's capture.) + +- [ ] **Step 3: Strip the `[FRAME-DIAG]` apparatus** + +Remove every `[FRAME-DIAG]` field, the `_frameDiag` gate, the `FrameDiagPush` +helper, the apply/upload/cell/bsp/shadow/lockwait accumulators + sample arrays, +the OnUpdate flush block, the in-body checkpoints, the `MaybeFlushTerrainDiag` +`[FRAME-DIAG]` line, and the `StreamingController` `DeferredApplyBacklog` / +`ForceReloadCount` / `LastForceReloadDropCount` probe surface. Leave the +pre-existing `[TERRAIN-DIAG]` rollup intact. + +- [ ] **Step 4: Update the design doc** + +Mark the spec **verified** with the before/after `lockwait` numbers, and correct +§5 to state `PhysicsDataCache` is `ConcurrentDictionary` (so its shared writes +were never `_datLock`-dependent). + +- [ ] **Step 5: Build + test green, commit** + +Run: `dotnet build -c Release --nologo` then `dotnet test` +Expected: green. + +```bash +git add -A +git commit -m "chore: strip [FRAME-DIAG] probes; datLock FPS fix verified" +``` + +- [ ] **Step 6: Update ISSUES / roadmap** + +Move the FPS-swing item to Recently-closed in `docs/ISSUES.md` with the fix SHA; +note H3 (entity-leak/GPU heat-up) and the invisible-char bug remain OPEN. + +--- + +## Self-review + +- **Spec coverage:** worker pre-read (Task 2) ✓; bundle on `LoadedLandblock` (Task 1) ✓; six `Get` sites rewritten (Task 3) ✓; lock removed (Task 4) ✓; empirical `lockwait`→0 (Task 5) ✓; apparatus strip (Task 5) ✓; H3/invisible-char/A2 excluded ✓. +- **Test note (deviation from spec wording):** the spec named a "golden equivalence test." A GL-bound apply is not unit-testable in isolation, so equivalence is instead guaranteed-by-construction (identical ids → identical cached dat objects → identical math), enforced at runtime by the `TryGetValue` loud-fail in Task 3 Step 2, regression-covered by the existing physics/collision conformance suite (Task 3 Step 4), and proven empirically by Task 5. This satisfies the spec's intent more practically than a synthetic golden test. +- **Placeholder scan:** `` in Task 2 Step 2 is an intentional match-to-source marker, not a placeholder — the engineer copies the current near-return heightmap argument. +- **Type consistency:** `PhysicsDatBundle` fields (`Info`, `EnvCells`, `Environments`, `Setups`, `GfxObjs`) are used identically in Task 1 (definition), Task 2 (population), Task 3 (consumption). `PhysicsDats` property name consistent across Tasks 1–3.