docs: datLock-contention FPS fix design spec
The 30↔200 FPS swing is _datLock contention: the streaming worker holds the global dat lock for the full per-landblock build (lockwait measured 24ms median / 88ms p95), stalling the update thread's ApplyLoadedTerrain. Fix (A1): the worker pre-reads the apply's six Get<T> sites into a physics dat bundle so ApplyLoadedTerrainLocked makes zero DatCollection calls and its lock(_datLock) is removed. Approved design; implementation next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Erik
parent
92e95bea53
commit
c715e55937
1 changed files with 177 additions and 0 deletions
|
|
@ -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<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`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue