perf(pathing): pool the StepCache strata buffer, then clean up the pathing engine around it (#2523)

Started as an allocation pass over `StepCache` and grew into a cleanup of the surrounding pathing engine. Four commits, each independently reviewable; net **−560 lines**.

Build clean (0 warnings). All 122 `Server.Tests.Pathfinding` tests pass.

---

## 1. `perf`: pool the strata buffer, cut a hot-path dictionary lookup

**The headline is that `TryGetMask` — the actual hot path — was already allocation-free.** `StepMask` is a readonly struct, `StaticTileEnumerable` is a `ref struct`, `ChunkMissState` is a struct in a `Dictionary`. So most of this is a bake-throughput and GC-churn win, with one exception noted below.

`BuildChunk` accumulated packed multi-Z strata into a `List<byte>` that grew by doubling (256 → 512 → 1024 → …) and then paid a final `ToArray()`. A full map bake runs it ~114k times. It now writes into a `byte[]` rented from `STArrayPool<byte>.Shared` through a span writer, and hands the chunk one exact-size copy.

**This required fixing a latent out-of-bounds guard.** The record-fit check reserved headroom for **8** strata (`StratumByteLength * 8`) while `ComputeStandableSurfaceZs` can return up to **16** — so a cell could write 305 bytes starting from a 65,383-byte offset. Against a `List` that was benign (it just grew past 64 KB, and emitted offsets stayed under the `NoStrata` sentinel). Against a fixed-size rented buffer it is an out-of-bounds write, so tightening it was a *prerequisite* for the pooling, not a drive-by. The guard is now exact, which additionally proves no emitted offset can collide with `NoStrata == ushort.MaxValue`.

**One genuine query-path win:** `ShouldPromoteAfterMiss` did *two* dictionary lookups per miss — a `TryGetValue`, then an indexer assignment that re-hashes and re-probes. It now mutates in place via `CollectionsMarshal.GetValueRefOrNullRef`. This runs on every uncached chunk touch during A* expansion. The window-expiry branch keeps its explicit early return, so `MissPromotionThreshold == 1` still resets rather than promoting.

Also dropped `StepProbe.ComputeStrataAt` / `ComputedStratum` (dead code, zero callers) and collapsed six 18-argument `new StepMask(0, 0, …, kind)` blocks into `Fallthrough(kind)`.

**Considered and rejected:** pooling the `Direction[]` that `Find` returns. It *escapes* the call — `MovementPath` holds it across ticks while `PathFollower` walks `m_Index` through it — so it cannot be rented-and-returned, and it cannot be borrowed from the shared `BitmapAStarAlgorithm.Instance` without one creature clobbering another's in-flight path. `CheckPath` rate-limits repaths to one per 2s per creature, putting this at roughly 60 KB/sec at 1,000 pathing creatures. Not worth a public API break plus a use-after-return footgun.

## 2. `docs`: rewrite the comments for publication

The comments had accumulated as development notes: internal phase jargon (`Tier 4`, `the Phase-2 synthesizer`), change narration aimed at a reviewer (`which the old ComputeStandingZ anchor missed`, `legacy behavior`), benchmark anecdotes (`benchmarked as near-optimal`, `a ~20 ns lookup`), and paragraphs restating the code.

Rewritten to keep the rationale you cannot recover by reading the code — why the source-Z guard cannot be widened, why multis fall through with a halo, why the promotion gate counts Finds rather than calls, why `ComputeFingerprint` must hash the *files* and not the live tile tables — and drop the history that got us there.

Three comments were **factually wrong**, not just wordy:

- `CacheEvictionTimer` and `CacheStats` documented a class called `StaticWalkabilityCache`. No such class exists — it is `StepCache`.
- `StepCacheFile` declared `File layout v8` while `FormatVersion` is 9, and called the current record layout "the v6 layout" in four places. The layout descriptions are now unversioned so they cannot drift again.
- `StepProbe.ComputeStandingZ` claimed `StepCache` uses it to bake `SourceZ`. It has not since the baker moved to the clearance-aware `ComputeStandableSurfaceZs`; only a parity test calls it.

## 3. `refactor`: simplify `StepCacheFile.Write`, consolidate the format tests

`SaveToFile` walked `_keysList` **twice** — once to count the map's chunks, then again through a `ChunkEnumerator` closure to emit them — because `Write` needed the count up front to size its index array. Both loops had the same root cause. Passing a **span** collapses them: the count is just `span.Length`.

That deletes the `ChunkEnumerator` delegate, the closure over the list enumerator, and **both `InvalidOperationException` throws**, which existed only to police the delegate's "yield exactly `chunkCount` chunks" contract — a contract a span makes unrepresentable.

`Write` now patches the header's `IndexOffset` by seeking back to it rather than reaching into the writer's live buffer with `BinaryPrimitives`. That also retires `IndexOffsetFieldPosition`, a hand-maintained byte offset that had to track the header layout, and sidesteps the stale-array hazard that motivated the manual patch (`BufferWriter` reallocates on growth).

**Tests:** `StepCacheFileV6/V7/V8Tests` were named for the format version that introduced each transform — and the format is now **v9**, so all three names described formats the loader rejects outright. Beyond triplicated builders and plumbing, two things were actually broken:

- The three near-identical rejection tests each cited a `MinSupportedVersion` that had since moved (`"version 5 < MinSupportedVersion 6"`, `"6 < 7"`, `"7 < 8"`). They passed for the wrong reason.
- `AssertBaseEqual` (used by V7 and V8) **silently skipped the swim and strata trailers**. A regression dropping either would not have failed those tests.

Now one `StepCacheFileFormatTests`, named for behavior — predictive-Z elision, compression, compact index — with a single `AssertIdentical` that does check both trailers, the three rejection tests folded into one theory that also covers a future version, and a zero-chunk case the delegate-based writer never had coverage for.

## 4. `test`: consolidate the parity and lifecycle tests

Three files tested "parity" and none of the names said *which*. They were three different layers, and the seams are the useful part, so they are now one `StepCacheParityTests` that names them:

| Test | Compares | Answers |
|---|---|---|
| `ProbeMatchesSlowPath` | StepProbe vs MovementImpl | Is the bake right? |
| `CacheMatchesProbe` | StepCache vs StepProbe | Is it stored and returned intact? |
| `CacheServesReachableWalkStates` | StepCache vs MovementImpl | End to end, over the states A* visits |

Merging removed a duplicated stub `Mobile`, duplicated region seeds, and a filename/class mismatch (`StepProbeParityTests.cs` declared `StaticWalkabilityParityTests`). `SwimBake_ProducesWetCells` moved with it — it lived in the cache parity file but never touched the cache.

Tests reached into `StepCache._chunks` via `GetField` in **9 places**, each rebuilding the key encoding and cell-index arithmetic by hand. `StepCache` now exposes `GetResidentChunk` and `ResidentIndexInSync` alongside the internal test hooks it already had (`LazyReaderHasChunk`, `CurrentFindGeneration`), and the shared arithmetic moved to `PathingTestSupport`. All 9 reflection blocks are gone.

`StepCacheLifecycleTests` is regrouped by what it covers — promotion gate, fallthrough routes, strata, swim layer, eviction — with the `Tier4*` names dropped. Removed `Singleton_IsAvailable`, which asserted an inline-initialized static property was not null; that is the entire 123 → 122 test-count delta.

---

## Verification

Tests were mutation-checked rather than just run, since round-trip and parity tests can pass while a transform silently no-ops:

- Injecting an off-by-one into the `IndexOffset` patch fails **15 of 123** — the format tests are load-bearing.
- Offsetting the cache's cell index by one fails **7 of 10** parity cases, and the 3 that stay green are exactly the ones that do not touch the cache. The layering localizes a fault rather than just reporting one.
This commit is contained in:
Kamron Batman 2026-07-12 20:02:29 -07:00 committed by GitHub
parent e035768ef8
commit b852bca41e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
23 changed files with 1663 additions and 2223 deletions

View file

@ -26,12 +26,11 @@ using MoveImpl = Server.Movement.MovementImpl;
namespace Server.PathAlgorithms;
/// <summary>
/// A* pathfinder with a single bitmap-cache lookup per cell expansion. Default walkers
/// take one <see cref="StepCache.TryGetMask"/> call returning the 8-direction
/// mask + per-direction Z. Non-default walkers (non-GM players, creatures with swim/fly/
/// door/clip capabilities) and per-cell cache fallthroughs route through
/// <see cref="GetSuccessorsSlowPath"/>, which runs the per-direction
/// <see cref="CalcMoves.CheckMovement"/> loop for that one cell.
/// A* pathfinder that expands a cell with a single <see cref="StepCache.TryGetMask"/> lookup,
/// which returns all 8 directions' walkability and destination Zs at once. Where the cache can't
/// answer — a fallthrough on that cell, or a flying creature the static cache can't model —
/// <see cref="GetSuccessorsSlowPath"/> runs the per-direction <see cref="CalcMoves.CheckMovement"/>
/// loop for that one cell instead, so a partial cache miss costs only the cells it affects.
/// </summary>
public class BitmapAStarAlgorithm : PathAlgorithm
{
@ -50,61 +49,50 @@ public class BitmapAStarAlgorithm : PathAlgorithm
private const int PlaneOffset = 128;
private const int PlaneCount = 13;
private const int PlaneHeight = 20;
// Default shared singleton (MaxSearchNodes = 1000, set from config in Configure). Typed
// as the concrete class so Configure can set its instance config; assignable anywhere a
// PathAlgorithm is expected. Specialized variants are just additional instances.
// The shared default. A differently-configured variant is just another instance.
public static readonly BitmapAStarAlgorithm Instance = new();
// Scratch buffers — reused across every Find on THIS instance. Per-instance (not static)
// so independently-configured algorithms don't share state. ~320 KB per instance; create
// specialized instances once (static readonly), never per-call. Safe to reuse per Find
// because the game loop is single-threaded and Find is never re-entered.
// Scratch reused across every Find on this instance — roughly 320 KB of it, so create
// instances once and hold them, never per call. Per-instance rather than static so two
// differently-configured algorithms don't share state. Reuse is safe because the game loop is
// single-threaded and Find never re-enters.
private readonly Direction[] _path = new Direction[AreaSize * AreaSize];
private readonly PathNode[] _nodes = new PathNode[NodeCount];
private readonly byte[] _nodeStates = new byte[NodeCount];
private readonly int[] _successors = new int[8];
private readonly PriorityQueue<int, int> _openQueue = new();
// A* node-expansion budget: the search bails (returning null) after this many node
// expansions. Benchmarked as near-optimal: above the ~500 needed to solve walled-off
// indoor routes, below the ~1500 window-exhaustion cost ceiling where a failed
// (unreachable) search's worst-case cost spikes for no solving benefit. Successful
// searches terminate on goal-found, so this never touches the common open-terrain case.
// Per-instance so specialized algorithms (e.g. a wider-budget variant for special NPCs)
// can coexist; the shared default lives on Instance and is set from config in Configure.
// Expansion budget: the search gives up and returns null past this many nodes. It bounds the
// cost of an unreachable goal, which would otherwise exhaust the whole search window. A
// successful search stops when it finds the goal, so the budget only binds on hard or hopeless
// routes — it needs to stay high enough to solve walled-off indoor ones.
public int MaxSearchNodes { get; set; } = 1000;
private int _xOffset;
private int _yOffset;
// When set, GetSuccessors delegates to the per-cell slow path on every expansion
// (creature has CanFly — Z-jumping is beyond the cache's static-only scope).
// Every expansion goes to the slow path: the creature can fly, and arbitrary Z-jumping is
// outside what a static cache can model.
private bool _currentMobileNeedsSlowPath;
// When set, diagonal corner-cut uses the strict AND-rule (BOTH cardinal partners
// must be walkable) instead of the lenient creature OR-rule. Cache still applies —
// partner bits live in the same source-cell mask byte. Non-GM players only.
// Diagonal corner-cut uses the strict rule — both cardinal partners walkable, not just one.
// Non-GM players only. The cache still applies; the partner bits are in the same mask byte.
private bool _currentMobilePlayerStrict;
// Capability overlay applied to cache results. Layered each cell:
// Capability overlay on the cache's two rule sets, applied per cell as
// effective = (walkMask & !cantWalk) | (wetMask & canSwim)
// Reset at end of Find.
private bool _currentMobileCanSwim;
private bool _currentMobileCantWalk;
// Dynamic-obstacle pass capability flags (per-mobile, captured in Find).
// Mirrors MovementImpl.Check's per-mobile derivations so per-cell items/mobiles
// checks can be evaluated without re-deriving.
// Per-mobile flags for the dynamic-obstacle pass, derived once in Find rather than per cell.
private bool _currentMobileIgnoreDoors;
private bool _currentMobileIgnoreSpellFields;
private bool _currentMobileIgnoreMovableImpassables;
public static void Configure()
{
// A* node-expansion budget. Default 1000 is benchmarked near-optimal (see
// MaxSearchNodes). Applied to the shared singleton; specialized instances pass their
// own value. Written back to server.cfg on first boot. Auto-invoked at startup via
// AssemblyHandler.Invoke("Configure").
// Shard-tunable expansion budget for the shared instance; see MaxSearchNodes. Written back
// to server.cfg on first boot so it's discoverable.
Instance.MaxSearchNodes = ServerConfiguration.GetOrUpdateSetting(
"pathfinding.maxSearchNodes",
1000
@ -136,10 +124,8 @@ public class BitmapAStarAlgorithm : PathAlgorithm
return null;
}
// Mark a new Find generation so the StepCache promotion gate counts THIS pathfind
// as one touch per chunk regardless of how many times the expansion frontier
// probes a given chunk. Without this, A* hits each visited chunk dozens of times
// and trips the threshold immediately.
// The frontier probes a given chunk dozens of times over one search; opening a generation
// is what makes the cache's promotion gate count all of that as a single touch.
StepCache.Instance.BeginFindGeneration();
PathfindRecorder.RecordIfEnabled(m, map, start, goal);
@ -161,7 +147,7 @@ public class BitmapAStarAlgorithm : PathAlgorithm
_currentMobileIgnoreMovableImpassables = false;
}
// Mirrors MovementImpl: dead/spectral mobiles also ignore doors.
// Dead and spectral mobiles pass through doors too. Mirrors MovementImpl.
_currentMobileIgnoreDoors |= !m.Alive || m.Body.BodyID == 0x3DB || m.IsDeadBondedPet;
_currentMobileIgnoreSpellFields = m is PlayerMobile && map != Map.Felucca;
@ -209,7 +195,8 @@ public class BitmapAStarAlgorithm : PathAlgorithm
_nodeStates[bestNode] = 2;
// Set MovementImpl globals so per-cell slow-path fallthroughs see the right state.
// MovementImpl reads these statics, so a slow-path fallthrough on any cell below needs
// them set for this mobile.
if (bc != null)
{
MoveImpl.AlwaysIgnoreDoors = bc.CanOpenDoors;
@ -326,11 +313,10 @@ public class BitmapAStarAlgorithm : PathAlgorithm
}
/// <summary>
/// One <see cref="StepCache.TryGetMask"/> call returns the 8-direction
/// walkable mask + destination Zs. Diagonal corner-cut applies the lenient creature
/// OR-rule using partner bits in the same mask byte — no neighbor-chunk lookup needed.
/// On cache fallthrough or for non-default walkers, defers to
/// <see cref="GetSuccessorsSlowPath"/> for THIS cell only.
/// Expands one cell into its walkable neighbours. A single cache lookup covers all 8
/// directions, including the partner bits the diagonal corner-cut needs, so no neighbouring
/// cell has to be consulted. Falls back to <see cref="GetSuccessorsSlowPath"/> for this cell
/// alone when the cache can't answer.
/// </summary>
private int GetSuccessors(int p, Mobile m, Map map)
{
@ -353,16 +339,12 @@ public class BitmapAStarAlgorithm : PathAlgorithm
if (!lookup.IsHit)
{
// Multi-covered cells: synthesize a multi-aware mask in ONE pass (over land + statics +
// house/boat component tiles) instead of the slow path's 8x per-cell CheckMovement.
// Fliers and cache-off already returned at the top of GetSuccessors, so this only runs
// for cacheable walkers/swimmers. The synthesized mask flows through the SAME
// capability-overlay + diagonal corner-cut + dynamic-obstacle loop below as a static hit.
// A multi-covered cell still gets a whole-cell mask, synthesized over the house or boat
// components rather than looked up. That keeps it out of the slow path's 8 separate
// CheckMovement calls, and the result flows through the same overlay, corner-cut and
// dynamic-obstacle logic below as a cache hit would.
if (lookup.HitKind == CacheHitKind.Fallthrough_Multi)
{
// Multi-covered cell: the per-multiID interior cache serves a ~20 ns lookup for
// interior cells (and records the right counter); it falls back internally to the
// Phase-2 live synthesizer for perimeter / terrain-dirty / foundation cells.
lookup = MultiMaskCache.Instance.GetMask(map, p3D.X, p3D.Y, (sbyte)p3D.Z);
}
else
@ -371,8 +353,8 @@ public class BitmapAStarAlgorithm : PathAlgorithm
}
}
// Capability overlay: walking allowed unless cantWalk; swimming allowed if canSwim.
// Partner bits used for diagonal corner-cut also use the effective mask.
// Overlay the mobile's capabilities onto the cache's two rule sets. The corner-cut below
// reads its partner bits from this effective mask, not the raw one.
var walkBits = _currentMobileCantWalk ? (byte)0 : lookup.WalkMask;
var swimBits = _currentMobileCanSwim ? lookup.WetMask : (byte)0;
var mask = (byte)(walkBits | swimBits);
@ -393,9 +375,8 @@ public class BitmapAStarAlgorithm : PathAlgorithm
continue;
}
// Diagonal corner-cut. Creatures (default): OR-rule — at least one cardinal
// partner walkable. Non-GM players: AND-rule — BOTH partners must be walkable.
// Partner bits live in the same source-cell mask byte either way.
// Diagonal corner-cut: a creature needs at least one of the two flanking cardinals to
// be walkable, a non-GM player needs both.
if ((i & 1) == 1)
{
var leftBit = 1 << ((i - 1) & 0x7);
@ -408,9 +389,9 @@ public class BitmapAStarAlgorithm : PathAlgorithm
}
}
// Walking takes precedence over swimming when both apply (matches MovementImpl's
// surface-selection: closest-to-startZ wins, and walk surface is always closer
// when the creature is currently standing on land).
// Walking wins over swimming where both are possible. MovementImpl picks the surface
// closest to the start Z, and for a creature standing on land that is always the walk
// surface.
var useWalkZ = (walkBits & (1 << i)) != 0;
var z = useWalkZ
? i switch
@ -441,8 +422,8 @@ public class BitmapAStarAlgorithm : PathAlgorithm
var absX = x + _xOffset;
var absY = y + _yOffset;
// Dynamic-obstacle pass: items + mobiles at the target cell. Cache only
// covers static walkability; dynamic state has to be checked at query time.
// The cache only knows static terrain, so items and mobiles at the target cell have to
// be checked live.
if (IsBlockedByDynamic(m, map, absX, absY, z))
{
continue;
@ -464,11 +445,9 @@ public class BitmapAStarAlgorithm : PathAlgorithm
private const int MobileHeight = 15;
/// <summary>
/// Mirrors MovementImpl's dynamic-item / mobile collision phase for a target cell.
/// Items: ImpassableSurface that overlap (z, z+PersonHeight), respecting capability
/// overrides (CanOpenDoors → ignore door items; CanMoveOverObstacles → ignore movables;
/// non-Felucca players → ignore spell fields). Mobiles: any other mobile whose Z range
/// overlaps and which we can't move over.
/// MovementImpl's item and mobile collision phase for one target cell: impassable items
/// overlapping the mobile's vertical envelope block it, subject to the capability overrides,
/// as does any other mobile it can't move over.
/// </summary>
private bool IsBlockedByDynamic(Mobile m, Map map, int x, int y, int z)
{
@ -509,9 +488,9 @@ public class BitmapAStarAlgorithm : PathAlgorithm
}
}
// A* must be able to plan a path to the goal cell even when the target mobile is
// standing on it (the follower stops within range short of it). Skip the mob-block
// check at the goal cell ONLY; everywhere else dynamic mobiles still block.
// The goal cell is usually occupied by whatever the mobile is chasing, so blocking on it
// would fail every pursuit. The follower stops short of the goal anyway. Every other cell
// still blocks on mobiles.
var skipMobCheck = x == MoveImpl.Goal.X && y == MoveImpl.Goal.Y;
if (!skipMobCheck)
@ -534,18 +513,16 @@ public class BitmapAStarAlgorithm : PathAlgorithm
}
/// <summary>
/// Mirrors MovementImpl.CanMoveOver — true when m can step onto t's cell (dead bodies,
/// hidden staff, etc.).
/// True when m can step onto t's cell — a corpse, hidden staff, and so on. Mirrors
/// MovementImpl.CanMoveOver.
/// </summary>
private static bool CanMoveOver(Mobile m, Mobile t) =>
!t.Alive || !m.Alive || t.IsDeadBondedPet || m.IsDeadBondedPet
|| t.Hidden && t.AccessLevel > AccessLevel.Player;
/// <summary>
/// Per-direction <see cref="CalcMoves.CheckMovement"/> loop for a single source cell.
/// Runs on cache fallthrough or when <see cref="_currentMobileNeedsSlowPath"/> is set.
/// CheckMovement validates land/statics/items via MovementImpl; dynamic mobile blocking
/// is layered on top because MovementImpl doesn't iterate same-cell mobiles.
/// Expands one cell the long way, with a CheckMovement call per direction. The same-cell
/// mobile check is layered on top because MovementImpl doesn't iterate those.
/// </summary>
private int GetSuccessorsSlowPath(Mobile m, Map map, int px, int py, Point3D p3D, int[] vals)
{
@ -586,11 +563,10 @@ public class BitmapAStarAlgorithm : PathAlgorithm
}
/// <summary>
/// True for creatures whose movement rules the static cache can't model. Currently
/// only CanFly — flying creatures Z-jump arbitrarily and the cache's source-Z guard
/// would over-fire. CanSwim / CantWalk are handled via the capability overlay (walkMask
/// + wetMask). CanOpenDoors / CanMoveOverObstacles only affect dynamic items and don't
/// disqualify the cache.
/// True for creatures the static cache can't model at all. Only flying ones qualify: they
/// Z-jump freely, so the cache's source-Z guard would reject nearly every cell anyway. Swim
/// and cant-walk are handled by the capability overlay, and the door / obstacle capabilities
/// only affect dynamic items, so none of those disqualify the cache.
/// </summary>
private static bool RequiresSlowPath(Mobile m) => m is BaseCreature bc && bc.CanFly;
}