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 Setups by id, and the entity GfxObj/Setups 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".
• 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.