From 048d252b4cc071a08a3f21d6da0b4efd139a4d5a Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 09:48:51 -0500 Subject: [PATCH 01/39] docs: parser improvement plan Ordered correctness+performance plan for the Source 2 replay parser, derived from a subsystem review against the Clarity (Java) reference parser. Executed one goal per commit on jcoene/goal-perf. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 421 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 421 insertions(+) create mode 100644 IMPROVEMENTS.md diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md new file mode 100644 index 0000000..7486896 --- /dev/null +++ b/IMPROVEMENTS.md @@ -0,0 +1,421 @@ +# Manta parser — correctness & performance improvement plan + +Borrowed from the Clarity (Java) reference parser at `../clarity`. Executed on branch +`jcoene/goal-perf`, one goal per commit, in `/goal` mode. + +This plan was produced by a 14-agent review (7 subsystem analyses, each adversarially +verified against both codebases with real CPU/alloc profiles of `replays/2159568145.dem`). +27 findings confirmed, 19 adjusted, 1 rejected. + +## Ground rules + +- **Strictly no public API change.** Internals only — no new exported symbols, no changed + signatures. The de-box work stays behind the existing `Entity.Get`/`GetFloat32` (box lazily + on read). The internal `fieldDecoder` type is unexported and is fair game. +- **One optimization per commit.** Each commit message carries its `benchstat` table vs the + previous commit + `go test ./...` PASS. The branch history is the story. +- **Commits only — do NOT open a PR, do NOT push.** Make local commits on `jcoene/goal-perf` and + report the cumulative results here for review. The user opens the PR. +- **Sequence:** safe high-ROI wins first, re-baseline, then the invasive typed-state rewrite. + +## Verification protocol (per goal) + +- **Correctness gate (non-negotiable):** `go test ./...` stays green with **identical** golden + values (manta_test.go runs ~48 real replays asserting exact `expectEntityEvents` counts, + `expectHeroEntityMana` floats, combat-log counts). A perf change that alters output is a regression. +- **Perf measure:** `go test -run=XXX -bench= -benchmem -benchtime=10x -count=10` before + and after; compare with `benchstat` (`go install golang.org/x/perf/cmd/benchstat@latest`). + Report `sec/op`, `B/op`, **`allocs/op`** (the near-deterministic leading indicator). +- **Profile confirmation:** `make memprofile` / `make cpuprofile` — confirm the *targeted* site + actually moved, not just the aggregate. +- **Canonical bench replay (held fixed for the whole branch):** **`8552595443`** (build 6601 — most + representative of current real-world replays). The review's profile evidence below was gathered on + `2159568145`; re-capture the alloc profile on `8552595443` at P0 to confirm the top sources before + optimizing (relative signal is expected to hold). + +### Profile evidence (gathered on `replays/2159568145.dem`, M4 Pro; canonical bench going forward is `8552595443`) +- ~11.0M allocs/op, ~315 MB/op. +- Streaming `*os.File`: 798 ms/op · `bufio`: 658 ms/op · in-memory `bytes.Reader`: 644 ms/op. +- Top alloc sources: `readFieldPaths` slice 56–60% · quantized-float box ~10–12% · QAngle + `[]float32` ~4.5–5.5% · signed/unsigned int box ~2–3% · `*pendingMessage` ~5% · `*outerMessage` ~5%. + +--- + +## Goal summary + +| ID | Goal | Type | Risk | Effort | Expected impact | +|----|------|------|------|--------|-----------------| +| **P0** | In-memory benchmark rig + recent-replay bench | infra | none | small | makes parse wins measurable (removes ~80% syscall noise) | +| **P1.1** | `pendingMessage` value-slice + typed `sort.Stable` (reuse buffer) | perf | low | small | −0.54M allocs, −20 MB; +determinism | +| **P1.2** | `outerMessage` by value | perf | low | small | −0.56M allocs, −17 MB | +| **P1.3** | snappy scratch-buffer reuse | perf | med | small | −9 MB/op | +| **P1.4** | modifier emit: early-return when no handler | perf | low | small | removes modifier unmarshal from hot path | +| **P1.5** | reuse per-packet `tuples` + entity-data reader | perf | low | small | −0.44M allocs | +| **P1.6** | hoist per-entity `fpCache`/`fpNoop` maps to class | perf | low | small | −2 maps/entity; smaller Entity | +| **P1.7** | hoist `v(6)` debug guard out of per-field loop | perf | low | small | −33M branch/calls (CPU) | +| **P1.8** | buffer `stream` IO (bufio when not byte-reader) | perf | low | small | 798→658 ms streaming path | +| **P2.1** | **string-table additive index fix** (real bug) | correctness | low | small | fixes 4,469 wrong indices | +| **P2.2** | string-table: fail loud, don't swallow panics | correctness | low | small | surfaces silent corruption | +| **P2.3** | `getEventKey` off-by-one (`>` → `>=`) | correctness | low | small | prevents panic on skewed events | +| **P2.4** | modifier: skip empty/deleted entries | correctness | low | small | no spurious zero events | +| **P2.5** | combat-log `Type()/TypeName()/String()` descriptor-driven | correctness | low | small | latent landmine fix | +| **P2.6** | decoder forward-compat additions (never-occur types) | correctness | low | small | CUtlBinaryBlock/Quaternion/int64/etc | +| **P2.7** | HSequence / HeroID_t decode parity (value-changing) | correctness | med | small | matches clarity; suite-gated | +| **P2.8** | BloodType fixed-8 decode (risky, suite-gated) | correctness | med | small | only if all goldens stay green | +| **P2.9** | QAngle precise/32/0-bit forward-compat | correctness | low | small | future builds | +| **P2.10** | mana/runetime patch sentinel guards | correctness | low | small | robustness/parity | +| **P2.11** | outer-message size sanity bound | correctness | low | small | OOM hardening on corrupt input | +| **P2.12** | field-path depth-7 guard + comment | correctness | low | small | loud failure vs cryptic panic | +| **P1.9** | **reusable field-path buffer** (value-type, no pool/copy/slice) | perf | med | medium | **−56–60% of all allocs** | +| **P1.10** | word-at-a-time bit reader | perf | med | small | top CPU win (after P0) | +| **P1.11** | varint + `readByte`/`readLeUintX` from accumulator | perf | low | small | compounds P1.10 | +| **P1.12** | flatten huffman tree to int arrays (no dispatch) | perf | low | medium | removes per-bit interface calls | +| **P1.13** | 8-bit field-op lookup table | perf | med | medium | resolves majority of ops in 1 index | +| **P1.14** | decode class baseline once, clone per entity | perf | med | medium | baseline re-decode is ~4× update decode | +| **P3.1** | **typed entity state (eliminate boxing)** | perf | med | large | ~20% of allocs (float+qangle+int) | +| — | string-table Items: map → dense slice | perf | med | medium | locality; depends on P2.1 | +| — | integrate `CDemoStringTables` full dumps | correctness | med | medium | seek/robustness; depends on P2.1 | +| **PARK** | combat-log name-resolution helper / `CombatLogEntry` | correctness | — | — | **blocked: new exported API** | +| **PARK** | S2 HLTV `CMsgDOTACombatLogEntry` path | correctness | — | — | **blocked: new exported API** | +| **PARK** | VTProtobuf zero-reflection unmarshal | perf | med | large | envelope decode only; large effort | + +Ordering note: P1.9–P1.14 are the bigger structural wins; they're listed after the trivial +P1.1–P1.8 alloc trims so we build a clean measured baseline first, exactly as agreed. + +--- + +## P0 — In-memory benchmark rig (foundational, no parser logic) + +**Why:** the current `BenchmarkMatch` (manta_test.go:609) streams from `*os.File` inside the timed +loop with no `b.ResetTimer()`; the review measured ~80% of CPU in per-byte-read syscalls (798 ms vs +644 ms in-memory). Parse optimizations are invisible under that noise, and `B/op` is polluted by IO. + +**Change:** add `BenchmarkMatch8552595443` (build 6601) as the canonical bench, and rewrite `bench()` to +read the replay into `[]byte` once before the loop, `b.ResetTimer()`, and parse via in-memory +`NewParser(buf)` in the loop. Point the Makefile `memprofile`/`cpuprofile` targets at `8552595443` too. +No parser code touched → cannot affect correctness. + +**Verify:** `go test ./...` unchanged; capture the P0 baseline (`sec/op`, `B/op`, `allocs/op`) and a fresh +`make memprofile` on `8552595443` — this is the reference all later commits compare against, and it +reconfirms the top alloc sources (expect `readFieldPaths` to dominate). + +--- + +## Phase 1 — safe perf wins + +### P1.1 — `pendingMessage` value-slice + typed `sort.Stable` +- **Now:** `onCDemoPacket` builds `[]*pendingMessage`, allocating a pointer per embedded message + (~5.4M/run), then `sort.Sort` (non-stable) every packet (demo_packet.go:62-89). +- **Change:** make `pendingMessages` a `[]pendingMessage` value slice reusing a parser-level + `pendingMsgBuf[:0]`; change `priority()` to a value receiver; use **typed `sort.Stable`** (not + `sort.SliceStable`, which adds ~1.46M reflection allocs). Store the buffer back on both the error + and success return paths. +- **Clarity:** dispatches embedded messages in file order, no buffering (InputSourceProcessor.java:198-240). +- **Caveat:** manta deliberately reorders across priority buckets — `sort.Stable` gives + *file-order-within-bucket* determinism; do **not** describe it as full clarity parity, and do **not** + remove the priority sort (would change dispatch order and break golden counts). +- **Impact (measured):** 11.0M→10.47M allocs, 315→295 MB. **Reentrancy verified safe:** CDemoPacket is + only dispatched via `callByDemoType`, never nested in `callByPacketType`. +- _This is the change a review agent prototyped + reverted; reimplement cleanly here with a benchmark._ +- **Result:** _(pending)_ + +### P1.2 — `outerMessage` by value +- **Now:** `readOuterMessage` heap-allocates `&outerMessage{}` per message (parser.go:238); single + consumer is `Start()` (parser.go:142), value never retained past the iteration. +- **Change:** return by value (or reuse a `p.outerMsg` field); update the `var msg *outerMessage` decl. +- **Impact (measured):** −557K allocs, −17 MB. Zero risk. +- **Result:** _(pending)_ + +### P1.3 — snappy scratch-buffer reuse +- **Now:** `snappy.Decode(nil, buf)` per compressed outer message (parser.go:232) allocates fresh. +- **Change:** pass `p.snappyScratch[:cap(...)]`; snappy@v0.0.3 reuses dst when `dLen <= len(dst)`. +- **Risk (keep medium):** the decompressed buffer becomes `msg.data` passed to handlers. If any + CDemo* handler retains a subslice across messages, reuse corrupts it. **The full `go test` suite is + the gate** — do not rely on assertion that string tables copy; verify empirically. +- **Impact (measured):** −9 MB/op. +- **Result:** _(pending)_ + +### P1.4 — modifier emit early-return when no handler +- **Now:** `emitModifierTableEvents` (modifier.go:18) allocates + unmarshals a proto per ActiveModifiers + item per snapshot/delta even when `p.modifierTableEntryHandlers` is empty (the bench registers none). +- **Change:** `if len(p.modifierTableEntryHandlers) == 0 { return nil }` at the top of + `emitModifierTableEvents` (covers both call sites string_table.go:126,171). Also switch + `proto.NewBuffer(v).Unmarshal` → `proto.Unmarshal` so a reused msg is safe (Buffer.Unmarshal merges + without reset; only safe on fresh msg). +- **Impact:** removes all modifier unmarshal from the bench hot path. Verified ActiveModifiers-only + (never touches instancebaseline). +- **Result:** _(pending)_ + +### P1.5 — reuse per-packet `tuples` + entity-data reader +- **Now:** `onCSVCMsg_PacketEntities` allocates `newReader(m.GetEntityData())` and + `tuples := make([]tuple, 0, updates)` per packet (entity.go:223,244). +- **Change:** parser-level `tuples[:0]` reset each call (preserves append order → preserves the + handlers-outer/tuples-inner emission order `expectEntityEvents` depends on); `reader.reset(buf)` + method for the entity-data reader. +- **Impact:** −0.44M allocs. (The baseline reader at entity.go:269 is handled by P1.14, not here.) +- **Result:** _(pending)_ + +### P1.6 — hoist per-entity `fpCache`/`fpNoop` maps to class +- **Now:** `newEntity` allocates two maps per entity (entity.go:69-70); name→fieldPath depends only on + `class.serializer`, so every entity of a class recomputes/re-stores the identical mapping. +- **Change:** move both to a single shared map on `*class` (single-goroutine parse → plain map, no + sync.Map). The class-level `*fieldPath` is read-only/never released (matches today's retain-on-hit). +- **Clarity:** resolves name→FieldPath on the DTClass, Entity holds only state (Entity.java:88-114). +- **Impact:** −2 maps/entity + smaller Entity struct. Golden-safe (resolved fieldPath is class-invariant; + `expectPlayer6Name`/`expectHeroEntityName` guard it). +- **Result:** _(pending)_ + +### P1.7 — hoist `v(6)` debug guard out of the per-field loop +- **Now:** `readFields` calls `v(6)` twice per field path in the hot loop (field_reader.go:13,23). +- **Change:** evaluate `dbg := v(6)` once at the top of `readFields` (per call — **not** higher; debug + tick mutates `debugLevel` mid-parse and `readFields` is called fresh per entity-update, so per-call + re-eval preserves behavior). +- **Impact:** −~33M calls/branches (CPU only, ~0 alloc when disabled). +- **Result:** _(pending)_ + +### P1.8 — buffer `stream` IO +- **Now:** `stream.readByte`→`readBytes(1)`→`io.ReadFull` per byte; 3 varints + payload per outer + message (stream.go, parser.go:198-227). +- **Change:** in `newStream`, wrap with `bufio.NewReaderSize` **only** when the reader is not already an + `io.ByteReader` (`*os.File` isn't; `*bytes.Reader` is → `NewParser` stays unwrapped, no double-buffer). +- **Impact:** 798→658 ms on the streaming path. **Note:** the canonical in-memory bench (P0) won't show + this — it's a real-world streaming-path win; verify with a streaming-reader bench variant. +- **Result:** _(pending)_ + +### P1.9 — reusable field-path buffer ⭐ biggest single win +- **Now:** `readFieldPaths` (field_path.go:309-337) starts `paths := []*fieldPath{}` (cap 0) and + `append(paths, fp.copy())` per op; this slice growth is **56–60% of all allocations** (17M objects/op, + 309 MB), independently confirmed by two agents. `fp.copy()` itself allocs ~0 (sync.Pool). +- **Change (do the three coupled findings together):** (a) stop materializing a fresh `[]*fieldPath` per + call — reuse a buffer reset to len 0 each call; (b) make `fieldPath` carry a fixed `[7]int` value array + so a "copy" is a cheap value store, not a pool Get/Put; (c) since `readFields` immediately iterates + + releases, consider fusing decode inline (ops apply in stream order; decoder lookup depends only on the + current fp value, so interleaving = resolve-all-then-decode — verified equivalent). +- **Plumbing:** `readFieldPaths(r)` / `readFields(r,s,state)` are free functions with no Parser handle — + thread a buffer param or reuse the existing `fpPool` sync.Pool (already concurrency-safe; manta runs + independent Parsers, so a package-global mutable buffer is **unsafe**). Keep `*fieldPath` for the cold + `getFieldPaths()`/`Entity.Map()` paths. +- **Clarity:** one reused `S2ModifiableFieldPath` writing immutable snapshots into a fixed + `fieldPaths[MAX_PROPERTIES=0x3FFF]` (FieldReader.java:11-14, S2FieldReader.java:50-65). +- **Impact:** allocs/op ~11M → ~4–5M. **Keep the fixed depth 7** (`[7]int`). Golden-safe (only the + container changes; field-path values + decode order unchanged). +- **Result:** _(pending)_ + +### P1.10 — word-at-a-time bit reader +- **Now:** `readBits` refills the accumulator one byte at a time (reader.go:50-61) with a per-byte + bounds-check+panic in `nextByte`. `readBits` is the #1 CPU consumer. +- **Change:** refill a full `uint64` at once: + `r.bitVal |= binary.LittleEndian.Uint64(r.buf[r.pos:]) << r.bitCount; free := (64 - r.bitCount) >> 3; + r.pos += free; r.bitCount += free*8`, keeping a byte-loop for the final ≤8 bytes (protobuf-owned + buffers have unknown trailing capacity → tail guard `r.pos+8 <= r.size`). +- **Correction to the naive formula:** advance is `(64 - bitCount) >> 3` whole bytes, **not** + `8 - bitCount/8` (bitCount transiently reaches ~33–39 after a refill). +- **Safety invariant (empirically verified):** max `n` observed across real replays is exactly 32 + (`readBits(qfd.Bitcount)`, `noscaleDecoder` readBits(32), `readUBitVarFP` readBits(31)), so one uint64 + load always yields ≥32 valid bits. A 5,000-trial fuzz of byte-vs-word produced bit-identical output. +- **Add a build-time assert** that quantized `Bitcount <= 32` (quantizedfloat.go integer-encode loop can + raise it) so a pathological field can't overflow the accumulator. +- **Impact:** large CPU win (only visible after P0). 0 allocs delta. +- **Result:** _(pending)_ + +### P1.11 — varints + `readByte`/`readLeUintX` from the accumulator +- **Now:** `readVarUint32/64` call `readByte` per byte (byte-aligned fast path → `nextByte`), each a + separate bounds-check (reader.go:102-140). +- **Change:** after P1.10, route `readByte` through `readBits(8)` so varint bytes share the loaded word; + fold the now-dead `bitCount==0` fast path; read `readLeUint32/64`/`readFloat` straight from the + accumulator instead of `binary.LittleEndian.UintXX(readBytes(N))` (the unaligned `readBytes` path + allocates). +- **Impact:** compounds P1.10. `BenchmarkReadVarUint32/64` (reader_test.go) measures it directly. +- **Guard:** preserve `readBytes` zero-copy aliasing (P-guard below). +- **Result:** _(pending)_ + +### P1.12 — flatten huffman tree to int arrays +- **Now:** `readFieldPaths` walks an interface pointer-tree per bit (`node.Right()/Left()/IsLeaf()/Value()` + — ~2–3 interface dispatches/bit, huffman.go:9-77, field_path.go:316-332). +- **Change:** flatten to parallel int arrays `tree[node][bit]` (negative = `-ordinal-1` leaf), walked with + no dispatch. **Build from manta's OWN `huffTree`** (manta's tie-break differs structurally from + clarity's; codes were verified bit-for-bit identical when built from manta's table — porting clarity's + numbering would desync). +- **Impact:** removes per-bit interface calls. 0 allocs. **Add a permanent decoder-equivalence test** + (interface-walk vs flat path → identical ordinal + bits-consumed across all 256 prefixes). +- **Result:** _(pending)_ + +### P1.13 — 8-bit field-op lookup table +- **Now:** even flattened, each op is a 3–8 iteration bit-walk. +- **Change:** precompute a 256-entry table (`bits0-7` = consumed count, `bits8-15` = op ordinal or + fall-back node) resolving most ops in one index (clarity FieldOpHuffmanTree.java:19-46, + BitStream64.java:53-84). Depends on P1.12. +- **Safety-critical correction:** manta's reader is a **consuming** stream — the peek must read only + available bits and **zero-pad** the rest, never over-read (`FieldPathEncodeFinish` = '10', 2 bits, fires + near buffer end and would panic `nextByte` past `r.size`). Add a `reader.peekBits(n)` primitive and + unit-test it at the <8-bits-remaining boundary. Also assert `huffTree` is never mutated + (`swapNodes`/`addNode`) after the table is built. +- **Note:** the "99.7%" is clarity's runtime figure; manta's static weights resolve ~98.4% within ≤8 bits + (28/40 ops have >8-bit codes, max 17) — still a large majority. Don't cite 99.7% as verified. +- **Result:** _(pending)_ + +### P1.14 — decode class baseline once, clone per entity +- **Now:** every entity CREATE re-parses raw baseline bytes from scratch + (`readFields(newReader(baseline), …)`, entity.go:269); profiled at ~2.05% — **~4× the cost of the + actual update decode** and more than `newEntity` itself. +- **Change:** decode each class's baseline once into a template state; clone into new entities. **Without + COW** (manta has none today) the clone is a recursive deep-copy of the `*fieldState` tree (must + faithfully clone nested `*fieldState` — 10 type-assert sites rely on leaf-vs-`*fieldState`; mutating one + entity must never corrupt the template/siblings; reproduce the `set` rule that won't overwrite a slot + holding a `*fieldState`). +- **Clarity:** caches decoded baseline per class, `getBaseline().copy()` with copy-on-write + (Entities.java:655-676, NestedArrayEntityState.java:28-43,219-224). +- **Impact:** net win = clone cheaper than re-decode (it is, especially after P3.1 enables cheap COW). + Golden-critical: cloned+overlaid state must be value-identical to today's fresh decode. +- **Result:** _(pending)_ + +### P-guard — zero-copy `readBytes` invariant (correctness guard, do alongside P1.10/P1.11) +- `readBytes` aligned path returns `r.buf[r.pos-n:r.pos]` aliasing the protobuf buffer (reader.go:81); + `demo_packet.go` stores these into `pendingMessage` and parses them **later**, and sendtable/string + reads depend on the aliasing. A clarity-style padded-copy reader would corrupt this. Add an explicit + invariant + test that byte-aligned `readBytes` stays zero-copy after the word-reader rewrite. + +--- + +## Phase 2 — correctness (independent; can interleave with Phase 1) + +### P2.1 — string-table additive index fix ⭐ real latent bug +- **Now:** non-increment branch computes **absolute** `index = readVarUint32() + 1` (string_table.go:226). +- **Change:** **additive** `index += readVarUint32() + 2` (clarity S2StringTableEmitter.java:91; matches + manta's own entity decoder entity.go:247). +- **Evidence (experiment):** on the bench replay the additive scheme is strictly monotonic within every + blob; the current absolute scheme produces 2,309 impossible non-increasing steps and 4,469 divergent + indices (all in ActiveModifiers; e.g. manta=11 where correct=50). Invisible to goldens only because no + test resolves a delta-updated table by index. Applying the one-line fix: `TestParseStringTable*` and 5 + golden `TestMatch*` all pass with identical values. Init stays `-1`; do **not** switch to `readUBitVar`. +- **Result:** _(pending)_ + +### P2.2 — string-table: fail loud +- `parseStringTable` defers `recover()` and returns partial `items` silently (string_table.go:181-186); + callers see success and apply a half-populated table. Propagate the error (callbacks already return + `error`). `recover` never fires on the bench replay → no healthy-replay regression. Add a truncated-blob + unit test. (Also reconcile the inconsistency: `UpdateStringTable` `_panicf`s on a missing table id at + string_table.go:139 — harsher than clarity, which skips unknown ids.) +- **Result:** _(pending)_ + +### P2.3 — `getEventKey` off-by-one +- `if f.i > len(e.m.GetKeys())` lets `f.i == len` pass → `GetKeys()[f.i]` panics (game_event.go:166). + Change to `>=`. One char; zero golden risk (well-formed descriptors never hit it). +- **Result:** _(pending)_ + +### P2.4 — modifier: skip empty/deleted entries +- `emitModifierTableEvents` unmarshals every item including empty (`Value == []byte{}`), raising all-zero + messages to handlers. Add `if len(item.Value) == 0 { continue }` (modifier.go:19), matching clarity + `if (value != null)`. A real new modifier is never zero-length on the wire. +- **Result:** _(pending)_ + +### P2.5 — combat-log `Type()/TypeName()/String()` descriptor-driven +- These hardcode `keys[0].GetValByte()` (game_event.go:37,41,46), bypassing the descriptor field map. Resolve + `type` via the descriptor (clarity S1CombatLogIndices.java:8) and route through `GetInt32`-style dispatch + (not raw `GetValByte`). On current descriptors `type` is index 0 → output unchanged; these methods aren't + called by any manta source/test → zero golden risk. **Fix all three** (the review noted `String()` too). +- **Result:** _(pending)_ + +### P2.6 — decoder forward-compat additions (never-occur today → golden-neutral) +Verified zero occurrences across all 39 build replays, so safe insurance: +- `CUtlBinaryBlock`: no decoder → falls to varint (would desync). Add `n:=readVarUint32(); readBytes(n)` + (clarity CUtlBinaryBlockDecoder). +- `Quaternion`: add to **`fieldTypeFactories`** as `vectorFactory(4)` (128 bits; not `fieldTypeDecoders`). +- `int64`: maps to 32-bit `readVarInt32` (truncates); add `signed64Decoder → readVarInt64` (exists). Note: + changes stored dynamic type int32→int64 — a `.(int32)` consumer would break (forward-compat, not asserted). +- `ResourceId_t → unsigned64Decoder`, `CGlobalSymbol → stringDecoder`, `HSequence → readVarUint32()-1` + (see P2.7 for the value-changing nuance). **Drop** `CBaseVRHandAttachmentHandle` (already correct — no-op). +- Place each in the correct map; note `findDecoderByBaseType` (variable-array childDecoder) consults only + `fieldTypeDecoders`, not factories. +- **Result:** _(pending)_ + +### P2.7 — HSequence / HeroID_t decode parity (value-changing, suite-gated) +- `HSequence` is REAL (644k decodes): manta stores `value`, clarity `value-1` → off-by-one on every + HSequence field today. `HeroID_t` REAL (1536× on builds 6600/6601): clarity uses signed varint (same bits, + different value for negative ids). Both change consumer-visible output but no golden asserts them. Decide + storage signedness (HSequence `value-1` underflows if stored unsigned at value 0). **Run the full suite + incl. 6600/6601 replays; accept only if all green.** +- **Result:** _(pending)_ + +### P2.8 — BloodType fixed-8 decode (risky) +- `m_nBloodType` (109k× on builds 6600/6601, **in the golden suite**) currently a varint; clarity reads a + fixed 8-bit. Identical bits only when value < 128; if any ≥127, widths differ → desync → broken goldens. + **Suite-gated:** apply only if all goldens (esp. 6600/6601) stay green; otherwise drop. +- **Result:** _(pending)_ + +### P2.9 — QAngle precise/32/0-bit forward-compat +- `qangle_pitch_yaw` doesn't handle bitCount ∈ {0,32} (raw float); no `qangle_precise` (20-bit) handling → + would fall to the coord path and desync. Zero occurrences on the corpus (observed bitcounts {0,8,13}) → + pure forward-compat. Add the clarity special-cases (QAnglePitchYawOnly/Precise/NoScale decoders). +- **Result:** _(pending)_ + +### P2.10 — mana/runetime patch sentinel guards +- Mana (builds ≤954) and simtime/runetime (all builds) patches apply unconditionally (field_patch.go:51-78); + clarity guards on the sentinel `±3.4028235e38` bounds. Adding the guards is a no-op on the corpus + (robustness/parity). **Keep manta's bespoke 4-bit `runeTimeDecoder`** (Outlanders case) and the `/30` + simtime API — guards only; touching the runetime decode math risks goldens. +- **Result:** _(pending)_ + +### P2.11 — outer-message size sanity bound +- `readOuterMessage` passes the size varint straight to `stream.readBytes` which does `make([]byte, n)` with + no bound (parser.go:219, stream.go:26) → a corrupt/huge varint can OOM before `io.ReadFull` errors. Add a + max-size guard (safely above the largest legitimate full packet) returning an error. Golden-neutral. +- **Result:** _(pending)_ + +### P2.12 — field-path depth-7 guard + comment +- `Push*` ops do `fp.last++` then index `fp.path[fp.last]` with no guard → depth>6 panics with a raw + out-of-range (field_path.go). Clarity bounds at 7 and fails loudly (S2LongFieldPathFormat.java:7-58). Keep + 7, add a cheap descriptive guard + comment. No behavior change (nothing exceeds 7 today). **Must stay `[7]int` + when P1.9 lands.** +- **Result:** _(pending)_ + +--- + +## Phase 3 — invasive (after Phase 1 re-baseline) + +### P3.1 — typed entity state (eliminate per-field boxing) ⭐ +- **Now:** `fieldState.state` is `[]interface{}`; every decoder returns `interface{}` (field_decoder.go:7). + In Go, boxing float32 / `[]float32` / large uint64 / int32 (>255) into `interface{}` **always** heap-allocates, + on the hot write path. Measured: quantized float box ~10–12%, QAngle `[]float32` ~4.5–5.5%, signed int box + ~2%, unsigned int box — together **~20% of all allocs**. +- **Change (internal only):** store scalars unboxed — e.g. a tagged-union cell `{kind uint8; f float32; + i uint64; ref interface{}}` (strings/sub-state use `ref`); decoders write into the typed lane; box **lazily** + in `Entity.Get`/`GetFloat32` (reads are rare vs writes). For vectors, store per-element float32 cells in the + nested `fieldState` and reassemble the slice on `Get`. +- **Scope reality (verifier):** `*fieldState` shares the same `[]interface{}` slots as leaf values, distinguished + by `.(*fieldState)` in 10 sites (field.go, field_state.go) plus `Map()/getFieldPaths` — all must be reworked. + This is **not** a clarity mirror (clarity's `Object[]` also boxes; the JVM just amortizes via TLAB/escape + analysis) — it's a manta-specific optimization. risk medium, effort large. +- **Golden-critical:** `Get("m_flMaxMana").(float32)` (manta_test.go:694) must stay bit-identical — `Get`/ + `GetFloat32` must still return a boxed `float32`. Folds in P-decoder findings (quantized/qangle/int boxing) + which deliver 0 allocs alone and explicitly depend on this. Also unlocks cheap COW for P1.14. +- **Result:** _(pending)_ + +--- + +## Deferred / parked + +- **string-table Items map → dense slice** (perf, depends P2.1): indices become dense after the additive fix; + replicate clarity's `setValueForIndex(index Date: Fri, 19 Jun 2026 09:57:54 -0500 Subject: [PATCH 02/39] perf(bench): in-memory benchmark rig + recent canonical replay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The benchmark streamed from *os.File inside the timed loop, so ~80% of CPU was per-byte read syscalls, masking parser changes. Read the replay into memory once, ResetTimer, and parse via in-memory NewParser in the loop. Add BenchmarkMatch8552595443 (build 6601, more representative of current replays) as the canonical bench and repoint the Makefile profile targets to it. No parser code changed; go test ./... green (identical golden values). Baseline (8552595443, M4 Pro, -benchmem -benchtime=10x -count=10): sec/op 1.523 ± 1% | B/op 791.5Mi | allocs/op 20.75M Alloc profile reconfirms readFieldPaths dominates at 56.6% of allocations. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 6 +++++- Makefile | 6 +++--- manta_test.go | 22 +++++++++++++++++----- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 7486896..c16953d 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -99,6 +99,10 @@ No parser code touched → cannot affect correctness. `make memprofile` on `8552595443` — this is the reference all later commits compare against, and it reconfirms the top alloc sources (expect `readFieldPaths` to dominate). +**Result:** baseline **1.523 s/op ±1%, 791.5 MiB/op, 20.75M allocs/op** (8552595443, M4 Pro, 10x×10). +Alloc profile confirms `readFieldPaths` #1 at **56.6%**, then quantized-float box 5.4%, `onCDemoPacket` +pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2%. + --- ## Phase 1 — safe perf wins @@ -418,4 +422,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | After goal | sec/op | B/op | allocs/op | go test | |------------|--------|------|-----------|---------| -| P0 baseline | _tbd_ | _tbd_ | _tbd_ | PASS | +| P0 baseline | 1.523 s ±1% | 791.5 MiB | 20.75M | PASS | diff --git a/Makefile b/Makefile index 8a57e7d..1acb0c6 100644 --- a/Makefile +++ b/Makefile @@ -7,19 +7,19 @@ testnew: go test -cover -run=TestMatchNew -v bench: - go test -run=XXX -bench=BenchmarkMatch -benchtime=1m -v + go test -run=XXX -bench=BenchmarkMatch8552595443 -benchmem -benchtime=10x -count=10 -v cover: go test -cover -coverpkg github.com/dotabuff/manta,github.com/dotabuff/manta/vbkv -coverprofile /tmp/manta.cov -v go tool cover -html=/tmp/manta.cov cpuprofile: - go test -v -run=TestMatch2159568145 -test.cpuprofile=/tmp/manta.cpuprof + go test -v -run=TestMatchNew8552595443 -test.cpuprofile=/tmp/manta.cpuprof go tool pprof -svg -output=/tmp/manta.cpuprof.svg manta.test /tmp/manta.cpuprof open /tmp/manta.cpuprof.svg memprofile: - go test -v -run=TestMatch2159568145 -test.memprofile=/tmp/manta.memprof -test.memprofilerate=1 + go test -v -run=TestMatchNew8552595443 -test.memprofile=/tmp/manta.memprof -test.memprofilerate=1 go tool pprof --alloc_space manta.test /tmp/manta.memprof update: update-protobufs generate diff --git a/manta_test.go b/manta_test.go index ded9ef9..636511c 100644 --- a/manta_test.go +++ b/manta_test.go @@ -1,6 +1,7 @@ package manta import ( + "io" "os" "testing" @@ -9,6 +10,7 @@ import ( ) func BenchmarkMatch2159568145(b *testing.B) { testScenarios[2159568145].bench(b) } +func BenchmarkMatch8552595443(b *testing.B) { testScenarios[8552595443].bench(b) } // Test client func TestMatchNew8552595443(t *testing.T) { testScenarios[8552595443].test(t) } @@ -607,10 +609,22 @@ var testScenarios = map[int64]testScenario{ } func (s testScenario) bench(b *testing.B) { - for n := 0; n < b.N; n++ { - r := mustGetReplayReader(s.matchId, s.replayUrl) + // Read the replay fully into memory once so the benchmark measures parsing + // (CPU + allocations), not per-byte file IO. The streaming os.File path + // spends ~80% of CPU in read syscalls, which would otherwise mask the + // parser changes we are trying to measure. + rc := mustGetReplayReader(s.matchId, s.replayUrl) + buf, err := io.ReadAll(rc) + rc.Close() + if err != nil { + b.Fatalf("unable to read replay: %s", err) + } + + b.ReportAllocs() + b.ResetTimer() - parser, err := NewStreamParser(r) + for n := 0; n < b.N; n++ { + parser, err := NewParser(buf) if err != nil { b.Fatalf("unable to instantiate parser: %s", err) } @@ -624,8 +638,6 @@ func (s testScenario) bench(b *testing.B) { b.Fatal(err) } } - - b.ReportAllocs() } func (s testScenario) test(t *testing.T) { From f96810767720ef6c212697973d814f5993c3016f Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:03:06 -0500 Subject: [PATCH 03/39] perf(packet): reuse pendingMessage value-slice, stable sort onCDemoPacket allocated a *pendingMessage per embedded message (~0.7M allocs/parse) and sorted via sort.Sort's reflection path. Switch pendingMessages to a value slice reusing a parser-level buffer, give priority() a value receiver, and use typed sort.Stable (deterministic file-order within each priority bucket, no reflection allocs). The priority reordering is preserved; onCDemoPacket is non-re-entrant so the reused backing array is safe. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P0: sec/op 1.523 -> 1.514 -0.62% (p=0.000) B/op 791.5Mi -> 763.8Mi -3.51% allocs/op 20.75M -> 20.06M -3.33% Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- demo_packet.go | 32 ++++++++++++++++++++------------ parser.go | 1 + 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index c16953d..b972fdf 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -121,7 +121,8 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 - **Impact (measured):** 11.0M→10.47M allocs, 315→295 MB. **Reentrancy verified safe:** CDemoPacket is only dispatched via `callByDemoType`, never nested in `callByPacketType`. - _This is the change a review agent prototyped + reverted; reimplement cleanly here with a benchmark._ -- **Result:** _(pending)_ +- **Result:** sec/op 1.523→1.514 (−0.62%, p=0.000), B/op 791.5→763.8 MiB (−3.51%), allocs/op + 20.75M→20.06M (−3.33%, −0.69M). go test green. ✅ ### P1.2 — `outerMessage` by value - **Now:** `readOuterMessage` heap-allocates `&outerMessage{}` per message (parser.go:238); single @@ -423,3 +424,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | After goal | sec/op | B/op | allocs/op | go test | |------------|--------|------|-----------|---------| | P0 baseline | 1.523 s ±1% | 791.5 MiB | 20.75M | PASS | +| P1.1 pendingMessage value-slice | 1.514 s (−0.6%) | 763.8 MiB (−3.5%) | 20.06M (−3.3%) | PASS | diff --git a/demo_packet.go b/demo_packet.go index a53dc79..72d3733 100644 --- a/demo_packet.go +++ b/demo_packet.go @@ -14,7 +14,7 @@ type pendingMessage struct { } // Calculates the priority of the message. Lower is more important. -func (m *pendingMessage) priority() int { +func (m pendingMessage) priority() int { switch m.t { case // These messages provide context needed for the rest of the tick @@ -42,7 +42,7 @@ func (m *pendingMessage) priority() int { } // Provides a sortable structure for storing messages in the same packet. -type pendingMessages []*pendingMessage +type pendingMessages []pendingMessage func (ms pendingMessages) Len() int { return len(ms) } func (ms pendingMessages) Swap(i, j int) { ms[i], ms[j] = ms[j], ms[i] } @@ -60,9 +60,12 @@ func (ms pendingMessages) Less(i, j int) bool { // multiple inner packets from a single CDemoPacket. This is the main structure // that contains all other data types in the demo file. func (p *Parser) onCDemoPacket(m *dota.CDemoPacket) error { - // Create a slice to store pending mesages. Messages are read first as - // pending messages then sorted before dispatch. - ms := make(pendingMessages, 0, 2) + // Reuse a parser-level buffer to store pending messages. Messages are read + // first as pending messages then sorted before dispatch. onCDemoPacket is + // never re-entrant (it is dispatched only via callByDemoType, never nested + // within a callByPacketType call), so a single reused backing array is safe + // and avoids a heap allocation per embedded message. + ms := p.pendingMsgBuf[:0] // Read all messages from the buffer. Messages are packed serially as // {type, size, data}. We keep reading until until less than a byte remains. @@ -71,20 +74,25 @@ func (p *Parser) onCDemoPacket(m *dota.CDemoPacket) error { t := int32(r.readUBitVar()) size := r.readVarUint32() buf := r.readBytes(size) - ms = append(ms, &pendingMessage{p.Tick, t, buf}) + ms = append(ms, pendingMessage{p.Tick, t, buf}) } // Sort messages to ensure dependencies are met. For example, we need to - // process string tables before game events that may reference them. - sort.Sort(ms) - - // Dispatch messages in order, returning on handler error. - for _, m := range ms { - if err := p.Callbacks.callByPacketType(m.t, m.buf); err != nil { + // process string tables before game events that may reference them. A + // stable sort keeps equal-priority messages in their original file order + // and avoids the reflection allocations of sort.Sort's interface path. + sort.Stable(ms) + + // Dispatch messages in order, returning on handler error. Store the + // (possibly grown) backing array back on the parser on every exit path. + for i := range ms { + if err := p.Callbacks.callByPacketType(ms[i].t, ms[i].buf); err != nil { + p.pendingMsgBuf = ms return err } } + p.pendingMsgBuf = ms return nil } diff --git a/parser.go b/parser.go index 75ea78d..dafdb4c 100644 --- a/parser.go +++ b/parser.go @@ -45,6 +45,7 @@ type Parser struct { isStopping bool modifierTableEntryHandlers []ModifierTableEntryHandler serializers map[string]*serializer + pendingMsgBuf pendingMessages stream *stream stringTables *stringTables stopAtTick uint32 From 638122a336beed65e0560226cc8bc2a616ab1fff Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:07:57 -0500 Subject: [PATCH 04/39] perf(parser): return outerMessage by value readOuterMessage allocated &outerMessage{} per outer message and returned it as a pointer, forcing a heap escape. Its only caller (Start) consumes the message immediately and never retains it, so return it by value to keep it on the stack. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.1: allocs/op 20.06M -> 20.03M -0.19% (-30K) B/op 763.8Mi -> 762.6Mi -0.15% sec/op 1.514 -> 1.519 +0.32% (within run-to-run noise) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 ++++- parser.go | 23 ++++++++++++----------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index b972fdf..5742792 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -129,7 +129,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 consumer is `Start()` (parser.go:142), value never retained past the iteration. - **Change:** return by value (or reuse a `p.outerMsg` field); update the `var msg *outerMessage` decl. - **Impact (measured):** −557K allocs, −17 MB. Zero risk. -- **Result:** _(pending)_ +- **Result:** allocs/op 20.06M→20.03M (−0.19%, −30K), B/op −0.15%; sec/op +0.32% (run-to-run thermal + noise, +5 ms abs). Smaller than the −557K review estimate — the 70 MB replay has far fewer outer + messages than 2159568145. go test green. ✅ ### P1.3 — snappy scratch-buffer reuse - **Now:** `snappy.Decode(nil, buf)` per compressed outer message (parser.go:232) allocates fresh. @@ -425,3 +427,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: |------------|--------|------|-----------|---------| | P0 baseline | 1.523 s ±1% | 791.5 MiB | 20.75M | PASS | | P1.1 pendingMessage value-slice | 1.514 s (−0.6%) | 763.8 MiB (−3.5%) | 20.06M (−3.3%) | PASS | +| P1.2 outerMessage by value | 1.519 s (−0.3%) | 762.6 MiB (−3.7%) | 20.03M (−3.5%) | PASS | diff --git a/parser.go b/parser.go index dafdb4c..bebfdf7 100644 --- a/parser.go +++ b/parser.go @@ -118,7 +118,7 @@ func NewStreamParser(r io.Reader) (*Parser, error) { // Start parsing the replay. Will stop processing new events after Stop() is called. func (p *Parser) Start() (err error) { - var msg *outerMessage + var msg outerMessage defer p.afterStop() @@ -191,14 +191,16 @@ type outerMessage struct { data []byte } -// Read the next outer message from the buffer. -func (p *Parser) readOuterMessage() (*outerMessage, error) { +// Read the next outer message from the buffer. The message is returned by +// value so it does not escape to the heap (its single caller, Start, consumes +// it immediately and never retains it). +func (p *Parser) readOuterMessage() (outerMessage, error) { // Read a command header, which includes both the message type // well as a flag to determine whether or not whether or not the // message is compressed with snappy. command, err := p.stream.readCommand() if err != nil { - return nil, err + return outerMessage{}, err } // Extract the type and compressed flag out of the command @@ -208,7 +210,7 @@ func (p *Parser) readOuterMessage() (*outerMessage, error) { // Read the tick that the message corresponds with. tick, err := p.stream.readVarUint32() if err != nil { - return nil, err + return outerMessage{}, err } // This appears to actually be an int32, where a -1 means pre-game. @@ -219,29 +221,28 @@ func (p *Parser) readOuterMessage() (*outerMessage, error) { // Read the size and following buffer. size, err := p.stream.readVarUint32() if err != nil { - return nil, err + return outerMessage{}, err } buf, err := p.stream.readBytes(size) if err != nil { - return nil, err + return outerMessage{}, err } // If the buffer is compressed, decompress it with snappy. if msgCompressed { var err error if buf, err = snappy.Decode(nil, buf); err != nil { - return nil, err + return outerMessage{}, err } } // Return the message - msg := &outerMessage{ + return outerMessage{ tick: tick, typeId: msgType, data: buf, - } - return msg, nil + }, nil } // parseToTick configures this Parser to stop once it has parsed the given tick. From 1b00c341828010687f9f7dc400b80c3ebddc7065 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:12:15 -0500 Subject: [PATCH 05/39] perf(parser): reuse snappy scratch buffer for outer messages snappy.Decode(nil, ...) allocated a fresh output buffer for every compressed outer message. Pass a reused parser-level scratch buffer as the destination; snappy reuses it when large enough, amortizing decompression to ~the largest compressed message. Safe because the decoded buffer is consumed within this message's dispatch and never retained across outer messages. go test ./... green, including the full 48-replay golden suite (the gate for this reuse-aliasing change). benchstat (8552595443, 10x x10) vs P1.2: B/op 762.6Mi -> 700.6Mi -8.14% (p=0.000) sec/op 1.519 -> 1.511 -0.49% allocs/op 20.03M -> 20.00M -0.12% Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- parser.go | 11 +++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 5742792..99880a3 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -140,7 +140,8 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 CDemo* handler retains a subslice across messages, reuse corrupts it. **The full `go test` suite is the gate** — do not rely on assertion that string tables copy; verify empirically. - **Impact (measured):** −9 MB/op. -- **Result:** _(pending)_ +- **Result:** B/op 762.6→700.6 MiB (**−8.14%** — snappy full-packet buffers), sec/op 1.519→1.511 + (−0.49%), allocs/op −0.12% (−30K). Full 48-replay suite green → reuse aliasing is safe on the corpus. ✅ ### P1.4 — modifier emit early-return when no handler - **Now:** `emitModifierTableEvents` (modifier.go:18) allocates + unmarshals a proto per ActiveModifiers @@ -428,3 +429,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P0 baseline | 1.523 s ±1% | 791.5 MiB | 20.75M | PASS | | P1.1 pendingMessage value-slice | 1.514 s (−0.6%) | 763.8 MiB (−3.5%) | 20.06M (−3.3%) | PASS | | P1.2 outerMessage by value | 1.519 s (−0.3%) | 762.6 MiB (−3.7%) | 20.03M (−3.5%) | PASS | +| P1.3 snappy scratch reuse | 1.511 s (−0.8%) | 700.6 MiB (−11.5%) | 20.00M (−3.6%) | PASS | diff --git a/parser.go b/parser.go index bebfdf7..4b65e06 100644 --- a/parser.go +++ b/parser.go @@ -46,6 +46,7 @@ type Parser struct { modifierTableEntryHandlers []ModifierTableEntryHandler serializers map[string]*serializer pendingMsgBuf pendingMessages + snappyScratch []byte stream *stream stringTables *stringTables stopAtTick uint32 @@ -229,12 +230,18 @@ func (p *Parser) readOuterMessage() (outerMessage, error) { return outerMessage{}, err } - // If the buffer is compressed, decompress it with snappy. + // If the buffer is compressed, decompress it with snappy, reusing a + // parser-level scratch buffer across messages. snappy.Decode reuses the + // destination when it is large enough, amortizing the decompression + // allocation to roughly the largest compressed message seen. This is safe + // because the decoded buffer is consumed within the dispatch of this + // message and never retained across outer messages. if msgCompressed { var err error - if buf, err = snappy.Decode(nil, buf); err != nil { + if buf, err = snappy.Decode(p.snappyScratch[:cap(p.snappyScratch)], buf); err != nil { return outerMessage{}, err } + p.snappyScratch = buf } // Return the message From 3116d5b6706631848a4768d650de83e069d0062d Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:16:22 -0500 Subject: [PATCH 06/39] perf(modifier): skip emission entirely when no handler registered emitModifierTableEvents allocated a proto message and unmarshalled every ActiveModifiers entry on every create/update even when no OnModifierTableEntry handler was registered (the common case). Early-return when there are no handlers. Behavior is unchanged for consumers that register one. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.3: allocs/op 20.00M -> 18.74M -6.32% (p=0.000, -1.26M) B/op 700.6Mi -> 647.9Mi -7.51% sec/op 1.511 -> 1.479 -2.15% Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 ++++- modifier.go | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 99880a3..b594027 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -152,7 +152,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 without reset; only safe on fresh msg). - **Impact:** removes all modifier unmarshal from the bench hot path. Verified ActiveModifiers-only (never touches instancebaseline). -- **Result:** _(pending)_ +- **Result:** allocs/op 20.00M→18.74M (**−6.32%, −1.26M** — bigger than the profile top-12 suggested; + the cost was spread across proto internals), B/op −7.51%, sec/op −2.15%. Did the early-return only; + left per-item fresh-msg unmarshal on the handler path (no consumer retain risk). go test green. ✅ ### P1.5 — reuse per-packet `tuples` + entity-data reader - **Now:** `onCSVCMsg_PacketEntities` allocates `newReader(m.GetEntityData())` and @@ -430,3 +432,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.1 pendingMessage value-slice | 1.514 s (−0.6%) | 763.8 MiB (−3.5%) | 20.06M (−3.3%) | PASS | | P1.2 outerMessage by value | 1.519 s (−0.3%) | 762.6 MiB (−3.7%) | 20.03M (−3.5%) | PASS | | P1.3 snappy scratch reuse | 1.511 s (−0.8%) | 700.6 MiB (−11.5%) | 20.00M (−3.6%) | PASS | +| P1.4 modifier early-return | 1.479 s (−2.9%) | 647.9 MiB (−18.1%) | 18.74M (−9.7%) | PASS | diff --git a/modifier.go b/modifier.go index 4b7395f..79e7a45 100644 --- a/modifier.go +++ b/modifier.go @@ -16,6 +16,13 @@ func (p *Parser) OnModifierTableEntry(fn ModifierTableEntryHandler) { // emitModifierTableEvents emits ModifierBuffTableEntry events // from the given string table items. func (p *Parser) emitModifierTableEvents(items []*stringTableItem) error { + // Nothing to do if no consumer is listening; avoid the per-item proto + // allocation + unmarshal entirely. This is the common case (e.g. the + // benchmark and any parse that doesn't register OnModifierTableEntry). + if len(p.modifierTableEntryHandlers) == 0 { + return nil + } + for _, item := range items { msg := &dota.CDOTAModifierBuffTableEntry{} if err := proto.NewBuffer(item.Value).Unmarshal(msg); err != nil { From a29739f2330df3a57e46929adab72d3dc837aa85 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:21:05 -0500 Subject: [PATCH 07/39] perf(entity): reuse per-packet tuple buffer and entity-data reader onCSVCMsg_PacketEntities allocated a new reader and a fresh []tuple (sized to the update count) on every PacketEntities message. Hoist the tuple type to a package-level entityOpTuple, add reader.reset, and reuse parser-level buffers for both. The handler still sees identical entities/ops in identical order (append order preserved), so expectEntityEvents is unchanged. Not re-entrant, so reuse is safe. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.4: B/op 647.9Mi -> 571.0Mi -11.87% (p=0.000) allocs/op 18.74M -> 18.66M -0.40% sec/op 1.479 -> 1.494 +1.03% (run variance) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 ++++- entity.go | 23 ++++++++++++++--------- parser.go | 2 ++ reader.go | 10 ++++++++++ 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index b594027..a46ece3 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -163,7 +163,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 handlers-outer/tuples-inner emission order `expectEntityEvents` depends on); `reader.reset(buf)` method for the entity-data reader. - **Impact:** −0.44M allocs. (The baseline reader at entity.go:269 is handled by P1.14, not here.) -- **Result:** _(pending)_ +- **Result:** B/op 647.9→571.0 MiB (**−11.87%** — the per-packet tuples backing array was large: + updates×16B over many packets), allocs/op −0.40% (−80K), sec/op +1.03% (run variance; cumulative sec + still −1.9% vs P0). Hoisted tuple to package-level `entityOpTuple`; reused parser-level reader+tuples. go test green. ✅ ### P1.6 — hoist per-entity `fpCache`/`fpNoop` maps to class - **Now:** `newEntity` allocates two maps per entity (entity.go:69-70); name→fieldPath depends only on @@ -433,3 +435,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.2 outerMessage by value | 1.519 s (−0.3%) | 762.6 MiB (−3.7%) | 20.03M (−3.5%) | PASS | | P1.3 snappy scratch reuse | 1.511 s (−0.8%) | 700.6 MiB (−11.5%) | 20.00M (−3.6%) | PASS | | P1.4 modifier early-return | 1.479 s (−2.9%) | 647.9 MiB (−18.1%) | 18.74M (−9.7%) | PASS | +| P1.5 reuse tuples + reader | 1.494 s (−1.9%) | 571.0 MiB (−27.9%) | 18.66M (−10.1%) | PASS | diff --git a/entity.go b/entity.go index 5fa7bbe..b91b057 100644 --- a/entity.go +++ b/entity.go @@ -218,9 +218,17 @@ func (p *Parser) FilterEntity(fb func(*Entity) bool) []*Entity { return entities } +// entityOpTuple pairs an entity with the operation performed on it. Updates are +// buffered during a PacketEntities message, then dispatched to handlers. +type entityOpTuple struct { + e *Entity + op EntityOp +} + // Internal Callback for OnCSVCMsg_PacketEntities. func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error { - r := newReader(m.GetEntityData()) + r := &p.entityReader + r.reset(m.GetEntityData()) var index = int32(-1) var updates = int(m.GetUpdatedEntries()) @@ -237,11 +245,7 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error p.entityFullPackets++ } - type tuple struct { - e *Entity - op EntityOp - } - tuples := make([]tuple, 0, updates) + tuples := p.entityTuples[:0] for ; updates > 0; updates-- { index += int32(r.readUBitVar()) + 1 @@ -300,12 +304,13 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error } } - tuples = append(tuples, tuple{e, op}) + tuples = append(tuples, entityOpTuple{e, op}) } + p.entityTuples = tuples for _, h := range p.entityHandlers { - for _, t := range tuples { - if err := h(t.e, t.op); err != nil { + for i := range tuples { + if err := h(tuples[i].e, tuples[i].op); err != nil { return err } } diff --git a/parser.go b/parser.go index 4b65e06..b6e05e8 100644 --- a/parser.go +++ b/parser.go @@ -47,6 +47,8 @@ type Parser struct { serializers map[string]*serializer pendingMsgBuf pendingMessages snappyScratch []byte + entityReader reader + entityTuples []entityOpTuple stream *stream stringTables *stringTables stopAtTick uint32 diff --git a/reader.go b/reader.go index 0f01a4d..f91849f 100644 --- a/reader.go +++ b/reader.go @@ -20,6 +20,16 @@ func newReader(buf []byte) *reader { return &reader{buf, uint32(len(buf)), 0, 0, 0} } +// reset reinitializes the reader to read from the given buffer, allowing a +// single reader to be reused across messages without allocating a new one. +func (r *reader) reset(buf []byte) { + r.buf = buf + r.size = uint32(len(buf)) + r.pos = 0 + r.bitVal = 0 + r.bitCount = 0 +} + // remBits calculates the number of unread bits in the buffer func (r *reader) remBits() uint32 { return r.remBytes() + r.bitCount From 729d63c8da0d0c87e0ce61320667f4792036c5eb Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:25:47 -0500 Subject: [PATCH 08/39] perf(entity): share field-path name caches on the class name -> field-path resolution depends only on the class serializer, yet each entity allocated its own fpCache/fpNoop maps and recomputed the identical mapping. Move both caches to *class so a name is resolved once per class instead of once per entity, and drop two map allocations per entity creation. Parsing is single-goroutine, so plain maps are safe. go test ./... green (identical golden values; expectPlayer6Name / expectHeroEntityName exercise Get-by-name across many entities). benchstat (8552595443, 10x x10) vs P1.5: allocs/op 18.66M -> 18.63M -0.16% (-30K) B/op 571.0Mi -> 569.5Mi -0.27% sec/op ~ (p=0.190) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- class.go | 8 ++++++++ entity.go | 32 ++++++++++++++------------------ 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index a46ece3..4a8afb1 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -175,7 +175,8 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 - **Clarity:** resolves name→FieldPath on the DTClass, Entity holds only state (Entity.java:88-114). - **Impact:** −2 maps/entity + smaller Entity struct. Golden-safe (resolved fieldPath is class-invariant; `expectPlayer6Name`/`expectHeroEntityName` guard it). -- **Result:** _(pending)_ +- **Result:** allocs/op −0.16% (−30K, two maps/entity removed), B/op −0.27%, sec/op ~ (p=0.190, noise). + Caches now shared on `*class`; single-goroutine parse so safe. go test green. ✅ ### P1.7 — hoist `v(6)` debug guard out of the per-field loop - **Now:** `readFields` calls `v(6)` twice per field path in the hot loop (field_reader.go:13,23). @@ -436,3 +437,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.3 snappy scratch reuse | 1.511 s (−0.8%) | 700.6 MiB (−11.5%) | 20.00M (−3.6%) | PASS | | P1.4 modifier early-return | 1.479 s (−2.9%) | 647.9 MiB (−18.1%) | 18.74M (−9.7%) | PASS | | P1.5 reuse tuples + reader | 1.494 s (−1.9%) | 571.0 MiB (−27.9%) | 18.66M (−10.1%) | PASS | +| P1.6 hoist fp caches to class | 1.509 s (−0.9%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | diff --git a/class.go b/class.go index d10ed2b..13a14b0 100644 --- a/class.go +++ b/class.go @@ -16,6 +16,12 @@ type class struct { classId int32 name string serializer *serializer + + // fpCache and fpNoop memoize name -> field-path resolution, which is + // class-invariant. They are shared by every entity of the class so a name + // is resolved once per class instead of once per entity. + fpCache map[string]*fieldPath + fpNoop map[string]bool } func (c *class) getNameForFieldPath(fp *fieldPath) string { @@ -67,6 +73,8 @@ func (p *Parser) onCDemoClassInfo(m *dota.CDemoClassInfo) error { classId: classId, name: networkName, serializer: p.serializers[networkName], + fpCache: make(map[string]*fieldPath), + fpNoop: make(map[string]bool), } p.classesById[class.classId] = class p.classesByName[class.name] = class diff --git a/entity.go b/entity.go index b91b057..88fe2fd 100644 --- a/entity.go +++ b/entity.go @@ -49,25 +49,21 @@ type EntityHandler func(*Entity, EntityOp) error // Entity represents a single game entity in the replay type Entity struct { - index int32 - serial int32 - class *class - active bool - state *fieldState - fpCache map[string]*fieldPath - fpNoop map[string]bool + index int32 + serial int32 + class *class + active bool + state *fieldState } // newEntity returns a new entity for the given index, serial and class func newEntity(index, serial int32, class *class) *Entity { return &Entity{ - index: index, - serial: serial, - class: class, - active: true, - state: newFieldState(), - fpCache: make(map[string]*fieldPath), - fpNoop: make(map[string]bool), + index: index, + serial: serial, + class: class, + active: true, + state: newFieldState(), } } @@ -92,20 +88,20 @@ func (e *Entity) Dump() { // Get returns the current value of the Entity state for the given key func (e *Entity) Get(name string) interface{} { - if fp, ok := e.fpCache[name]; ok { + if fp, ok := e.class.fpCache[name]; ok { return e.state.get(fp) } - if e.fpNoop[name] { + if e.class.fpNoop[name] { return nil } fp := newFieldPath() if !e.class.getFieldPathForName(fp, name) { - e.fpNoop[name] = true + e.class.fpNoop[name] = true fp.release() return nil } - e.fpCache[name] = fp + e.class.fpCache[name] = fp return e.state.get(fp) } From 16bc0aaffb9b9cfdf1f0306d0124715225dbf3b4 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:30:12 -0500 Subject: [PATCH 09/39] perf(reader): hoist debug verbosity check out of the field loop readFields called v(6) twice per field path in the hot loop (millions of times per parse). Evaluate it once per readFields call into a local. readFields is invoked fresh per entity update, so a mid-parse debug-level change still takes effect on the next call. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.6: sec/op 1.509 -> 1.476 -2.18% (p=0.002) allocs/op unchanged (CPU-only) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- field_reader.go | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 4a8afb1..9983928 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -184,7 +184,8 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 tick mutates `debugLevel` mid-parse and `readFields` is called fresh per entity-update, so per-call re-eval preserves behavior). - **Impact:** −~33M calls/branches (CPU only, ~0 alloc when disabled). -- **Result:** _(pending)_ +- **Result:** sec/op 1.509→1.476 (**−2.18%**, p=0.002 — `v(6)` was 2 calls/field across millions of + fields), allocs/B unchanged (CPU-only). go test green. ✅ ### P1.8 — buffer `stream` IO - **Now:** `stream.readByte`→`readBytes(1)`→`io.ReadFull` per byte; 3 varints + payload per outer @@ -438,3 +439,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.4 modifier early-return | 1.479 s (−2.9%) | 647.9 MiB (−18.1%) | 18.74M (−9.7%) | PASS | | P1.5 reuse tuples + reader | 1.494 s (−1.9%) | 571.0 MiB (−27.9%) | 18.66M (−10.1%) | PASS | | P1.6 hoist fp caches to class | 1.509 s (−0.9%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | +| P1.7 hoist v(6) debug guard | 1.476 s (−3.1%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | diff --git a/field_reader.go b/field_reader.go index ae1fee9..1d87a5a 100644 --- a/field_reader.go +++ b/field_reader.go @@ -7,10 +7,14 @@ import ( func readFields(r *reader, s *serializer, state *fieldState) { fps := readFieldPaths(r) + // Evaluate the debug verbosity once per call rather than twice per field in + // the hot loop. readFields is called fresh per entity update, so a mid-parse + // debug-level change (e.g. a debug-tick) still takes effect on the next call. + dbg := v(6) for _, fp := range fps { decoder := s.getDecoderForFieldPath(fp, 0) - if v(6) { + if dbg { name := strings.Join(s.getNameForFieldPath(fp, 0), ".") typ := s.getTypeForFieldPath(fp, 0) field := s.getFieldForFieldPath(fp, 0) @@ -20,7 +24,7 @@ func readFields(r *reader, s *serializer, state *fieldState) { val := decoder(r) state.set(fp, val) - if v(6) { + if dbg { name := strings.Join(s.getNameForFieldPath(fp, 0), ".") fp2 := newFieldPath() b := s.getFieldPathForName(fp2, name) From a354e5f86a460cdc89308fde5cdcd2071fc9e6c2 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:40:56 -0500 Subject: [PATCH 10/39] perf(stream): buffer non-byte-reader inputs (e.g. *os.File) The outer loop reads 3 varints + payload per message, one byte at a time; against a raw *os.File that is one read syscall per byte (~80% of CPU on the streaming path). Wrap the reader in a bufio.Reader when it does not already implement io.ByteReader, so in-memory readers (*bytes.Reader, used by NewParser) are left unwrapped and incur no extra copy. go test ./... green (identical golden values). Canonical in-memory bench flat (bytes.Reader unwrapped). Streaming NewStreamParser(os.File) path, measured separately: sec/op 1.612 -> 1.507 -6.47% (p=0.002) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 ++++- stream.go | 10 +++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 9983928..a1e871e 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -194,7 +194,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 `io.ByteReader` (`*os.File` isn't; `*bytes.Reader` is → `NewParser` stays unwrapped, no double-buffer). - **Impact:** 798→658 ms on the streaming path. **Note:** the canonical in-memory bench (P0) won't show this — it's a real-world streaming-path win; verify with a streaming-reader bench variant. -- **Result:** _(pending)_ +- **Result:** canonical in-memory bench flat (+0.46%, noise — `bytes.Reader` left unwrapped, no + regression). Streaming `NewStreamParser(os.File)` path **1.612→1.507 s/op (−6.47%, p=0.002)**, measured + with a throwaway streaming bench (reverted before commit). go test green. ✅ ### P1.9 — reusable field-path buffer ⭐ biggest single win - **Now:** `readFieldPaths` (field_path.go:309-337) starts `paths := []*fieldPath{}` (cap 0) and @@ -440,3 +442,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.5 reuse tuples + reader | 1.494 s (−1.9%) | 571.0 MiB (−27.9%) | 18.66M (−10.1%) | PASS | | P1.6 hoist fp caches to class | 1.509 s (−0.9%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | | P1.7 hoist v(6) debug guard | 1.476 s (−3.1%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | +| P1.8 buffer stream IO (streaming −6.5%) | 1.483 s (flat, in-mem) | 569.5 MiB | 18.63M | PASS | diff --git a/stream.go b/stream.go index 3f3c069..0f0af3f 100644 --- a/stream.go +++ b/stream.go @@ -1,6 +1,7 @@ package manta import ( + "bufio" "io" "github.com/dotabuff/manta/dota" @@ -16,8 +17,15 @@ type stream struct { size uint32 } -// newStream creates a new stream from a given io.Reader +// newStream creates a new stream from a given io.Reader. If the reader does not +// already provide buffered byte access (e.g. an *os.File), it is wrapped in a +// bufio.Reader so the per-byte varint reads in the outer loop do not issue one +// read syscall per byte. In-memory readers such as *bytes.Reader already +// satisfy io.ByteReader and are left unwrapped to avoid a redundant copy. func newStream(r io.Reader) *stream { + if _, ok := r.(io.ByteReader); !ok { + r = bufio.NewReaderSize(r, buffer) + } return &stream{r, make([]byte, buffer), buffer} } From 769aad1fcb6b2c7f36e96e50685fe1de17350ea9 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 10:49:06 -0500 Subject: [PATCH 11/39] perf(fieldpath): reuse a value-type field-path buffer (eliminate 56%-alloc slice) readFieldPaths allocated a fresh []*fieldPath per entity update and grew it via append(fp.copy()), which the profile attributed to ~56% of all allocations. Make fieldPath a value type with a fixed [7]int array and decode the op stream into a reused parser-level []fieldPath buffer; readFields consumes it in place. The two-pass order (all ops, then all values) and the field-path values are unchanged, so decoding is identical. The op accumulator is borrowed from the existing sync.Pool so taking its address for the op functions does not force a per-call heap escape (a value-typed local would, regressing B/op). go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.8: allocs/op 18.63M -> 10.65M -42.86% (p=0.000, -7.98M) B/op 569.5Mi -> 398.6Mi -30.00% sec/op 1.483 -> 1.198 -19.22% Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 6 +++++- entity.go | 6 +++--- field_path.go | 42 ++++++++++++++++++++---------------------- field_reader.go | 11 ++++++----- parser.go | 1 + 5 files changed, 35 insertions(+), 31 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index a1e871e..c878207 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -215,7 +215,10 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 `fieldPaths[MAX_PROPERTIES=0x3FFF]` (FieldReader.java:11-14, S2FieldReader.java:50-65). - **Impact:** allocs/op ~11M → ~4–5M. **Keep the fixed depth 7** (`[7]int`). Golden-safe (only the container changes; field-path values + decode order unchanged). -- **Result:** _(pending)_ +- **Result:** allocs/op 18.63M→10.65M (**−42.86%, −7.98M**), B/op 569.5→398.6 MiB (−30.0%), sec/op + 1.483→1.198 (**−19.22%**). The 56%-of-allocs `readFieldPaths` container is gone. Gotcha: first attempt + regressed B/op +31% because `&fp` escaped via the indirect closure call — borrowing the accumulator + from the pool fixed it. go test green. ✅ **(headline win)** ### P1.10 — word-at-a-time bit reader - **Now:** `readBits` refills the accumulator one byte at a time (reader.go:50-61) with a per-byte @@ -443,3 +446,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.6 hoist fp caches to class | 1.509 s (−0.9%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | | P1.7 hoist v(6) debug guard | 1.476 s (−3.1%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | | P1.8 buffer stream IO (streaming −6.5%) | 1.483 s (flat, in-mem) | 569.5 MiB | 18.63M | PASS | +| **P1.9 reusable field-path buffer** | **1.198 s (−21.3%)** | **398.6 MiB (−49.6%)** | **10.65M (−48.7%)** | PASS | diff --git a/entity.go b/entity.go index 88fe2fd..f2cb1fc 100644 --- a/entity.go +++ b/entity.go @@ -266,8 +266,8 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error e = newEntity(index, serial, class) p.entities[index] = e - readFields(newReader(baseline), class.serializer, e.state) - readFields(r, class.serializer, e.state) + p.fpBuf = readFields(newReader(baseline), class.serializer, e.state, p.fpBuf) + p.fpBuf = readFields(r, class.serializer, e.state, p.fpBuf) op = EntityOpCreated | EntityOpEntered } else { @@ -281,7 +281,7 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error op |= EntityOpEntered } - readFields(r, e.class.serializer, e.state) + p.fpBuf = readFields(r, e.class.serializer, e.state, p.fpBuf) } } else { diff --git a/field_path.go b/field_path.go index a2582cf..71f53a5 100644 --- a/field_path.go +++ b/field_path.go @@ -9,7 +9,7 @@ import ( var huffTree = newHuffmanTree() type fieldPath struct { - path []int + path [7]int last int done bool } @@ -259,7 +259,7 @@ func (fp *fieldPath) pop(n int) { // copy returns a copy of the fieldPath func (fp *fieldPath) copy() *fieldPath { x := fpPool.Get().(*fieldPath) - copy(x.path, fp.path) + x.path = fp.path x.last = fp.last x.done = fp.done return x @@ -283,19 +283,13 @@ func newFieldPath() *fieldPath { var fpPool = &sync.Pool{ New: func() interface{} { - return &fieldPath{ - path: make([]int, 7), - last: 0, - done: false, - } + return &fieldPath{} }, } -var fpReset = []int{-1, 0, 0, 0, 0, 0, 0} - // reset resets the fieldPath to the empty value func (fp *fieldPath) reset() { - copy(fp.path, fpReset) + fp.path = [7]int{-1} fp.last = 0 fp.done = false } @@ -305,15 +299,21 @@ func (fp *fieldPath) release() { fpPool.Put(fp) } -// readFieldPaths reads a new slice of fieldPath values from the given reader -func readFieldPaths(r *reader) []*fieldPath { - fp := newFieldPath() +// readFieldPaths decodes the field-path operation stream from r into the +// provided buffer (which the caller resets to length 0) and returns the grown +// slice. Field paths are cumulative deltas, snapshotted by value into the +// buffer, so there is no per-path allocation or sync.Pool churn and the buffer +// is reused across calls. +func readFieldPaths(r *reader, paths []fieldPath) []fieldPath { + // Borrow a reusable accumulator from the pool. Taking its address for the + // field-path op functions (an indirect call) would otherwise force it to + // escape and heap-allocate on every call; the pool keeps that amortized. + fp := fpPool.Get().(*fieldPath) + fp.reset() node, next := huffTree, huffTree - paths := []*fieldPath{} - - for !fp.done { + for { if r.readBits(1) == 1 { next = node.Right() } else { @@ -323,17 +323,15 @@ func readFieldPaths(r *reader) []*fieldPath { if next.IsLeaf() { node = huffTree fieldPathTable[next.Value()].fn(r, fp) - if !fp.done { - paths = append(paths, fp.copy()) + if fp.done { + fpPool.Put(fp) + return paths } + paths = append(paths, *fp) } else { node = next } } - - fp.release() - - return paths } // newHuffmanTree creates a new huffmanTree from the field path table diff --git a/field_reader.go b/field_reader.go index 1d87a5a..877b8d5 100644 --- a/field_reader.go +++ b/field_reader.go @@ -4,14 +4,15 @@ import ( "strings" ) -func readFields(r *reader, s *serializer, state *fieldState) { - fps := readFieldPaths(r) +func readFields(r *reader, s *serializer, state *fieldState, fpBuf []fieldPath) []fieldPath { + fpBuf = readFieldPaths(r, fpBuf[:0]) // Evaluate the debug verbosity once per call rather than twice per field in // the hot loop. readFields is called fresh per entity update, so a mid-parse // debug-level change (e.g. a debug-tick) still takes effect on the next call. dbg := v(6) - for _, fp := range fps { + for i := range fpBuf { + fp := &fpBuf[i] decoder := s.getDecoderForFieldPath(fp, 0) if dbg { @@ -41,7 +42,7 @@ func readFields(r *reader, s *serializer, state *fieldState) { _debugf(" => %#v", val) } - - fp.release() } + + return fpBuf } diff --git a/parser.go b/parser.go index b6e05e8..3f740e1 100644 --- a/parser.go +++ b/parser.go @@ -49,6 +49,7 @@ type Parser struct { snappyScratch []byte entityReader reader entityTuples []entityOpTuple + fpBuf []fieldPath stream *stream stringTables *stringTables stopAtTick uint32 From 54c074f25a8530323613704ee00628dc06d7708a Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 11:00:45 -0500 Subject: [PATCH 12/39] perf(reader): refill the bit accumulator a word at a time readBits refilled the accumulator one byte at a time; it is the hottest function in the parser. Load a full 64-bit little-endian word when >= 8 bytes remain, masking it to the whole bytes of headroom before merging so no stale partial-byte bits are left above bitCount, and fall back to byte-at-a-time only for the buffer tail. Every read is <= 32 bits (now asserted for quantized floats), so one refill always suffices. Because the refill reads bytes ahead into the accumulator, add realign() so the byte-aligned readBytes/readByte fast path rewinds those buffered bytes and stays zero-copy (aliasing the underlying buffer) at any byte boundary, not just when bitCount==0. This makes byte reads zero-copy more often than before. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.9: sec/op 1.198 -> 1.058 -11.64% (p=0.000) B/op flat allocs/op flat Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 7 ++++++- quantizedfloat.go | 8 ++++++++ reader.go | 50 +++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index c878207..685bb6d 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -235,7 +235,11 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 - **Add a build-time assert** that quantized `Bitcount <= 32` (quantizedfloat.go integer-encode loop can raise it) so a pathological field can't overflow the accumulator. - **Impact:** large CPU win (only visible after P0). 0 allocs delta. -- **Result:** _(pending)_ +- **Result:** sec/op 1.198→1.058 (**−11.64%**), allocs/B flat. Word refill masks before shifting (no + stale partial-byte bits). Added `realign()` so byte-aligned `readBytes`/`readByte` rewind the + buffered word and stay zero-copy — this extended the fast path beyond the old `bitCount==0`, making byte + reads zero-copy *more* often than before (hence no B/op regression). Added quantized `Bitcount<=32` + assert. go test green. ✅ (also satisfies the P-guard zero-copy invariant; P-guard adds the test.) ### P1.11 — varints + `readByte`/`readLeUintX` from the accumulator - **Now:** `readVarUint32/64` call `readByte` per byte (byte-aligned fast path → `nextByte`), each a @@ -447,3 +451,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.7 hoist v(6) debug guard | 1.476 s (−3.1%) | 569.5 MiB (−28.1%) | 18.63M (−10.2%) | PASS | | P1.8 buffer stream IO (streaming −6.5%) | 1.483 s (flat, in-mem) | 569.5 MiB | 18.63M | PASS | | **P1.9 reusable field-path buffer** | **1.198 s (−21.3%)** | **398.6 MiB (−49.6%)** | **10.65M (−48.7%)** | PASS | +| **P1.10 word-at-a-time reader** | **1.058 s (−30.5%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | diff --git a/quantizedfloat.go b/quantizedfloat.go index aefea68..b9c3a4d 100644 --- a/quantizedfloat.go +++ b/quantizedfloat.go @@ -221,6 +221,14 @@ func newQuantizedFloatDecoder(bitCount, flags *int32, lowValue, highValue *float qfd.High = qfd.Low + float32(Range2) - qfd.Offset } + // The bit reader refills its accumulator a word at a time and relies on + // every read being at most 32 bits. The integer-encoding path above can in + // principle raise the bit count, so guard the invariant explicitly rather + // than silently corrupting the stream. + if qfd.Bitcount > 32 { + _panicf("quantized float bit count %d exceeds 32", qfd.Bitcount) + } + // Assign multipliers qfd.assignMultipliers(uint32(steps)) diff --git a/reader.go b/reader.go index f91849f..db1b7a1 100644 --- a/reader.go +++ b/reader.go @@ -56,24 +56,63 @@ func (r *reader) nextByte() byte { return r.buf[r.pos-1] } -// readBits returns the uint32 value for the given number of sequential bits +// readBitMasks[k] is a bitmask of the low k bits. It is used instead of an +// inline (1< r.bitCount && r.pos+8 <= r.size { + w := binary.LittleEndian.Uint64(r.buf[r.pos:]) + free := (64 - r.bitCount) >> 3 // whole bytes of accumulator headroom + bits := free * 8 + r.bitVal |= (w & readBitMasks[bits]) << r.bitCount + r.pos += free + r.bitCount += bits + } for n > r.bitCount { r.bitVal |= uint64(r.nextByte()) << r.bitCount r.bitCount += 8 } - x := (r.bitVal & ((1 << n) - 1)) + x := r.bitVal & readBitMasks[n] r.bitVal >>= n r.bitCount -= n return uint32(x) } +// realign discards the whole bytes the word refill buffered in the accumulator +// by rewinding the read position, so byte-oriented reads can proceed directly +// from (and alias) the underlying buffer. Only valid when byte-aligned, i.e. +// bitCount is a multiple of 8. +func (r *reader) realign() { + r.pos -= r.bitCount >> 3 + r.bitVal = 0 + r.bitCount = 0 +} + // readByte reads a single byte func (r *reader) readByte() byte { // Fast path if we're byte aligned - if r.bitCount == 0 { + if r.bitCount&7 == 0 { + if r.bitCount != 0 { + r.realign() + } return r.nextByte() } @@ -83,7 +122,10 @@ func (r *reader) readByte() byte { // readBytes reads the given number of bytes func (r *reader) readBytes(n uint32) []byte { // Fast path if we're byte aligned - if r.bitCount == 0 { + if r.bitCount&7 == 0 { + if r.bitCount != 0 { + r.realign() + } r.pos += n if r.pos > r.size { _panicf("readBytes: insufficient buffer (%d of %d)", r.pos, r.size) From 142f22a25d33a5f5f23cec71d865938c9d295e7a Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 11:04:26 -0500 Subject: [PATCH 13/39] docs: skip P1.11 (varints-from-accumulator breaks reader pos contract) Routing readByte/readLeUintX through readBits would consume into the word reader's read-ahead accumulator, leaving r.pos ahead of the logical position. The reader contract (r.pos, remBytes) and the reader unit tests rely on r.pos being the logical position. The marginal gain wasn't worth it; P1.10's realign already keeps these byte reads fast and zero-copy. Reverted. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 685bb6d..6b0fb6f 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -250,7 +250,12 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 allocates). - **Impact:** compounds P1.10. `BenchmarkReadVarUint32/64` (reader_test.go) measures it directly. - **Guard:** preserve `readBytes` zero-copy aliasing (P-guard below). -- **Result:** _(pending)_ +- **Result:** **SKIPPED.** Routing `readByte`/`readLeUintX` through `readBits` makes them consume into + the read-ahead accumulator, leaving `r.pos` ahead of the logical position. The reader unit tests + (`TestReaderReplayBeginning`, `TestReaderVarints`) assert exact `r.pos` and manually set `r.pos = 0`, and + `r.pos`/`remBytes` are part of the reader contract. The marginal gain (avoiding a `make` on rare + *unaligned* `readLeUintX`) wasn't worth rewriting tests and risking the `pos` contract. P1.10's `realign` + already keeps these byte reads fast and zero-copy with `pos` accurate. Reverted. ### P1.12 — flatten huffman tree to int arrays - **Now:** `readFieldPaths` walks an interface pointer-tree per bit (`node.Right()/Left()/IsLeaf()/Value()` @@ -452,3 +457,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.8 buffer stream IO (streaming −6.5%) | 1.483 s (flat, in-mem) | 569.5 MiB | 18.63M | PASS | | **P1.9 reusable field-path buffer** | **1.198 s (−21.3%)** | **398.6 MiB (−49.6%)** | **10.65M (−48.7%)** | PASS | | **P1.10 word-at-a-time reader** | **1.058 s (−30.5%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | +| P1.11 varints from accumulator | _skipped (breaks reader pos contract)_ | — | — | — | From 59f33d6dbc41300336ba93bd6344363574f23665 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 11:09:47 -0500 Subject: [PATCH 14/39] perf(fieldpath): flatten the field-path huffman tree into int arrays readFieldPaths walked an interface-based huffman tree one bit at a time, with ~2-3 interface-method dispatches plus a pointer chase per bit. Flatten the tree (derived from manta's own huffTree, so codes are identical) into int32 child arrays where a negative entry -(op+1) marks a leaf, and walk those instead. Adds TestHuffmanFlatMatchesTree to lock the flat arrays to the interface tree. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.10: sec/op 1.058 -> 0.960 -9.28% (p=0.000) B/op / allocs/op unchanged Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 ++++- field_path.go | 53 ++++++++++++++++++++++++++++++++++++++++++------- huffman_test.go | 43 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 8 deletions(-) create mode 100644 huffman_test.go diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 6b0fb6f..121a8a1 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -266,7 +266,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 numbering would desync). - **Impact:** removes per-bit interface calls. 0 allocs. **Add a permanent decoder-equivalence test** (interface-walk vs flat path → identical ordinal + bits-consumed across all 256 prefixes). -- **Result:** _(pending)_ +- **Result:** sec/op 1058.5→960.2 ms (**−9.28%**), allocs/B unchanged. Flat `int32` child arrays + (negative = leaf op) replace the per-bit interface walk; built from manta's own tree. Added + `TestHuffmanFlatMatchesTree` (flat ≡ interface tree). go test green. ✅ (under 1 s now.) ### P1.13 — 8-bit field-op lookup table - **Now:** even flattened, each op is a 3–8 iteration bit-walk. @@ -458,3 +460,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **P1.9 reusable field-path buffer** | **1.198 s (−21.3%)** | **398.6 MiB (−49.6%)** | **10.65M (−48.7%)** | PASS | | **P1.10 word-at-a-time reader** | **1.058 s (−30.5%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | | P1.11 varints from accumulator | _skipped (breaks reader pos contract)_ | — | — | — | +| **P1.12 flatten huffman tree** | **0.960 s (−37.0%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | diff --git a/field_path.go b/field_path.go index 71f53a5..dd1145a 100644 --- a/field_path.go +++ b/field_path.go @@ -8,6 +8,44 @@ import ( var huffTree = newHuffmanTree() +// Flattened representation of huffTree for fast field-path op decoding. Each +// internal node n occupies huffTreeLeft[n] and huffTreeRight[n]; a non-negative +// entry is a child node index and a negative entry -(op+1) is a leaf carrying +// field-path op id op. Walking int arrays avoids the interface-method dispatch +// and pointer chasing of the huffmanTree on the hot path. It is derived from +// manta's own huffTree, so the codes are identical to the interface-tree walk. +var ( + huffTreeLeft []int32 + huffTreeRight []int32 + huffTreeRoot int32 +) + +func init() { + huffTreeRoot = flattenHuffmanTree(huffTree) +} + +// flattenHuffmanTree appends the internal nodes of t to huffTreeLeft/huffTreeRight +// and returns the index assigned to t. Leaf children are encoded inline as +// -(op+1). +func flattenHuffmanTree(t huffmanTree) int32 { + idx := int32(len(huffTreeLeft)) + huffTreeLeft = append(huffTreeLeft, 0) + huffTreeRight = append(huffTreeRight, 0) + + if l := t.Left(); l.IsLeaf() { + huffTreeLeft[idx] = -(int32(l.Value()) + 1) + } else { + huffTreeLeft[idx] = flattenHuffmanTree(l) + } + if r := t.Right(); r.IsLeaf() { + huffTreeRight[idx] = -(int32(r.Value()) + 1) + } else { + huffTreeRight[idx] = flattenHuffmanTree(r) + } + + return idx +} + type fieldPath struct { path [7]int last int @@ -311,25 +349,26 @@ func readFieldPaths(r *reader, paths []fieldPath) []fieldPath { fp := fpPool.Get().(*fieldPath) fp.reset() - node, next := huffTree, huffTree + node := huffTreeRoot for { + var child int32 if r.readBits(1) == 1 { - next = node.Right() + child = huffTreeRight[node] } else { - next = node.Left() + child = huffTreeLeft[node] } - if next.IsLeaf() { - node = huffTree - fieldPathTable[next.Value()].fn(r, fp) + if child < 0 { + node = huffTreeRoot + fieldPathTable[-child-1].fn(r, fp) if fp.done { fpPool.Put(fp) return paths } paths = append(paths, *fp) } else { - node = next + node = child } } } diff --git a/huffman_test.go b/huffman_test.go new file mode 100644 index 0000000..9dc3e9f --- /dev/null +++ b/huffman_test.go @@ -0,0 +1,43 @@ +package manta + +import "testing" + +// TestHuffmanFlatMatchesTree verifies the flattened field-path huffman arrays +// (huffTreeLeft/huffTreeRight) decode identically to the interface tree they are +// derived from, so the fast path can never silently desync from the canonical +// tree if the op table or weights change. +func TestHuffmanFlatMatchesTree(t *testing.T) { + var walk func(node int32, itree huffmanTree) + walk = func(node int32, itree huffmanTree) { + if c := huffTreeLeft[node]; c < 0 { + l := itree.Left() + if !l.IsLeaf() { + t.Fatalf("node %d left: flat=leaf op %d, tree=internal", node, -c-1) + } + if got := int32(l.Value()); got != -c-1 { + t.Fatalf("node %d left: flat op %d != tree op %d", node, -c-1, got) + } + } else { + if itree.Left().IsLeaf() { + t.Fatalf("node %d left: flat=internal %d, tree=leaf op %d", node, c, itree.Left().Value()) + } + walk(c, itree.Left()) + } + + if c := huffTreeRight[node]; c < 0 { + r := itree.Right() + if !r.IsLeaf() { + t.Fatalf("node %d right: flat=leaf op %d, tree=internal", node, -c-1) + } + if got := int32(r.Value()); got != -c-1 { + t.Fatalf("node %d right: flat op %d != tree op %d", node, -c-1, got) + } + } else { + if itree.Right().IsLeaf() { + t.Fatalf("node %d right: flat=internal %d, tree=leaf op %d", node, c, itree.Right().Value()) + } + walk(c, itree.Right()) + } + } + walk(huffTreeRoot, huffTree) +} From b11b3c24a3b1e6b77fe38295832a5d7502f56b23 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 11:14:35 -0500 Subject: [PATCH 15/39] perf(fieldpath): resolve field-path ops via an 8-bit lookup table Replace the per-bit flat-tree walk with a 256-entry lookup that resolves most ops (codes <= 8 bits) in a single index, falling back to the flat-tree walk for longer codes. Add reader.peekBits (zero-pads, never over-reads past the buffer) and skipBits so the lookup can inspect a fixed window even at the final FieldPathEncodeFinish near the end of the stream. The table is built once from manta's immutable huffTree (swapNodes/addNode are never called). TestHuffmanLookupMatchesWalk checks the lookup matches the walk (op + bits consumed) over 5000 random streams. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P1.12: sec/op 0.960 -> 0.796 -17.10% (p=0.000) B/op / allocs/op unchanged Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 +++- field_path.go | 80 +++++++++++++++++++++++++++++++++++++++---------- huffman_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++- reader.go | 28 +++++++++++++++++ 4 files changed, 165 insertions(+), 18 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 121a8a1..78db033 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -282,7 +282,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 (`swapNodes`/`addNode`) after the table is built. - **Note:** the "99.7%" is clarity's runtime figure; manta's static weights resolve ~98.4% within ≤8 bits (28/40 ops have >8-bit codes, max 17) — still a large majority. Don't cite 99.7% as verified. -- **Result:** _(pending)_ +- **Result:** sec/op 960.2→796.0 ms (**−17.10%**), allocs/B unchanged. 256-entry lookup resolves most + ops in one index; `peekBits` zero-pads (never over-reads), flat-walk fallback for >8-bit codes. + `TestHuffmanLookupMatchesWalk` (5000 random streams) confirms lookup ≡ walk (op + bits consumed). go test green. ✅ ### P1.14 — decode class baseline once, clone per entity - **Now:** every entity CREATE re-parses raw baseline bytes from scratch @@ -461,3 +463,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **P1.10 word-at-a-time reader** | **1.058 s (−30.5%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | | P1.11 varints from accumulator | _skipped (breaks reader pos contract)_ | — | — | — | | **P1.12 flatten huffman tree** | **0.960 s (−37.0%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | +| **P1.13 8-bit op lookup table** | **0.796 s (−47.7%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | diff --git a/field_path.go b/field_path.go index dd1145a..7870df6 100644 --- a/field_path.go +++ b/field_path.go @@ -22,6 +22,41 @@ var ( func init() { huffTreeRoot = flattenHuffmanTree(huffTree) + buildHuffLookup() +} + +// huffLookupBits is the width of the field-path op lookup window. Most ops have +// codes no longer than this and resolve in a single table index. +const huffLookupBits = 8 + +// huffLookup resolves up to huffLookupBits of the field-path op stream in one +// step. Each entry packs the consumed bit count in its low byte (0 meaning the +// code is longer than the window) and, in its high byte, either the resolved op +// id (when the low byte is non-zero) or the flat-tree node to continue from. +var huffLookup [1 << huffLookupBits]uint16 + +func buildHuffLookup() { + for v := 0; v < len(huffLookup); v++ { + node := huffTreeRoot + resolved := false + for bit := uint32(0); bit < huffLookupBits; bit++ { + var child int32 + if (v>>bit)&1 == 1 { + child = huffTreeRight[node] + } else { + child = huffTreeLeft[node] + } + if child < 0 { + huffLookup[v] = uint16(bit+1) | uint16(-child-1)<<8 + resolved = true + break + } + node = child + } + if !resolved { + huffLookup[v] = uint16(node) << 8 + } + } } // flattenHuffmanTree appends the internal nodes of t to huffTreeLeft/huffTreeRight @@ -349,27 +384,40 @@ func readFieldPaths(r *reader, paths []fieldPath) []fieldPath { fp := fpPool.Get().(*fieldPath) fp.reset() - node := huffTreeRoot - for { - var child int32 - if r.readBits(1) == 1 { - child = huffTreeRight[node] + // Resolve the op via the 8-bit lookup table, falling back to a flat-tree + // walk for the rare codes longer than the lookup window. peekBits never + // over-reads, so the final FieldPathEncodeFinish near the end of the + // buffer is handled correctly. + var op int32 + entry := huffLookup[r.peekBits(huffLookupBits)] + if consumed := entry & 0xFF; consumed != 0 { + r.skipBits(uint32(consumed)) + op = int32(entry >> 8) } else { - child = huffTreeLeft[node] + r.skipBits(huffLookupBits) + node := int32(entry >> 8) + for { + var child int32 + if r.readBits(1) == 1 { + child = huffTreeRight[node] + } else { + child = huffTreeLeft[node] + } + if child < 0 { + op = -child - 1 + break + } + node = child + } } - if child < 0 { - node = huffTreeRoot - fieldPathTable[-child-1].fn(r, fp) - if fp.done { - fpPool.Put(fp) - return paths - } - paths = append(paths, *fp) - } else { - node = child + fieldPathTable[op].fn(r, fp) + if fp.done { + fpPool.Put(fp) + return paths } + paths = append(paths, *fp) } } diff --git a/huffman_test.go b/huffman_test.go index 9dc3e9f..d8480b8 100644 --- a/huffman_test.go +++ b/huffman_test.go @@ -1,6 +1,9 @@ package manta -import "testing" +import ( + "math/rand" + "testing" +) // TestHuffmanFlatMatchesTree verifies the flattened field-path huffman arrays // (huffTreeLeft/huffTreeRight) decode identically to the interface tree they are @@ -41,3 +44,68 @@ func TestHuffmanFlatMatchesTree(t *testing.T) { } walk(huffTreeRoot, huffTree) } + +// decodeOneHuffOpLookup decodes a single field-path op code (without executing +// the op) using the 8-bit lookup fast path. +func decodeOneHuffOpLookup(r *reader) int32 { + entry := huffLookup[r.peekBits(huffLookupBits)] + if consumed := entry & 0xFF; consumed != 0 { + r.skipBits(uint32(consumed)) + return int32(entry >> 8) + } + r.skipBits(huffLookupBits) + node := int32(entry >> 8) + for { + var child int32 + if r.readBits(1) == 1 { + child = huffTreeRight[node] + } else { + child = huffTreeLeft[node] + } + if child < 0 { + return -child - 1 + } + node = child + } +} + +// decodeOneHuffOpWalk decodes a single field-path op code using only the flat +// tree walk. +func decodeOneHuffOpWalk(r *reader) int32 { + node := huffTreeRoot + for { + var child int32 + if r.readBits(1) == 1 { + child = huffTreeRight[node] + } else { + child = huffTreeLeft[node] + } + if child < 0 { + return -child - 1 + } + node = child + } +} + +// TestHuffmanLookupMatchesWalk verifies the 8-bit lookup fast path decodes the +// same op and consumes the same number of bits as the pure flat-tree walk for +// many random streams. An 8-byte buffer comfortably holds the longest code +// (17 bits), so neither path reaches the buffer end. +func TestHuffmanLookupMatchesWalk(t *testing.T) { + rng := rand.New(rand.NewSource(1)) + for trial := 0; trial < 5000; trial++ { + buf := make([]byte, 8) + for i := range buf { + buf[i] = byte(rng.Intn(256)) + } + ra := newReader(buf) + rb := newReader(buf) + opA := decodeOneHuffOpLookup(ra) + opB := decodeOneHuffOpWalk(rb) + consumedA := ra.pos*8 - ra.bitCount + consumedB := rb.pos*8 - rb.bitCount + if opA != opB || consumedA != consumedB { + t.Fatalf("trial %d buf %x: lookup op=%d bits=%d, walk op=%d bits=%d", trial, buf, opA, consumedA, opB, consumedB) + } + } +} diff --git a/reader.go b/reader.go index db1b7a1..d7eeb76 100644 --- a/reader.go +++ b/reader.go @@ -96,6 +96,34 @@ func (r *reader) readBits(n uint32) uint32 { return uint32(x) } +// peekBits returns the next n (<= 32) bits without consuming them, refilling the +// accumulator as needed. If fewer than n bits remain in the buffer the missing +// high bits are returned as zero; the buffer is never over-read past its end. +// This is required by the field-path op lookup, which inspects a fixed window +// that can extend past the final op near the end of the stream. +func (r *reader) peekBits(n uint32) uint32 { + for n > r.bitCount && r.pos+8 <= r.size { + w := binary.LittleEndian.Uint64(r.buf[r.pos:]) + free := (64 - r.bitCount) >> 3 + bits := free * 8 + r.bitVal |= (w & readBitMasks[bits]) << r.bitCount + r.pos += free + r.bitCount += bits + } + for n > r.bitCount && r.pos < r.size { + r.bitVal |= uint64(r.nextByte()) << r.bitCount + r.bitCount += 8 + } + + return uint32(r.bitVal & readBitMasks[n]) +} + +// skipBits discards n (<= bitCount) bits already buffered in the accumulator. +func (r *reader) skipBits(n uint32) { + r.bitVal >>= n + r.bitCount -= n +} + // realign discards the whole bytes the word refill buffered in the accumulator // by rewinding the read position, so byte-oriented reads can proceed directly // from (and alias) the underlying buffer. Only valid when byte-aligned, i.e. From 067326980a7604d22919a0f927e6d6c3772c8d2a Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 11:17:52 -0500 Subject: [PATCH 16/39] perf(entity): decode each class baseline once and clone per entity Every entity creation re-decoded the raw class baseline bitstream from scratch (~4x the cost of a delta update). Decode each class baseline once into a template fieldState and clone it for new entities instead. clone() shares immutable leaf values and deep-copies nested fieldStates, so it is cheaper than re-decoding (no huffman/bit decode, no re-boxing) while remaining independent per entity. Templates are invalidated when the instancebaseline string table updates, so a changed baseline is re-decoded. go test ./... green (identical golden values; the cloned+overlaid state is value-identical to a fresh decode). benchstat (8552595443, 10x x10) vs P1.13: sec/op 0.796 -> 0.766 -3.71% (p=0.000) B/op 398.6Mi -> 378.1Mi -5.14% allocs/op 10.65M -> 10.26M -3.61% Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 6 +++++- class.go | 4 ++++ entity.go | 15 ++++++++++++--- field_state.go | 15 +++++++++++++++ parser.go | 26 ++++++++++++++------------ 5 files changed, 50 insertions(+), 16 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 78db033..6007150 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -299,7 +299,10 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 (Entities.java:655-676, NestedArrayEntityState.java:28-43,219-224). - **Impact:** net win = clone cheaper than re-decode (it is, especially after P3.1 enables cheap COW). Golden-critical: cloned+overlaid state must be value-identical to today's fresh decode. -- **Result:** _(pending)_ +- **Result:** sec/op 796.0→766.5 ms (−3.71%), B/op 398.6→378.1 MiB (−5.14%), allocs/op 10.65M→10.26M + (−3.61%, −0.39M). Baseline decoded once per class into a template, `clone()`d per entity (shares + immutable leaf values, deep-copies nested states). Templates invalidated (`clear`) on instancebaseline + update for correctness. go test green. ✅ ### P-guard — zero-copy `readBytes` invariant (correctness guard, do alongside P1.10/P1.11) - `readBytes` aligned path returns `r.buf[r.pos-n:r.pos]` aliasing the protobuf buffer (reader.go:81); @@ -464,3 +467,4 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P1.11 varints from accumulator | _skipped (breaks reader pos contract)_ | — | — | — | | **P1.12 flatten huffman tree** | **0.960 s (−37.0%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | | **P1.13 8-bit op lookup table** | **0.796 s (−47.7%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | +| **P1.14 baseline decode-once clone** | **0.766 s (−49.7%)** | **378.1 MiB (−52.2%)** | **10.26M (−50.6%)** | PASS | diff --git a/class.go b/class.go index 13a14b0..67d586a 100644 --- a/class.go +++ b/class.go @@ -109,4 +109,8 @@ func (p *Parser) updateInstanceBaseline() { } p.classBaselines[classId] = item.Value } + + // Decoded baseline templates may now be stale; drop them so they are + // re-decoded lazily from the updated bytes on the next entity creation. + clear(p.classBaselineStates) } diff --git a/entity.go b/entity.go index f2cb1fc..ea3a7a8 100644 --- a/entity.go +++ b/entity.go @@ -259,14 +259,23 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error _panicf("unable to find new class %d", classId) } - baseline := p.classBaselines[classId] + // Decode the class baseline once into a reusable template and + // clone it for each new entity, instead of re-decoding the raw + // baseline bytes on every creation. + baseline := p.classBaselineStates[classId] if baseline == nil { - _panicf("unable to find new baseline %d", classId) + raw := p.classBaselines[classId] + if raw == nil { + _panicf("unable to find new baseline %d", classId) + } + baseline = newFieldState() + p.fpBuf = readFields(newReader(raw), class.serializer, baseline, p.fpBuf) + p.classBaselineStates[classId] = baseline } e = newEntity(index, serial, class) + e.state = baseline.clone() p.entities[index] = e - p.fpBuf = readFields(newReader(baseline), class.serializer, e.state, p.fpBuf) p.fpBuf = readFields(r, class.serializer, e.state, p.fpBuf) op = EntityOpCreated | EntityOpEntered diff --git a/field_state.go b/field_state.go index 1b2e956..63ee6ea 100644 --- a/field_state.go +++ b/field_state.go @@ -52,6 +52,21 @@ func (s *fieldState) set(fp *fieldPath, v interface{}) { } } +// clone returns a deep copy of the fieldState. Leaf values are shared (they are +// immutable once decoded), while nested fieldStates are copied recursively so an +// entity cloned from a baseline template can be mutated independently of the +// template and its siblings. +func (s *fieldState) clone() *fieldState { + c := &fieldState{state: make([]interface{}, len(s.state))} + copy(c.state, s.state) + for i, v := range c.state { + if sub, ok := v.(*fieldState); ok { + c.state[i] = sub.clone() + } + } + return c +} + func max(a, b int) int { if a > b { return a diff --git a/parser.go b/parser.go index 3f740e1..6718d06 100644 --- a/parser.go +++ b/parser.go @@ -32,6 +32,7 @@ type Parser struct { AfterStopCallback func() classBaselines map[int32][]byte + classBaselineStates map[int32]*fieldState classesById map[int32]*class classesByName map[string]*class classIdSize uint32 @@ -70,18 +71,19 @@ func NewStreamParser(r io.Reader) (*Parser, error) { NetTick: 0, GameBuild: 0, - classBaselines: make(map[int32][]byte), - classesById: make(map[int32]*class), - classesByName: make(map[string]*class), - entities: make(map[int32]*Entity), - entityHandlers: make([]EntityHandler, 0), - gameEventHandlers: make(map[string][]GameEventHandler), - gameEventNames: make(map[int32]string), - gameEventTypes: make(map[string]*gameEventType), - isStopping: false, - serializers: make(map[string]*serializer), - stream: newStream(r), - stringTables: newStringTables(), + classBaselines: make(map[int32][]byte), + classBaselineStates: make(map[int32]*fieldState), + classesById: make(map[int32]*class), + classesByName: make(map[string]*class), + entities: make(map[int32]*Entity), + entityHandlers: make([]EntityHandler, 0), + gameEventHandlers: make(map[string][]GameEventHandler), + gameEventNames: make(map[int32]string), + gameEventTypes: make(map[string]*gameEventType), + isStopping: false, + serializers: make(map[string]*serializer), + stream: newStream(r), + stringTables: newStringTables(), } // Parse out the header, ensuring that it's valid. From a0eeecfc8c5c17455ed1697dc8427ce19cb76caa Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 11:19:13 -0500 Subject: [PATCH 17/39] test(reader): guard zero-copy readBytes invariant Add TestReaderReadBytesZeroCopy asserting a byte-aligned readBytes returns a slice aliasing the underlying buffer (not a copy), both when bitCount==0 and through the word reader's realign path. CDemoPacket pendingMessage parsing and sendtables rely on this aliasing; the test prevents a future reader change from silently turning it into a copy. go test ./... green. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 +++++ reader_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 6007150..603e697 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -309,6 +309,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 `demo_packet.go` stores these into `pendingMessage` and parses them **later**, and sendtable/string reads depend on the aliasing. A clarity-style padded-copy reader would corrupt this. Add an explicit invariant + test that byte-aligned `readBytes` stays zero-copy after the word-reader rewrite. +- **Result:** Added `TestReaderReadBytesZeroCopy` — verifies byte-aligned `readBytes` aliases the buffer + (zero-copy) both at `bitCount==0` and through the word reader's `realign` path (non-zero multiple of 8). + Test-only; bench unchanged. The realign mechanism itself landed in P1.10. go test green. ✅ --- @@ -468,3 +471,5 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **P1.12 flatten huffman tree** | **0.960 s (−37.0%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | | **P1.13 8-bit op lookup table** | **0.796 s (−47.7%)** | 398.6 MiB (−49.6%) | 10.65M (−48.7%) | PASS | | **P1.14 baseline decode-once clone** | **0.766 s (−49.7%)** | **378.1 MiB (−52.2%)** | **10.26M (−50.6%)** | PASS | +| P-guard zero-copy readBytes test | test-only (no perf change) | — | — | PASS | +| **Phase 1 total vs P0** | **−49.7%** | **−52.2%** | **−50.6%** | PASS | diff --git a/reader_test.go b/reader_test.go index 332d6f4..92a528e 100644 --- a/reader_test.go +++ b/reader_test.go @@ -102,6 +102,45 @@ func TestReaderUnaligned(t *testing.T) { assert.Equal(uint32(0x01), r.readBits(1)) } +// TestReaderReadBytesZeroCopy guards the invariant that a byte-aligned readBytes +// returns a slice aliasing the underlying buffer rather than a copy. Callers such +// as CDemoPacket pendingMessage parsing and sendtables rely on this aliasing, and +// the word-at-a-time reader must preserve it (via realign) even when it has +// buffered bytes ahead in the accumulator. +func TestReaderReadBytesZeroCopy(t *testing.T) { + buf := make([]byte, 32) + for i := range buf { + buf[i] = byte(i + 1) + } + + // Aligned read at the start of the buffer (bitCount == 0). + r := newReader(buf) + got := r.readBytes(4) + idx := r.pos - 4 + buf[idx] ^= 0xFF + if got[0] != buf[idx] { + t.Fatalf("aligned readBytes returned a copy, not a zero-copy alias") + } + + // A bit read first buffers a whole word ahead; the following byte-aligned + // readBytes must realign and still alias the buffer (bitCount a non-zero + // multiple of 8). + for i := range buf { + buf[i] = byte(i + 1) + } + r = newReader(buf) + _ = r.readBits(8) + if r.bitCount == 0 || r.bitCount%8 != 0 { + t.Fatalf("expected non-zero byte alignment after readBits(8), bitCount=%d", r.bitCount) + } + got = r.readBytes(4) + idx = r.pos - 4 + buf[idx] ^= 0xFF + if got[0] != buf[idx] { + t.Fatalf("realigned readBytes returned a copy, not a zero-copy alias") + } +} + func BenchmarkReadVarUint32(b *testing.B) { r := newReader([]byte{0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x0F, 0x8C, 0x01}) b.ResetTimer() From 5d02c356892bcae6682e21b4280ee978e12401c4 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:49:00 -0500 Subject: [PATCH 18/39] fix(stringtable): additive non-increment index (was absolute) The non-increment branch computed an absolute index (= varuint+1) where the Source 2 format (and clarity's S2StringTableEmitter, and manta's own entity decoder) use an additive delta (+= varuint+2). The absolute form produced wrong, non-monotonic indices for delta-updated tables such as ActiveModifiers (thousands of mis-indexed entries on a typical replay). This was invisible to the existing tests because none resolve a delta-updated table by index. go test ./... green (identical golden values; init stays -1 so the increment path is unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 3 ++- string_table.go | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 603e697..e124a7f 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -326,7 +326,8 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 indices (all in ActiveModifiers; e.g. manta=11 where correct=50). Invisible to goldens only because no test resolves a delta-updated table by index. Applying the one-line fix: `TestParseStringTable*` and 5 golden `TestMatch*` all pass with identical values. Init stays `-1`; do **not** switch to `readUBitVar`. -- **Result:** _(pending)_ +- **Result:** Applied the one-line additive fix. Full `go test ./...` green — all golden values identical + (no test resolves a delta-updated table by index). Fixes the latent ActiveModifiers mis-indexing. ✅ ### P2.2 — string-table: fail loud - `parseStringTable` defers `recover()` and returns partial `items` silently (string_table.go:181-186); diff --git a/string_table.go b/string_table.go index 0d3badf..af134c8 100644 --- a/string_table.go +++ b/string_table.go @@ -223,7 +223,12 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b if incr { index++ } else { - index = int32(r.readVarUint32()) + 1 + // The non-increment delta is additive (relative to the running + // index), matching the S2 entity decoder (see entity.go) and + // clarity's S2StringTableEmitter. The previous absolute form + // (= varuint+1) produced wrong, non-monotonic indices for + // delta-updated tables such as ActiveModifiers. + index += int32(r.readVarUint32()) + 2 } // Some values have keys, some don't. From cd44c63c124e3f2c3c36b4d4f91b20c2f84e5b06 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:51:19 -0500 Subject: [PATCH 19/39] fix(stringtable): surface parse errors instead of swallowing them parseStringTable deferred a recover() that logged and returned a partially populated table, so callers saw success and applied incomplete state. Return the error and propagate it from onCSVCMsg_Create/UpdateStringTable, matching clarity's fail-loud behaviour. recover never fires on healthy replays. Adds TestParseStringTableTruncated. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- string_table.go | 24 ++++++++++++++++-------- string_table_test.go | 15 +++++++++++++-- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index e124a7f..73f02ce 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -335,7 +335,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 `error`). `recover` never fires on the bench replay → no healthy-replay regression. Add a truncated-blob unit test. (Also reconcile the inconsistency: `UpdateStringTable` `_panicf`s on a missing table id at string_table.go:139 — harsher than clarity, which skips unknown ids.) -- **Result:** _(pending)_ +- **Result:** `parseStringTable` now returns an error (recover → error); both callers propagate it. Added + `TestParseStringTableTruncated`. go test green (recover never fires on healthy replays). Left the + `UpdateStringTable` `_panicf`-on-missing-table as-is (separate behavior call). ✅ ### P2.3 — `getEventKey` off-by-one - `if f.i > len(e.m.GetKeys())` lets `f.i == len` pass → `GetKeys()[f.i]` panics (game_event.go:166). diff --git a/string_table.go b/string_table.go index af134c8..6d6a5c4 100644 --- a/string_table.go +++ b/string_table.go @@ -105,7 +105,10 @@ func (p *Parser) onCSVCMsg_CreateStringTable(m *dota.CSVCMsg_CreateStringTable) } // Parse the items out of the string table data - items := parseStringTable(buf, m.GetNumEntries(), t.name, t.userDataFixedSize, t.userDataSizeBits, t.flags, t.varintBitCounts) + items, err := parseStringTable(buf, m.GetNumEntries(), t.name, t.userDataFixedSize, t.userDataSizeBits, t.flags, t.varintBitCounts) + if err != nil { + return err + } // Insert the items into the table for _, item := range items { @@ -144,7 +147,10 @@ func (p *Parser) onCSVCMsg_UpdateStringTable(m *dota.CSVCMsg_UpdateStringTable) } // Parse the updates out of the string table data - items := parseStringTable(m.GetStringData(), m.GetNumChangedEntries(), t.name, t.userDataFixedSize, t.userDataSizeBits, t.flags, t.varintBitCounts) + items, err := parseStringTable(m.GetStringData(), m.GetNumChangedEntries(), t.name, t.userDataFixedSize, t.userDataSizeBits, t.flags, t.varintBitCounts) + if err != nil { + return err + } // Apply the updates to the parser state for _, item := range items { @@ -177,11 +183,13 @@ func (p *Parser) onCSVCMsg_UpdateStringTable(m *dota.CSVCMsg_UpdateStringTable) } // Parse a string table data blob, returning a list of item updates. -func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed bool, userDataSizeBits int32, flags int32, varintBitCounts bool) (items []*stringTableItem) { +func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed bool, userDataSizeBits int32, flags int32, varintBitCounts bool) (items []*stringTableItem, err error) { + // Surface a decode failure instead of silently returning a partially + // populated table, matching clarity's fail-loud behaviour. On healthy + // replays this never fires. defer func() { - if err := recover(); err != nil { - _debugf("warning: unable to parse string table %s: %s", name, err) - return + if r := recover(); r != nil { + err = _errorf("unable to parse string table %s: %v", name, r) } }() @@ -199,7 +207,7 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b // Some tables have no data if len(buf) == 0 { - return items + return items, nil } // Loop through entries in the data structure @@ -297,5 +305,5 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b items = append(items, &stringTableItem{index, key, value}) } - return items + return items, nil } diff --git a/string_table_test.go b/string_table_test.go index 2212af4..9078ff0 100644 --- a/string_table_test.go +++ b/string_table_test.go @@ -135,7 +135,8 @@ func TestParseStringTableCreate(t *testing.T) { assert.Equal(s.tableName, m.GetName(), s.tableName) // Parse the table data - items := parseStringTable(buf, m.GetNumEntries(), "", m.GetUserDataFixedSize(), m.GetUserDataSize(), m.GetFlags(), false) + items, err := parseStringTable(buf, m.GetNumEntries(), "", m.GetUserDataFixedSize(), m.GetUserDataSize(), m.GetFlags(), false) + assert.NoError(err) // Make sure we have the correct number of entries assert.Equal(s.itemCount, len(items), s.tableName) @@ -154,7 +155,8 @@ func TestParseStringTableUpdate(t *testing.T) { assert := assert.New(t) buf := _read_fixture("string_tables/updates/tick_03960_table_7_items_13_size_208") - items := parseStringTable(buf, 13, "", false, 0, 0, false) + items, err := parseStringTable(buf, 13, "", false, 0, 0, false) + assert.NoError(err) assert.Equal(int32(261), items[0].Index) assert.Equal("broodmother_spawn_spiderlings", items[0].Key) @@ -163,3 +165,12 @@ func TestParseStringTableUpdate(t *testing.T) { assert.Equal(int32(263), items[2].Index) assert.Equal("broodmother_incapacitating_bite", items[2].Key) } + +// TestParseStringTableTruncated verifies that a blob which decodes past the end +// of its buffer surfaces an error instead of silently returning a partial table. +func TestParseStringTableTruncated(t *testing.T) { + _, err := parseStringTable([]byte{0x07}, 1, "test", false, 0, 0, false) + if err == nil { + t.Fatal("expected an error from a truncated string table, got nil") + } +} From e5e388152028f34302ab7010cb48db598eb5e3a3 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:51:59 -0500 Subject: [PATCH 20/39] fix(gameevent): correct off-by-one bound in getEventKey getEventKey guarded with `f.i > len(keys)`, allowing f.i == len(keys) to fall through to GetKeys()[f.i] and panic. Use >= so a version-skewed event that advertises more fields than it carries returns the intended error instead. Well-formed events never hit this. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 2 +- game_event.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 73f02ce..9fe80c9 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -342,7 +342,7 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 ### P2.3 — `getEventKey` off-by-one - `if f.i > len(e.m.GetKeys())` lets `f.i == len` pass → `GetKeys()[f.i]` panics (game_event.go:166). Change to `>=`. One char; zero golden risk (well-formed descriptors never hit it). -- **Result:** _(pending)_ +- **Result:** Changed `>` to `>=` (game_event.go). go test green; zero golden change. ✅ ### P2.4 — modifier: skip empty/deleted entries - `emitModifierTableEvents` unmarshals every item including empty (`Value == []byte{}`), raising all-zero diff --git a/game_event.go b/game_event.go index 865090c..e8403d6 100644 --- a/game_event.go +++ b/game_event.go @@ -163,7 +163,7 @@ func (e *GameEvent) getEventKey(name string) (*dota.CMsgSource1LegacyGameEventKe return nil, _errorf("field %s: missing", name) } - if f.i > len(e.m.GetKeys()) { + if f.i >= len(e.m.GetKeys()) { return nil, _errorf("field %s: %d out of range", name, f.i) } From 66d7cd474fb7f54f1527207a18874fd21630aa35 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:52:37 -0500 Subject: [PATCH 21/39] fix(modifier): skip empty/deleted ActiveModifiers entries emitModifierTableEvents unmarshalled every entry including empty (deleted) ones, raising an all-zero CDOTAModifierBuffTableEntry to handlers. Skip entries with no value, matching clarity. A real modifier is never zero-length on the wire. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 3 ++- modifier.go | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 9fe80c9..79aec27 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -348,7 +348,8 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 - `emitModifierTableEvents` unmarshals every item including empty (`Value == []byte{}`), raising all-zero messages to handlers. Add `if len(item.Value) == 0 { continue }` (modifier.go:19), matching clarity `if (value != null)`. A real new modifier is never zero-length on the wire. -- **Result:** _(pending)_ +- **Result:** Added the empty-value skip. go test green; golden-neutral (the suite registers no modifier + handler). ✅ ### P2.5 — combat-log `Type()/TypeName()/String()` descriptor-driven - These hardcode `keys[0].GetValByte()` (game_event.go:37,41,46), bypassing the descriptor field map. Resolve diff --git a/modifier.go b/modifier.go index 79e7a45..b64408d 100644 --- a/modifier.go +++ b/modifier.go @@ -24,6 +24,13 @@ func (p *Parser) emitModifierTableEvents(items []*stringTableItem) error { } for _, item := range items { + // Skip deleted/empty entries (clarity does the same). An empty value + // would otherwise unmarshal into an all-zero message and be emitted as a + // spurious modifier event. A real modifier is never zero-length on wire. + if len(item.Value) == 0 { + continue + } + msg := &dota.CDOTAModifierBuffTableEntry{} if err := proto.NewBuffer(item.Value).Unmarshal(msg); err != nil { _debugf("unable to unmarshal ModifierBuffTableEntry: %s", err) From eaf131f45d5a1669a51c2203055fb94eb27cd228 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:54:11 -0500 Subject: [PATCH 22/39] fix(gameevent): resolve combat-log type via descriptor, not key[0] Type(), TypeName() and String() hardcoded keys[0].GetValByte() for the combat log type, assuming it is always the first key and always byte-encoded. Resolve it via the descriptor (GetInt32("type")) instead, which tolerates a different key order or int encoding across game versions. On current descriptors 'type' is index 0, so output is unchanged; the combat-log test already asserts GetInt32("type") succeeds on every event. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- game_event.go | 11 +++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 79aec27..c9597e4 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -356,7 +356,9 @@ pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2 `type` via the descriptor (clarity S1CombatLogIndices.java:8) and route through `GetInt32`-style dispatch (not raw `GetValByte`). On current descriptors `type` is index 0 → output unchanged; these methods aren't called by any manta source/test → zero golden risk. **Fix all three** (the review noted `String()` too). -- **Result:** _(pending)_ +- **Result:** All three now resolve `type` via `GetInt32("type")` (descriptor + typed dispatch). go test + green — and `GetInt32("type")` is golden-guarded (the combat-log test asserts it returns no error on + every event), so this is value-identical on current descriptors. ✅ ### P2.6 — decoder forward-compat additions (never-occur today → golden-neutral) Verified zero occurrences across all 39 build replays, so safe insurance: diff --git a/game_event.go b/game_event.go index e8403d6..24c5c06 100644 --- a/game_event.go +++ b/game_event.go @@ -34,17 +34,20 @@ type GameEvent struct { } func (ge *GameEvent) TypeName() string { - return dota.DOTA_COMBATLOG_TYPES_name[ge.m.GetKeys()[0].GetValByte()] + return dota.DOTA_COMBATLOG_TYPES_name[int32(ge.Type())] } func (ge *GameEvent) Type() dota.DOTA_COMBATLOG_TYPES { - return dota.DOTA_COMBATLOG_TYPES(ge.m.GetKeys()[0].GetValByte()) + // Resolve the type via the descriptor rather than assuming it is the first + // key, and via the typed-int dispatch rather than a raw byte read, so it + // stays correct if the key order or encoding changes across versions. + t, _ := ge.GetInt32("type") + return dota.DOTA_COMBATLOG_TYPES(t) } func (ge *GameEvent) String() string { keys := ge.m.GetKeys() - name := dota.DOTA_COMBATLOG_TYPES_name[keys[0].GetValByte()] - buf := bytes.NewBufferString("\n " + name + "\n") + buf := bytes.NewBufferString("\n " + ge.TypeName() + "\n") for name, field := range ge.t.fields { key := keys[field.i] From ca3fadea972e0025ae9a6ae817f1e98d1a94e4ea Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:57:03 -0500 Subject: [PATCH 23/39] feat(decoder): forward-compat decoders + 64-bit int64 Add decoders for base types clarity handles that manta lacked and that would otherwise fall through to the wrong default decoder (and desync) if a future build networked them: CUtlBinaryBlock (length-prefixed bytes), Quaternion (4-component vector), ResourceId_t (unsigned 64), CGlobalSymbol (string). None occur in the test corpus, so behavior is unchanged today. Also decode int64 with a full 64-bit varint (was 32-bit, which truncates large values). Only m_nTotalDamageTaken is int64 today and is <=1 byte, so its value is unchanged; note Get on an int64 field now returns int64 rather than int32. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 6 +++++- field_decoder.go | 21 ++++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index c9597e4..e8c1f18 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -371,7 +371,11 @@ Verified zero occurrences across all 39 build replays, so safe insurance: (see P2.7 for the value-changing nuance). **Drop** `CBaseVRHandAttachmentHandle` (already correct — no-op). - Place each in the correct map; note `findDecoderByBaseType` (variable-array childDecoder) consults only `fieldTypeDecoders`, not factories. -- **Result:** _(pending)_ +- **Result:** Added `CUtlBinaryBlock` (varint+bytes), `Quaternion` (vectorFactory4), `ResourceId_t` + (unsigned64), `CGlobalSymbol` (string), and `int64`→`signed64Decoder` (full 64-bit varint). The first + four never occur on the corpus (zero behavior change). `int64` affects only `m_nTotalDamageTaken` + (~175k decodes, ≤1 byte today so the value is unchanged, but `Get` now returns `int64` not `int32`). + Dropped CBaseVRHandAttachmentHandle (already correct). go test green (identical golden values). ✅ ### P2.7 — HSequence / HeroID_t decode parity (value-changing, suite-gated) - `HSequence` is REAL (644k decodes): manta stores `value`, clarity `value-1` → off-by-one on every diff --git a/field_decoder.go b/field_decoder.go index e2d15a5..fcf50cf 100644 --- a/field_decoder.go +++ b/field_decoder.go @@ -14,6 +14,7 @@ var fieldTypeFactories = map[string]fieldFactory{ "Vector2D": vectorFactory(2), "Vector4D": vectorFactory(4), "VectorWS": vectorFactory(3), + "Quaternion": vectorFactory(4), "uint64": unsigned64Factory, "QAngle": qangleFactory, "CHandle": unsignedFactory, @@ -29,7 +30,7 @@ var fieldTypeDecoders = map[string]fieldDecoder{ "color32": unsignedDecoder, "int16": signedDecoder, "int32": signedDecoder, - "int64": signedDecoder, + "int64": signed64Decoder, "int8": signedDecoder, "uint16": unsignedDecoder, "uint32": unsignedDecoder, @@ -47,6 +48,9 @@ var fieldTypeDecoders = map[string]fieldDecoder{ "CUtlString": stringDecoder, "CUtlStringToken": unsignedDecoder, "CUtlSymbolLarge": stringDecoder, + "CUtlBinaryBlock": cUtlBinaryBlockDecoder, + "CGlobalSymbol": stringDecoder, + "ResourceId_t": unsigned64Decoder, } func unsignedFactory(f *field) fieldDecoder { @@ -130,6 +134,21 @@ func signedDecoder(r *reader) interface{} { return r.readVarInt32() } +func signed64Decoder(r *reader) interface{} { + return r.readVarInt64() +} + +// cUtlBinaryBlockDecoder reads a length-prefixed binary blob (varint length +// followed by that many bytes), matching clarity's CUtlBinaryBlockDecoder. The +// bytes are copied so the stored value does not alias the transient read buffer. +func cUtlBinaryBlockDecoder(r *reader) interface{} { + n := r.readVarUint32() + b := r.readBytes(n) + out := make([]byte, len(b)) + copy(out, b) + return out +} + func floatCoordDecoder(r *reader) interface{} { return r.readCoord() } From ec1cb0f3e5fb82fecca253f16ca28c3c6dd2f863 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:58:08 -0500 Subject: [PATCH 24/39] fix(decoder): align HSequence and HeroID_t with clarity HSequence had no decoder and fell through to an unsigned varint; clarity decodes it as varint-1 (a sequence handle, -1 for none). HeroID_t likewise fell through to unsigned; clarity uses a signed varint. Both consume the same bits as before (no desync) but produce the clarity-correct values. Neither field is asserted by the golden tests, which stay green (incl. builds 6600/6601 where these fields appear). HSequence is decoded as a signed int32 so wire value 0 becomes -1 rather than wrapping. go test ./... green (identical golden assertions). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- field_decoder.go | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index e8c1f18..be3d295 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -383,7 +383,9 @@ Verified zero occurrences across all 39 build replays, so safe insurance: different value for negative ids). Both change consumer-visible output but no golden asserts them. Decide storage signedness (HSequence `value-1` underflows if stored unsigned at value 0). **Run the full suite incl. 6600/6601 replays; accept only if all green.** -- **Result:** _(pending)_ +- **Result:** Full suite green (incl. 6600/6601 replays). HSequence → `int32(varuint)-1`, HeroID_t → + signed varint. Same bits consumed (no desync); neither is asserted, so goldens are identical. Changes + live consumer-visible values to match clarity. ✅ ### P2.8 — BloodType fixed-8 decode (risky) - `m_nBloodType` (109k× on builds 6600/6601, **in the golden suite**) currently a varint; clarity reads a diff --git a/field_decoder.go b/field_decoder.go index fcf50cf..21b6c3c 100644 --- a/field_decoder.go +++ b/field_decoder.go @@ -38,6 +38,8 @@ var fieldTypeDecoders = map[string]fieldDecoder{ "GameTime_t": noscaleDecoder, "HeroFacetKey_t": unsigned64Decoder, + "HeroID_t": signedDecoder, + "HSequence": hSequenceDecoder, "BloodType": unsignedDecoder, "CBodyComponent": componentDecoder, @@ -138,6 +140,13 @@ func signed64Decoder(r *reader) interface{} { return r.readVarInt64() } +// hSequenceDecoder decodes a sequence handle as an unsigned varint minus one, +// matching clarity's IntMinusOneDecoder. It returns a signed int32 so the +// "none" handle (wire value 0) is -1 rather than wrapping to a large unsigned. +func hSequenceDecoder(r *reader) interface{} { + return int32(r.readVarUint32()) - 1 +} + // cUtlBinaryBlockDecoder reads a length-prefixed binary blob (varint length // followed by that many bytes), matching clarity's CUtlBinaryBlockDecoder. The // bytes are copied so the stored value does not alias the transient read buffer. From fa4170d101d7f1d8a47a7c057d910481173d6e23 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 13:59:17 -0500 Subject: [PATCH 25/39] fix(decoder): decode BloodType as a fixed 8-bit value m_nBloodType was decoded as an unsigned varint; clarity reads a fixed 8-bit value. These agree bit-for-bit only while the value is < 128. The full golden suite (including builds 6600/6601 where m_nBloodType is decoded ~109k times) stays green, confirming every value in the corpus is < 128, so this is a safe forward-compat alignment that also reads correctly for future values >= 128. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 3 ++- field_decoder.go | 9 ++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index be3d295..09370a4 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -391,7 +391,8 @@ Verified zero occurrences across all 39 build replays, so safe insurance: - `m_nBloodType` (109k× on builds 6600/6601, **in the golden suite**) currently a varint; clarity reads a fixed 8-bit. Identical bits only when value < 128; if any ≥127, widths differ → desync → broken goldens. **Suite-gated:** apply only if all goldens (esp. 6600/6601) stay green; otherwise drop. -- **Result:** _(pending)_ +- **Result:** **KEPT** — full suite green, so every `m_nBloodType` in the corpus is < 128 (fixed-8 ≡ + varint, no desync). Now matches clarity's encoding and is correct for future values ≥ 128. ✅ ### P2.9 — QAngle precise/32/0-bit forward-compat - `qangle_pitch_yaw` doesn't handle bitCount ∈ {0,32} (raw float); no `qangle_precise` (20-bit) handling → diff --git a/field_decoder.go b/field_decoder.go index 21b6c3c..f3dfed6 100644 --- a/field_decoder.go +++ b/field_decoder.go @@ -40,7 +40,7 @@ var fieldTypeDecoders = map[string]fieldDecoder{ "HeroFacetKey_t": unsigned64Decoder, "HeroID_t": signedDecoder, "HSequence": hSequenceDecoder, - "BloodType": unsignedDecoder, + "BloodType": bloodTypeDecoder, "CBodyComponent": componentDecoder, "CGameSceneNodeHandle": unsignedDecoder, @@ -140,6 +140,13 @@ func signed64Decoder(r *reader) interface{} { return r.readVarInt64() } +// bloodTypeDecoder reads a fixed 8-bit value, matching clarity's +// IntUnsignedDecoder(8). This is bit-identical to an unsigned varint only while +// the value is < 128; the full golden suite gates that this holds on the corpus. +func bloodTypeDecoder(r *reader) interface{} { + return uint64(r.readBits(8)) +} + // hSequenceDecoder decodes a sequence handle as an unsigned varint minus one, // matching clarity's IntMinusOneDecoder. It returns a signed int32 so the // "none" handle (wire value 0) is -1 rather than wrapping to a large unsigned. From 647a5bab9722cdcb10653dc9d7ceb37988e7caf4 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 14:01:50 -0500 Subject: [PATCH 26/39] feat(decoder): handle QAngle precise/noscale/pitch-yaw raw-float cases Rewrite qangleFactory to match clarity's QAngle decoders: qangle_pitch_yaw with bit count 0 or 32 carries raw 32-bit floats (not bit angles); qangle_precise reads three flagged 20-bit angles; and a general bit count of 32 reads three raw floats. Fields present in the corpus (bit counts 8/13, and the no-bit-count coord form) keep identical behavior, so these are dormant forward-compat paths for newer engine variants. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- field_decoder.go | 58 +++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 09370a4..6f671fe 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -398,7 +398,9 @@ Verified zero occurrences across all 39 build replays, so safe insurance: - `qangle_pitch_yaw` doesn't handle bitCount ∈ {0,32} (raw float); no `qangle_precise` (20-bit) handling → would fall to the coord path and desync. Zero occurrences on the corpus (observed bitcounts {0,8,13}) → pure forward-compat. Add the clarity special-cases (QAnglePitchYawOnly/Precise/NoScale decoders). -- **Result:** _(pending)_ +- **Result:** Rewrote `qangleFactory` from clarity's actual decoders: `qangle_pitch_yaw` bc∈{0,32}→raw + floats; `qangle_precise`→3 flags + 20-bit angles; general bc==32→raw floats. Current fields (bc {8,13}) + keep identical behavior. go test green; the new branches are dormant on the corpus (forward-compat). ✅ ### P2.10 — mana/runetime patch sentinel guards - Mana (builds ≤954) and simtime/runetime (all builds) patches apply unconditionally (field_patch.go:51-78); diff --git a/field_decoder.go b/field_decoder.go index f3dfed6..9d99466 100644 --- a/field_decoder.go +++ b/field_decoder.go @@ -182,24 +182,66 @@ func simulationTimeDecoder(r *reader) interface{} { } func qangleFactory(f *field) fieldDecoder { + bc := uint32(0) + if f.bitCount != nil { + bc = uint32(*f.bitCount) + } + if f.encoder == "qangle_pitch_yaw" { - n := uint32(*f.bitCount) + // Bit counts 0 and 32 carry raw 32-bit floats; otherwise bit angles. + if bc == 0 || bc == 32 { + return func(r *reader) interface{} { + return []float32{ + math.Float32frombits(r.readBits(32)), + math.Float32frombits(r.readBits(32)), + 0.0, + } + } + } return func(r *reader) interface{} { return []float32{ - r.readAngle(n), - r.readAngle(n), + r.readAngle(bc), + r.readAngle(bc), 0.0, } } } - if f.bitCount != nil && *f.bitCount != 0 { - n := uint32(*f.bitCount) + if f.encoder == "qangle_precise" { + return func(r *reader) interface{} { + ret := make([]float32, 3) + rX := r.readBoolean() + rY := r.readBoolean() + rZ := r.readBoolean() + if rX { + ret[0] = r.readAngle(20) + } + if rY { + ret[1] = r.readAngle(20) + } + if rZ { + ret[2] = r.readAngle(20) + } + return ret + } + } + + if bc == 32 { + return func(r *reader) interface{} { + return []float32{ + math.Float32frombits(r.readBits(32)), + math.Float32frombits(r.readBits(32)), + math.Float32frombits(r.readBits(32)), + } + } + } + + if bc != 0 { return func(r *reader) interface{} { return []float32{ - r.readAngle(n), - r.readAngle(n), - r.readAngle(n), + r.readAngle(bc), + r.readAngle(bc), + r.readAngle(bc), } } } From d4e8f57df82d3c364bd30828b380c82ce7f6019a Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 14:03:04 -0500 Subject: [PATCH 27/39] fix(patch): guard mana/runetime patches on the sentinel bounds The mana and runetime field patches applied unconditionally. Guard them on the synthetic full-float-range bounds (+/-MaxFloat32) the engine emits when no real range is set, matching clarity. The affected fields always carry that sentinel today, so behavior is unchanged; the guards just stop the patches from clobbering a field that already has real bounds. Manta keeps its bespoke 4-bit runetime decoder and /30 simtime handling. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- field_patch.go | 18 +++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 6f671fe..8d7b3d0 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -407,7 +407,9 @@ Verified zero occurrences across all 39 build replays, so safe insurance: clarity guards on the sentinel `±3.4028235e38` bounds. Adding the guards is a no-op on the corpus (robustness/parity). **Keep manta's bespoke 4-bit `runeTimeDecoder`** (Outlanders case) and the `/30` simtime API — guards only; touching the runetime decode math risks goldens. -- **Result:** _(pending)_ +- **Result:** Guarded the mana and runetime patches on the `±MaxFloat32` sentinel bounds (clarity-style); + kept the 4-bit runetime decoder and `/30` simtime. These fields always carry the sentinel, so the + guards always fire → go test green, identical goldens. Robustness/parity for edge-of-range fields. ✅ ### P2.11 — outer-message size sanity bound - `readOuterMessage` passes the size varint straight to `stream.readBytes` which does `make([]byte, n)` with diff --git a/field_patch.go b/field_patch.go index 46fd05d..8ca042a 100644 --- a/field_patch.go +++ b/field_patch.go @@ -1,6 +1,8 @@ package manta import ( + "math" + "github.com/golang/protobuf/proto" ) @@ -51,8 +53,12 @@ var fieldPatches = []fieldPatch{ fieldPatch{0, 954, func(f *field) { switch f.varName { case "m_flMana", "m_flMaxMana": - f.lowValue = nil - f.highValue = proto.Float32(8192.0) + // Only override the synthetic full-float-range bounds (the sentinel + // the engine emits when no real range is set), matching clarity. + if f.highValue != nil && *f.highValue == float32(math.MaxFloat32) { + f.lowValue = nil + f.highValue = proto.Float32(8192.0) + } } }}, fieldPatch{1016, 1027, func(f *field) { @@ -73,7 +79,13 @@ var fieldPatches = []fieldPatch{ case "m_flSimulationTime", "m_flAnimTime": f.encoder = "simtime" case "m_flRuneTime": - f.encoder = "runetime" + // Only patch when the field carries the synthetic full-float-range + // bounds, matching clarity's runetime guard. Manta keeps its bespoke + // 4-bit runetime decoder (see field_decoder.go). + if f.lowValue != nil && f.highValue != nil && + *f.lowValue == -float32(math.MaxFloat32) && *f.highValue == float32(math.MaxFloat32) { + f.encoder = "runetime" + } } }}, } From 8bf31cef06ca7471a16028253b453fe9e78cccf1 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 14:04:29 -0500 Subject: [PATCH 28/39] fix(parser): bound outer-message size before allocating readOuterMessage passed the size varint straight to stream.readBytes, which does make([]byte, n) with no upper bound, so a corrupt or truncated stream with a huge varint could attempt a multi-gigabyte allocation. Reject sizes above a generous 256 MiB cap (far above any legitimate message) with an error instead. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 3 ++- parser.go | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 8d7b3d0..b8e4574 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -415,7 +415,8 @@ Verified zero occurrences across all 39 build replays, so safe insurance: - `readOuterMessage` passes the size varint straight to `stream.readBytes` which does `make([]byte, n)` with no bound (parser.go:219, stream.go:26) → a corrupt/huge varint can OOM before `io.ReadFull` errors. Add a max-size guard (safely above the largest legitimate full packet) returning an error. Golden-neutral. -- **Result:** _(pending)_ +- **Result:** Added a 256 MiB `maxOuterMessageSize` guard before `readBytes` (every corpus replay is + ≤70 MB total, so no single message approaches it). go test green; only rejects corrupt sizes. ✅ ### P2.12 — field-path depth-7 guard + comment - `Push*` ops do `fp.last++` then index `fp.path[fp.last]` with no guard → depth>6 panics with a raw diff --git a/parser.go b/parser.go index 6718d06..2fdd228 100644 --- a/parser.go +++ b/parser.go @@ -13,6 +13,11 @@ import ( var magicSource1 = []byte{'P', 'U', 'F', 'D', 'E', 'M', 'S', '\000'} var magicSource2 = []byte{'P', 'B', 'D', 'E', 'M', 'S', '2', '\000'} +// maxOuterMessageSize bounds a single outer message length so a corrupt or +// truncated size varint cannot trigger a huge allocation. It is far above any +// legitimate message, including full packets. +const maxOuterMessageSize = 256 << 20 + // Parser is an instance of the replay parser type Parser struct { // Callbacks provide a mechanism for receiving notification @@ -230,6 +235,12 @@ func (p *Parser) readOuterMessage() (outerMessage, error) { return outerMessage{}, err } + // Reject an implausibly large size before allocating, so a corrupt or + // truncated stream fails cleanly instead of attempting a huge allocation. + if size > maxOuterMessageSize { + return outerMessage{}, _errorf("outer message size %d exceeds maximum %d", size, maxOuterMessageSize) + } + buf, err := p.stream.readBytes(size) if err != nil { return outerMessage{}, err From 6eb5c2c8166ded7745a4415bfb0d19620e07d2bc Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 14:05:06 -0500 Subject: [PATCH 29/39] docs(fieldpath): name the max field-path depth invariant Introduce maxFieldPathDepth (= 7) with a comment citing clarity's S2LongFieldPathFormat, and use it for the fixed path array. The fixed-size array already enforces the bound via a recovered bounds-check panic; this just documents the invariant. No behavior change. go test ./... green (identical golden values). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 +++- field_path.go | 11 +++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index b8e4574..371551b 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -423,7 +423,9 @@ Verified zero occurrences across all 39 build replays, so safe insurance: out-of-range (field_path.go). Clarity bounds at 7 and fails loudly (S2LongFieldPathFormat.java:7-58). Keep 7, add a cheap descriptive guard + comment. No behavior change (nothing exceeds 7 today). **Must stay `[7]int` when P1.9 lands.** -- **Result:** _(pending)_ +- **Result:** Added `maxFieldPathDepth = 7` const + comment citing `S2LongFieldPathFormat`, used for the + `[maxFieldPathDepth]int` path array. The fixed array already fails loudly (recovered bounds-check) on + overflow, so no hot-path guard was added. No behavior change. go test green. ✅ --- diff --git a/field_path.go b/field_path.go index 7870df6..0a5dbe2 100644 --- a/field_path.go +++ b/field_path.go @@ -81,8 +81,15 @@ func flattenHuffmanTree(t huffmanTree) int32 { return idx } +// maxFieldPathDepth is the maximum field-path depth, matching clarity's +// S2LongFieldPathFormat (whose BITS_PER_COMPONENT has 7 entries). The fixed-size +// path array enforces this invariant: a deeper push hits a bounds-check panic +// that the parser's top-level recover turns into an error, rather than silently +// corrupting entity state. +const maxFieldPathDepth = 7 + type fieldPath struct { - path [7]int + path [maxFieldPathDepth]int last int done bool } @@ -362,7 +369,7 @@ var fpPool = &sync.Pool{ // reset resets the fieldPath to the empty value func (fp *fieldPath) reset() { - fp.path = [7]int{-1} + fp.path = [maxFieldPathDepth]int{-1} fp.last = 0 fp.done = false } From cfcc6a869fb21dc6a4576ee2e60dbc50c5d30ddf Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Fri, 19 Jun 2026 14:07:09 -0500 Subject: [PATCH 30/39] docs: phase 2 (correctness) complete Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 371551b..cbd4bb5 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -493,3 +493,12 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **P1.14 baseline decode-once clone** | **0.766 s (−49.7%)** | **378.1 MiB (−52.2%)** | **10.26M (−50.6%)** | PASS | | P-guard zero-copy readBytes test | test-only (no perf change) | — | — | PASS | | **Phase 1 total vs P0** | **−49.7%** | **−52.2%** | **−50.6%** | PASS | +| Phase 2 (P2.1–P2.12, correctness) | ~ flat | ~ flat | ~ flat | PASS | +| **End of Phase 2 vs P0** | **−49.7%** | **−52.2%** | **−50.6%** | PASS | + +**Phase 2 (correctness) notes:** all 12 goals landed, full suite green with identical golden assertions, +and no perf regression (sec/op, B/op, allocs/op all statistically flat vs end of Phase 1). Highlights: +P2.1 fixed the real string-table additive-index bug; P2.6–P2.9 added forward-compat decoders +(CUtlBinaryBlock, Quaternion, int64-64bit, HSequence/HeroID_t/BloodType aligned to clarity, QAngle +precise/noscale); P2.2/P2.3/P2.11/P2.12 hardened error paths. P2.7/P2.8 change live field *values* to +match clarity (not asserted by goldens) — flagged for review. From fbca7ed3cbee02765425f0717a47eddaedf7fba3 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Sat, 20 Jun 2026 08:16:00 -0500 Subject: [PATCH 31/39] perf(entity): store field values in a typed cell, box lazily on read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Entity state stored every decoded value boxed in a []interface{}, so each float/int/handle heap-allocated on the write path — per field, per entity, per tick. Replace it with a 24-byte tagged-union cell {ref interface{}; num uint32; kind}: scalars (float32, int32, uint32, bool, and <=32-bit uint64 handles) live inline in num with zero write-path allocation; reference values (string, []float32, []byte) and the rare genuinely-64-bit ints (CStrongHandle, fixed64, int64, steamids) go in ref; nested tables are a *fieldState in ref. Decoders now return a cell; values are boxed into interface{} lazily in cell.iface(), only on read (Entity.Get), which is far rarer than the write path. Public API is unchanged: Entity.Get still returns interface{} with the exact same dynamic type and value as before (a pure storage refactor). go test ./... passes with identical golden assertions, including entity_test's exact-type checks (int32/uint64/bool/string, plus a >2^32 steamid proving 64-bit values are not truncated) and expectHeroEntityMana's float32 round-trip. benchstat (8552595443, 10x x10) vs end of Phase 2: allocs/op 10.264M -> 4.410M -57.03% (p=0.000) sec/op 0.766 -> 0.734 -4.23% B/op 378.1Mi -> 388.2Mi +2.65% (qangle/Vector []float32 left boxed to preserve the []float32 Get contract) Cumulative vs P0 baseline: allocs -78.8%, sec -51.8%, B/op -51.0%. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 14 ++++- field.go | 6 +-- field_decoder.go | 121 ++++++++++++++++++++--------------------- field_reader.go | 2 +- field_state.go | 137 ++++++++++++++++++++++++++++++++++++++--------- 5 files changed, 189 insertions(+), 91 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index cbd4bb5..286b4fd 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -447,7 +447,17 @@ Verified zero occurrences across all 39 build replays, so safe insurance: - **Golden-critical:** `Get("m_flMaxMana").(float32)` (manta_test.go:694) must stay bit-identical — `Get`/ `GetFloat32` must still return a boxed `float32`. Folds in P-decoder findings (quantized/qangle/int boxing) which deliver 0 allocs alone and explicitly depend on this. Also unlocks cheap COW for P1.14. -- **Result:** _(pending)_ +- **Result:** Implemented as a **24-byte tagged-union `cell`** + `{ref interface{}; num uint32; kind cellKind}`. Scalars (float32/int32/uint32/bool/≤32-bit uint64 + handles) live inline in `num` — zero write-path alloc; reference values (string/`[]float32`/`[]byte`) and + the rare genuinely-64-bit ints (CStrongHandle/fixed64/int64/steamids) go in `ref`; nested tables in `ref` + as `*fieldState`. Decoders now return `cell`; values box lazily in `cell.iface()` only on `Get` (rare). + Public API unchanged — `Get` still returns `interface{}` with the exact dynamic type (entity_test's + `int32`/`uint64`/`bool`/`string` assertions and `expectHeroEntityMana` float all pass; the >32-bit steamid + proves 64-bit values aren't truncated). **vs P2: allocs −57.0% (10.26M→4.41M), sec −4.2%, B/op +2.6%** + (residual is the qangle/Vector `[]float32` backing arrays, left boxed to respect no-API-change on `Get`). + Tried a 32-byte `uint64`-num cell first (no 64-bit boxing) but it cost +12.9% B/op; the 24-byte variant + recovers that for just +1% allocs since 64-bit values are rare (profile-confirmed). go test ./... green. ✅ --- @@ -495,6 +505,8 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **Phase 1 total vs P0** | **−49.7%** | **−52.2%** | **−50.6%** | PASS | | Phase 2 (P2.1–P2.12, correctness) | ~ flat | ~ flat | ~ flat | PASS | | **End of Phase 2 vs P0** | **−49.7%** | **−52.2%** | **−50.6%** | PASS | +| P3.1 typed entity state (de-box) | 0.734 s (−4.2% vs P2) | 388.2 MiB (+2.6% vs P2) | 4.41M (−57.0% vs P2) | PASS | +| **End of Phase 3 vs P0** | **−51.8%** | **−51.0%** | **−78.8%** | PASS | **Phase 2 (correctness) notes:** all 12 goals landed, full suite green with identical golden assertions, and no perf regression (sec/op, B/op, allocs/op all statistically flat vs end of Phase 1). Highlights: diff --git a/field.go b/field.go index 629cde5..5337622 100644 --- a/field.go +++ b/field.go @@ -252,7 +252,7 @@ func (f *field) getFieldPaths(fp *fieldPath, state *fieldState) []*fieldPath { if sub, ok := state.get(fp).(*fieldState); ok { fp.last++ for i, v := range sub.state { - if v != nil { + if !v.isEmpty() { fp.path[fp.last] = i x = append(x, fp.copy()) } @@ -271,7 +271,7 @@ func (f *field) getFieldPaths(fp *fieldPath, state *fieldState) []*fieldPath { if sub, ok := state.get(fp).(*fieldState); ok { fp.last++ for i, v := range sub.state { - if v != nil { + if !v.isEmpty() { fp.path[fp.last] = i x = append(x, fp.copy()) } @@ -283,7 +283,7 @@ func (f *field) getFieldPaths(fp *fieldPath, state *fieldState) []*fieldPath { if sub, ok := state.get(fp).(*fieldState); ok { fp.last += 2 for i, v := range sub.state { - if vv, ok := v.(*fieldState); ok { + if vv, ok := v.sub(); ok { fp.path[fp.last-1] = i x = append(x, f.serializer.getFieldPaths(fp, vv)...) } diff --git a/field_decoder.go b/field_decoder.go index 9d99466..2b8bfa4 100644 --- a/field_decoder.go +++ b/field_decoder.go @@ -4,7 +4,7 @@ import ( "math" ) -type fieldDecoder func(*reader) interface{} +type fieldDecoder func(*reader) cell type fieldFactory func(*field) fieldDecoder var fieldTypeFactories = map[string]fieldFactory{ @@ -86,8 +86,8 @@ func floatFactory(f *field) fieldDecoder { func quantizedFactory(f *field) fieldDecoder { qfd := newQuantizedFloatDecoder(f.bitCount, f.encodeFlags, f.lowValue, f.highValue) - return func(r *reader) interface{} { - return qfd.decode(r) + return func(r *reader) cell { + return floatCell(qfd.decode(r)) } } @@ -98,87 +98,87 @@ func vectorFactory(n int) fieldFactory { } d := floatFactory(f) - return func(r *reader) interface{} { + return func(r *reader) cell { x := make([]float32, n) for i := 0; i < n; i++ { - x[i] = d(r).(float32) + x[i] = d(r).asFloat32() } - return x + return refCell(x) } } } -func vectorNormalDecoder(r *reader) interface{} { - return r.read3BitNormal() +func vectorNormalDecoder(r *reader) cell { + return refCell(r.read3BitNormal()) } -func fixed64Decoder(r *reader) interface{} { - return r.readLeUint64() +func fixed64Decoder(r *reader) cell { + return refCell(r.readLeUint64()) } -func handleDecoder(r *reader) interface{} { - return r.readVarUint32() +func handleDecoder(r *reader) cell { + return uint32Cell(r.readVarUint32()) } -func booleanDecoder(r *reader) interface{} { - return r.readBoolean() +func booleanDecoder(r *reader) cell { + return boolCell(r.readBoolean()) } -func stringDecoder(r *reader) interface{} { - return r.readString() +func stringDecoder(r *reader) cell { + return refCell(r.readString()) } -func defaultDecoder(r *reader) interface{} { - return r.readVarUint32() +func defaultDecoder(r *reader) cell { + return uint32Cell(r.readVarUint32()) } -func signedDecoder(r *reader) interface{} { - return r.readVarInt32() +func signedDecoder(r *reader) cell { + return int32Cell(r.readVarInt32()) } -func signed64Decoder(r *reader) interface{} { - return r.readVarInt64() +func signed64Decoder(r *reader) cell { + return refCell(r.readVarInt64()) } // bloodTypeDecoder reads a fixed 8-bit value, matching clarity's // IntUnsignedDecoder(8). This is bit-identical to an unsigned varint only while // the value is < 128; the full golden suite gates that this holds on the corpus. -func bloodTypeDecoder(r *reader) interface{} { - return uint64(r.readBits(8)) +func bloodTypeDecoder(r *reader) cell { + return smallUint64Cell(uint64(r.readBits(8))) } // hSequenceDecoder decodes a sequence handle as an unsigned varint minus one, // matching clarity's IntMinusOneDecoder. It returns a signed int32 so the // "none" handle (wire value 0) is -1 rather than wrapping to a large unsigned. -func hSequenceDecoder(r *reader) interface{} { - return int32(r.readVarUint32()) - 1 +func hSequenceDecoder(r *reader) cell { + return int32Cell(int32(r.readVarUint32()) - 1) } // cUtlBinaryBlockDecoder reads a length-prefixed binary blob (varint length // followed by that many bytes), matching clarity's CUtlBinaryBlockDecoder. The // bytes are copied so the stored value does not alias the transient read buffer. -func cUtlBinaryBlockDecoder(r *reader) interface{} { +func cUtlBinaryBlockDecoder(r *reader) cell { n := r.readVarUint32() b := r.readBytes(n) out := make([]byte, len(b)) copy(out, b) - return out + return refCell(out) } -func floatCoordDecoder(r *reader) interface{} { - return r.readCoord() +func floatCoordDecoder(r *reader) cell { + return floatCell(r.readCoord()) } -func noscaleDecoder(r *reader) interface{} { - return math.Float32frombits(r.readBits(32)) +func noscaleDecoder(r *reader) cell { + return floatCell(math.Float32frombits(r.readBits(32))) } -func runeTimeDecoder(r *reader) interface{} { - return math.Float32frombits(r.readBits(4)) +func runeTimeDecoder(r *reader) cell { + return floatCell(math.Float32frombits(r.readBits(4))) } -func simulationTimeDecoder(r *reader) interface{} { - return float32(r.readVarUint32()) * (1.0 / 30) +func simulationTimeDecoder(r *reader) cell { + return floatCell(float32(r.readVarUint32()) * (1.0 / 30)) } func qangleFactory(f *field) fieldDecoder { @@ -190,25 +190,25 @@ func qangleFactory(f *field) fieldDecoder { if f.encoder == "qangle_pitch_yaw" { // Bit counts 0 and 32 carry raw 32-bit floats; otherwise bit angles. if bc == 0 || bc == 32 { - return func(r *reader) interface{} { - return []float32{ + return func(r *reader) cell { + return refCell([]float32{ math.Float32frombits(r.readBits(32)), math.Float32frombits(r.readBits(32)), 0.0, - } + }) } } - return func(r *reader) interface{} { - return []float32{ + return func(r *reader) cell { + return refCell([]float32{ r.readAngle(bc), r.readAngle(bc), 0.0, - } + }) } } if f.encoder == "qangle_precise" { - return func(r *reader) interface{} { + return func(r *reader) cell { ret := make([]float32, 3) rX := r.readBoolean() rY := r.readBoolean() @@ -222,31 +222,31 @@ func qangleFactory(f *field) fieldDecoder { if rZ { ret[2] = r.readAngle(20) } - return ret + return refCell(ret) } } if bc == 32 { - return func(r *reader) interface{} { - return []float32{ + return func(r *reader) cell { + return refCell([]float32{ math.Float32frombits(r.readBits(32)), math.Float32frombits(r.readBits(32)), math.Float32frombits(r.readBits(32)), - } + }) } } if bc != 0 { - return func(r *reader) interface{} { - return []float32{ + return func(r *reader) cell { + return refCell([]float32{ r.readAngle(bc), r.readAngle(bc), r.readAngle(bc), - } + }) } } - return func(r *reader) interface{} { + return func(r *reader) cell { ret := make([]float32, 3) rX := r.readBoolean() rY := r.readBoolean() @@ -260,24 +260,25 @@ func qangleFactory(f *field) fieldDecoder { if rZ { ret[2] = r.readCoord() } - return ret + return refCell(ret) } } -func vector2Decoder(r *reader) interface{} { - return []float32{r.readFloat(), r.readFloat()} +func vector2Decoder(r *reader) cell { + return refCell([]float32{r.readFloat(), r.readFloat()}) } -func unsignedDecoder(r *reader) interface{} { - return uint64(r.readVarUint32()) +func unsignedDecoder(r *reader) cell { + // readVarUint32 yields at most 32 bits, so the uint64 value fits inline. + return smallUint64Cell(uint64(r.readVarUint32())) } -func unsigned64Decoder(r *reader) interface{} { - return r.readVarUint64() +func unsigned64Decoder(r *reader) cell { + return refCell(r.readVarUint64()) } -func componentDecoder(r *reader) interface{} { - return r.readBits(1) +func componentDecoder(r *reader) cell { + return uint32Cell(r.readBits(1)) } func findDecoder(f *field) fieldDecoder { diff --git a/field_reader.go b/field_reader.go index 877b8d5..1856c54 100644 --- a/field_reader.go +++ b/field_reader.go @@ -40,7 +40,7 @@ func readFields(r *reader, s *serializer, state *fieldState, fpBuf []fieldPath) fp2.release() - _debugf(" => %#v", val) + _debugf(" => %#v", val.iface()) } } diff --git a/field_state.go b/field_state.go index 63ee6ea..8f071f5 100644 --- a/field_state.go +++ b/field_state.go @@ -1,67 +1,152 @@ package manta +import "math" + +// cellKind tags the typed value held by a cell. +type cellKind uint8 + +const ( + cellEmpty cellKind = iota + cellFloat32 + cellInt32 + cellUint32 + cellUint64 // value held inline in num (fits in 32 bits), reproduced as uint64 + cellBool + cellRef // ref holds a string, []float32, []byte, or a boxed 64-bit integer + cellSub // ref holds a *fieldState +) + +// cell is a tagged union holding one decoded field value inline. The common +// scalars (float32, int32, uint32, bool, and 32-bit-range uint64 handles) live +// in num without allocating. Reference values (strings, vectors, binary blobs) +// and the rare genuinely-64-bit integers (CStrongHandle, fixed64, int64) use the +// interface field. Keeping num a uint32 holds the cell to 24 bytes, which keeps +// the per-entity state arrays (cloned from the baseline and grown on write) +// compact. Scalars are boxed into interface{} lazily, on read through iface() +// (e.g. Entity.Get), which is far rarer than the per-field-per-tick write path. +type cell struct { + ref interface{} + num uint32 + kind cellKind +} + +func floatCell(f float32) cell { return cell{num: math.Float32bits(f), kind: cellFloat32} } +func int32Cell(v int32) cell { return cell{num: uint32(v), kind: cellInt32} } +func uint32Cell(v uint32) cell { return cell{num: v, kind: cellUint32} } +func refCell(v interface{}) cell { return cell{ref: v, kind: cellRef} } +func subCell(s *fieldState) cell { return cell{ref: s, kind: cellSub} } + +// smallUint64Cell stores a uint64 whose value is known to fit in 32 bits inline, +// reproduced as a uint64 on read. Used for varint handle types whose wire value +// cannot exceed 32 bits. Genuinely 64-bit values must use refCell instead. +func smallUint64Cell(v uint64) cell { return cell{num: uint32(v), kind: cellUint64} } + +func boolCell(b bool) cell { + c := cell{kind: cellBool} + if b { + c.num = 1 + } + return c +} + +func (c cell) isEmpty() bool { return c.kind == cellEmpty } + +// sub returns the nested fieldState if this cell holds one. +func (c cell) sub() (*fieldState, bool) { + if c.kind == cellSub { + return c.ref.(*fieldState), true + } + return nil, false +} + +// asFloat32 returns the float32 held by a scalar float cell. It is used by the +// vector decoders, which assemble a []float32 from per-component float decoders. +func (c cell) asFloat32() float32 { return math.Float32frombits(c.num) } + +// iface boxes the cell value into an interface{} for the public Get API, +// reproducing the exact dynamic type the decoder originally produced. This is +// the only place a stored scalar is heap-boxed. +func (c cell) iface() interface{} { + switch c.kind { + case cellFloat32: + return math.Float32frombits(c.num) + case cellInt32: + return int32(c.num) + case cellUint32: + return c.num + case cellUint64: + return uint64(c.num) + case cellBool: + return c.num != 0 + case cellRef, cellSub: + return c.ref + } + return nil +} + type fieldState struct { - state []interface{} + state []cell } func newFieldState() *fieldState { return &fieldState{ - state: make([]interface{}, 8), + state: make([]cell, 8), } } func (s *fieldState) get(fp *fieldPath) interface{} { x := s - z := 0 for i := 0; i <= fp.last; i++ { - z = fp.path[i] + z := fp.path[i] if len(x.state) < z+2 { return nil } if i == fp.last { - return x.state[z] + return x.state[z].iface() } - if _, ok := x.state[z].(*fieldState); !ok { + sub, ok := x.state[z].sub() + if !ok { return nil } - x = x.state[z].(*fieldState) + x = sub } return nil } -func (s *fieldState) set(fp *fieldPath, v interface{}) { +func (s *fieldState) set(fp *fieldPath, c cell) { x := s - z := 0 for i := 0; i <= fp.last; i++ { - z = fp.path[i] + z := fp.path[i] if y := len(x.state); y < z+2 { - z := make([]interface{}, max(z+2, y*2)) - copy(z, x.state) - x.state = z + grown := make([]cell, max(z+2, y*2)) + copy(grown, x.state) + x.state = grown } if i == fp.last { - if _, ok := x.state[z].(*fieldState); !ok { - x.state[z] = v + // Do not overwrite an existing sub-table with a leaf value. + if x.state[z].kind != cellSub { + x.state[z] = c } return } - if _, ok := x.state[z].(*fieldState); !ok { - x.state[z] = newFieldState() + if x.state[z].kind != cellSub { + x.state[z] = subCell(newFieldState()) } - x = x.state[z].(*fieldState) + x, _ = x.state[z].sub() } } -// clone returns a deep copy of the fieldState. Leaf values are shared (they are -// immutable once decoded), while nested fieldStates are copied recursively so an -// entity cloned from a baseline template can be mutated independently of the -// template and its siblings. +// clone returns a deep copy of the fieldState. Leaf cells are copied by value +// (their ref values — strings, vectors — are immutable once decoded and may be +// shared), while nested fieldStates are copied recursively so an entity cloned +// from a baseline template can be mutated independently of the template and its +// siblings. func (s *fieldState) clone() *fieldState { - c := &fieldState{state: make([]interface{}, len(s.state))} + c := &fieldState{state: make([]cell, len(s.state))} copy(c.state, s.state) - for i, v := range c.state { - if sub, ok := v.(*fieldState); ok { - c.state[i] = sub.clone() + for i := range c.state { + if sub, ok := c.state[i].sub(); ok { + c.state[i].ref = sub.clone() } } return c From 40da8ccc73073e6bc58d4630802e62b6401c8ca5 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 10:22:59 -0500 Subject: [PATCH 32/39] fix(entity): deep-copy mutable baseline leaves in clone clone() shallow-copied cells and only deep-copied nested fieldStates, so cells whose ref holds a []float32 vector or []byte blob were shared across every entity cloned from the cached class baseline (introduced when baselines became decode-once + cloned). A caller mutating a slice returned from Entity.Get/Map could corrupt the baseline template and sibling entities; before the branch, baselines were re-decoded per entity so this aliasing did not exist. Deep-copy []float32/[]byte leaves in clone(); strings and boxed integers are immutable and stay shared. Only fires for baseline vector/blob leaves. go test ./... green (identical golden values; deep-copy yields equal slices). benchstat (8552595443, 10x x10) vs P4 baseline: allocs/op 4.410M -> 4.478M +1.54% (+68K, the per-entity leaf copies) B/op 388.2Mi -> 389.4Mi +0.33% sec/op ~ (p=0.105) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ field_state.go | 20 +++++++++------ 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 286b4fd..b15bf3a 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -507,6 +507,12 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **End of Phase 2 vs P0** | **−49.7%** | **−52.2%** | **−50.6%** | PASS | | P3.1 typed entity state (de-box) | 0.734 s (−4.2% vs P2) | 388.2 MiB (+2.6% vs P2) | 4.41M (−57.0% vs P2) | PASS | | **End of Phase 3 vs P0** | **−51.8%** | **−51.0%** | **−78.8%** | PASS | +| P4 baseline (re-measured at fbca7ed) | 0.727 s | 388.2 MiB | 4.41M | PASS | +| P4.1 deep-copy mutable baseline leaves | 0.755 s (~) | 389.4 MiB (+0.3%) | 4.478M (+1.5%) | PASS | +| P4.2 clear reused tuple/pending buffers | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.3 guard skipBits underflow | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.4 fix debug position | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.5 lock value-changing decoders (tests+docs) | _tbd_ | _tbd_ | _tbd_ | _tbd_ | **Phase 2 (correctness) notes:** all 12 goals landed, full suite green with identical golden assertions, and no perf regression (sec/op, B/op, allocs/op all statistically flat vs end of Phase 1). Highlights: @@ -514,3 +520,63 @@ P2.1 fixed the real string-table additive-index bug; P2.6–P2.9 added forward-c (CUtlBinaryBlock, Quaternion, int64-64bit, HSequence/HeroID_t/BloodType aligned to clarity, QAngle precise/noscale); P2.2/P2.3/P2.11/P2.12 hardened error paths. P2.7/P2.8 change live field *values* to match clarity (not asserted by goldens) — flagged for review. + +--- + +## Phase 4 — post-review fixes + +Addresses external review feedback on the branch. P4.1/P4.2/P4.3 fix real regressions the branch +introduced; P4.4 is a debug-only cleanup; P4.5 documents and locks the deliberate API changes. +Re-baselined at `fbca7ed`; each step benchstat'd vs the previous so the *cost* of these correctness +fixes is explicit. + +### P4.1 — deep-copy mutable baseline leaves in clone() +- **Issue:** `clone()` (field_state.go) shallow-copies cells and only deep-copies nested `*fieldState`. + Cells whose `ref` holds `[]float32` (vectors) or `[]byte` (binary blobs) are shared across every entity + cloned from the cached class baseline (introduced by P1.14 decode-once + carried through P3.1). A caller + mutating a slice from `Entity.Get`/`Map` can corrupt the baseline template and sibling entities. Before + the branch, baselines were re-decoded per entity, so this aliasing didn't exist. +- **Fix:** type-switch in `clone()` and deep-copy `[]float32`/`[]byte` leaves; strings and boxed 64-bit + scalars are immutable and stay shared. Only fires for baseline vector/blob leaves, at entity-create. +- **Result:** **cost** allocs/op 4.410M→4.478M (+1.54%, +68K — the per-entity baseline leaf copies), + B/op +0.33%, sec/op ~ (p=0.105). go test green, identical goldens (deep-copy yields equal slices). ✅ + +### P4.2 — clear reused tuple/pending buffers after dispatch +- **Issue:** `p.entityTuples` (entity.go) and `p.pendingMsgBuf` (demo_packet.go) are stored back with full + length and stale `[len:cap]` entries, retaining `*Entity` pointers (and their de-boxed state) and inner + packet buffers an extra packet — stale slots from a larger prior packet can pin *deleted* entities. +- **Fix:** `clear()` the used entries and store the slice back at `[:0]`. Clearing the full written length + each packet keeps `[len:cap]` zero across packets, so no stale refs accumulate. +- **Result:** _(pending)_ + +### P4.3 — guard skipBits underflow + truncated-stream test +- **Issue:** `peekBits` zero-pads at EOF and `skipBits` blindly subtracts, so on truncated/corrupt input a + lookup entry can consume more bits than are buffered, underflowing `bitCount` (uint32 wraps huge) and + spinning on garbage ops instead of failing. The old per-bit walk hit a clean panic → parser error. + Cannot affect well-formed replays (the fast path never under-runs on valid streams). The committed + huffman test used 8-byte buffers and never hit the `<8 bits remaining` case. +- **Fix:** `skipBits` panics cleanly when `n > bitCount` (caught by the parser recover → error), restoring + fail-fast. Add a huffman test that truncates an op stream and asserts a clean error. +- **Result:** _(pending)_ + +### P4.4 — correct debug position() after word refill +- **Issue:** `position()` (reader.go) still assumes `bitCount <= 8`, but the word reader can leave 56/48/… + Wrong verbose-debug output only; no parsing impact. +- **Fix:** compute the logical bit position as `pos*8 - bitCount`. +- **Result:** _(pending)_ + +### P4.5 — lock value-changing decoders + document Decision A +- **Decision A (owned):** the branch deliberately changes what `.Get` returns for a few fields, to match + clarity's correct representation. We are keeping these and owning them as intentional behavior changes: + - `int64` fields: `int32` (truncated >32 bits) → **`int64`** (fixes truncation; type change is forced by + the fix). + - `HeroID_t`: `uint32` → **`int32`** (signed, clarity parity). + - `HSequence`: `uint32` → **`int32`, value − 1** (−1 = "none" handle, clarity parity). + - `BloodType`: stayed `uint64`; only the *encoding* changed (fixed-8 vs varint), value identical on the + corpus — not an API change. + Downstream callers doing concrete type assertions on those specific fields may need updates. +- **Fix:** add decoder-level representation tests asserting the exact dynamic type **and** value for each + (deterministic, not replay-dependent), so the intended downstream-visible values are locked in CI — this + is what the review asked for ("prove values are what you intend, not just no-desync"). Also assert the + inline-vs-boxed uint64 split round-trips, including a `> 2^32` value to prove no truncation. +- **Result:** _(pending)_ diff --git a/field_state.go b/field_state.go index 8f071f5..5a5e830 100644 --- a/field_state.go +++ b/field_state.go @@ -136,17 +136,23 @@ func (s *fieldState) set(fp *fieldPath, c cell) { } } -// clone returns a deep copy of the fieldState. Leaf cells are copied by value -// (their ref values — strings, vectors — are immutable once decoded and may be -// shared), while nested fieldStates are copied recursively so an entity cloned -// from a baseline template can be mutated independently of the template and its -// siblings. +// clone returns a deep copy of the fieldState so an entity cloned from a cached +// class baseline can be mutated independently of the template and its siblings. +// Nested fieldStates are copied recursively. Mutable leaf values (vector +// []float32 and binary-blob []byte) are also deep-copied, since a caller may +// mutate a slice returned from Entity.Get/Map; strings and boxed integers are +// immutable and shared by value. func (s *fieldState) clone() *fieldState { c := &fieldState{state: make([]cell, len(s.state))} copy(c.state, s.state) for i := range c.state { - if sub, ok := c.state[i].sub(); ok { - c.state[i].ref = sub.clone() + switch v := c.state[i].ref.(type) { + case *fieldState: + c.state[i].ref = v.clone() + case []float32: + c.state[i].ref = append([]float32(nil), v...) + case []byte: + c.state[i].ref = append([]byte(nil), v...) } } return c From ff0e2a1ad67c38670436e9233550f7c34f789424 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 10:26:14 -0500 Subject: [PATCH 33/39] fix(parser): clear reused tuple/pending buffers after dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit p.entityTuples and p.pendingMsgBuf were stored back with their full length and stale [len:cap] entries, so *Entity pointers (and their de-boxed state) and inner-packet buffers were retained an extra packet — stale slots from a larger prior packet could pin deleted entities. After dispatch, clear() the used entries and keep the slice at length zero; clearing the full written length each packet keeps [len:cap] zero across packets, so no stale references accumulate. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P4.1: allocs/op flat, B/op flat (clear is alloc-free) sec/op +1.4% nominal, consistent with thermal drift across the run Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 5 +++-- demo_packet.go | 10 ++++++---- entity.go | 6 +++++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index b15bf3a..63515d9 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -509,7 +509,7 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | **End of Phase 3 vs P0** | **−51.8%** | **−51.0%** | **−78.8%** | PASS | | P4 baseline (re-measured at fbca7ed) | 0.727 s | 388.2 MiB | 4.41M | PASS | | P4.1 deep-copy mutable baseline leaves | 0.755 s (~) | 389.4 MiB (+0.3%) | 4.478M (+1.5%) | PASS | -| P4.2 clear reused tuple/pending buffers | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.2 clear reused tuple/pending buffers | 0.765 s (~thermal) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.3 guard skipBits underflow | _tbd_ | _tbd_ | _tbd_ | _tbd_ | | P4.4 fix debug position | _tbd_ | _tbd_ | _tbd_ | _tbd_ | | P4.5 lock value-changing decoders (tests+docs) | _tbd_ | _tbd_ | _tbd_ | _tbd_ | @@ -547,7 +547,8 @@ fixes is explicit. packet buffers an extra packet — stale slots from a larger prior packet can pin *deleted* entities. - **Fix:** `clear()` the used entries and store the slice back at `[:0]`. Clearing the full written length each packet keeps `[len:cap]` zero across packets, so no stale refs accumulate. -- **Result:** _(pending)_ +- **Result:** allocs/op and B/op flat (clear is alloc-free); sec/op +1.4% nominal but consistent with the + thermal drift seen across the whole P4 run (744→754→765 ms), not the change. go test green. ✅ ### P4.3 — guard skipBits underflow + truncated-stream test - **Issue:** `peekBits` zero-pads at EOF and `skipBits` blindly subtracts, so on truncated/corrupt input a diff --git a/demo_packet.go b/demo_packet.go index 72d3733..3504a1e 100644 --- a/demo_packet.go +++ b/demo_packet.go @@ -83,16 +83,18 @@ func (p *Parser) onCDemoPacket(m *dota.CDemoPacket) error { // and avoids the reflection allocations of sort.Sort's interface path. sort.Stable(ms) - // Dispatch messages in order, returning on handler error. Store the - // (possibly grown) backing array back on the parser on every exit path. + // Dispatch messages in order, returning on handler error. for i := range ms { if err := p.Callbacks.callByPacketType(ms[i].t, ms[i].buf); err != nil { - p.pendingMsgBuf = ms return err } } - p.pendingMsgBuf = ms + // Release the inner-packet buffer references and keep the slice at length + // zero so the reused backing array does not retain packet data until the + // next packet overwrites the slot. + clear(ms) + p.pendingMsgBuf = ms[:0] return nil } diff --git a/entity.go b/entity.go index ea3a7a8..1cd9c63 100644 --- a/entity.go +++ b/entity.go @@ -311,7 +311,6 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error tuples = append(tuples, entityOpTuple{e, op}) } - p.entityTuples = tuples for _, h := range p.entityHandlers { for i := range tuples { @@ -321,6 +320,11 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error } } + // Release the entity references and keep the buffer at length zero so a + // reused backing array does not retain (possibly deleted) entities and + // their state until the next packet happens to overwrite the slot. + clear(tuples) + p.entityTuples = tuples[:0] return nil } From 69b3aaa03c00f17b925bd558733ed3f41b44deb5 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 10:29:08 -0500 Subject: [PATCH 34/39] fix(reader): guard skipBits against underflow on truncated streams peekBits zero-pads at end of buffer and skipBits blindly subtracted, so on a truncated or corrupt field-path op stream a lookup entry could consume more bits than were buffered, underflowing bitCount (uint32 wraps huge) and looping forever appending field paths. Make skipBits panic when n > bitCount; the parser's top-level recover turns it into an error, restoring the fail-fast behaviour the per-bit walk had. Cannot affect well-formed replays (the fast path never under-runs on valid streams). Adds TestReadFieldPathsTruncated (an all-zero buffer is an unbounded PlusOne run) asserting a clean panic instead of a hang. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P4.2: all metrics flat (guard is free; sec p=0.631, B/op p=0.971, allocs p=0.057). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 6 ++++-- huffman_test.go | 16 ++++++++++++++++ reader.go | 9 ++++++++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 63515d9..4a1b183 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -510,7 +510,7 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P4 baseline (re-measured at fbca7ed) | 0.727 s | 388.2 MiB | 4.41M | PASS | | P4.1 deep-copy mutable baseline leaves | 0.755 s (~) | 389.4 MiB (+0.3%) | 4.478M (+1.5%) | PASS | | P4.2 clear reused tuple/pending buffers | 0.765 s (~thermal) | 389.4 MiB (flat) | 4.478M (flat) | PASS | -| P4.3 guard skipBits underflow | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.3 guard skipBits underflow | 0.763 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.4 fix debug position | _tbd_ | _tbd_ | _tbd_ | _tbd_ | | P4.5 lock value-changing decoders (tests+docs) | _tbd_ | _tbd_ | _tbd_ | _tbd_ | @@ -558,7 +558,9 @@ fixes is explicit. huffman test used 8-byte buffers and never hit the `<8 bits remaining` case. - **Fix:** `skipBits` panics cleanly when `n > bitCount` (caught by the parser recover → error), restoring fail-fast. Add a huffman test that truncates an op stream and asserts a clean error. -- **Result:** _(pending)_ +- **Result:** all metrics flat (sec p=0.631, B/op p=0.971, allocs p=0.057 — the guard is free). + `TestReadFieldPathsTruncated` confirms a truncated stream now panics cleanly instead of looping + forever (an all-zero buffer = unbounded PlusOne run). go test green. ✅ ### P4.4 — correct debug position() after word refill - **Issue:** `position()` (reader.go) still assumes `bitCount <= 8`, but the word reader can leave 56/48/… diff --git a/huffman_test.go b/huffman_test.go index d8480b8..45cb7aa 100644 --- a/huffman_test.go +++ b/huffman_test.go @@ -87,6 +87,22 @@ func decodeOneHuffOpWalk(r *reader) int32 { } } +// TestReadFieldPathsTruncated verifies that a truncated field-path op stream +// fails cleanly instead of underflowing the bit accumulator. An all-zero buffer +// decodes as an unbounded run of PlusOne ops (code "0") that never reaches +// FieldPathEncodeFinish; once the bits run out, the lookup would consume past +// the end of the buffer. With the skipBits guard this panics (the parser +// recovers it into an error); without it, bitCount underflows and the decode +// loops forever appending field paths. +func TestReadFieldPathsTruncated(t *testing.T) { + defer func() { + if recover() == nil { + t.Fatal("expected a panic on a truncated field-path stream") + } + }() + readFieldPaths(newReader([]byte{0x00, 0x00}), nil) +} + // TestHuffmanLookupMatchesWalk verifies the 8-bit lookup fast path decodes the // same op and consumes the same number of bits as the pure flat-tree walk for // many random streams. An 8-byte buffer comfortably holds the longest code diff --git a/reader.go b/reader.go index d7eeb76..0c848e1 100644 --- a/reader.go +++ b/reader.go @@ -118,8 +118,15 @@ func (r *reader) peekBits(n uint32) uint32 { return uint32(r.bitVal & readBitMasks[n]) } -// skipBits discards n (<= bitCount) bits already buffered in the accumulator. +// skipBits discards n bits already buffered in the accumulator. It panics if +// fewer than n bits are buffered, which happens only on a truncated or corrupt +// stream (a field-path op lookup resolving past the end of the buffer). The +// parser's top-level recover turns that into an error, rather than letting +// bitCount underflow and the field-path decode spin on garbage. func (r *reader) skipBits(n uint32) { + if n > r.bitCount { + _panicf("skipBits: insufficient buffer (need %d, have %d bits)", n, r.bitCount) + } r.bitVal >>= n r.bitCount -= n } From 4e9a173840be61f923fd0a16ca2cff52f8b5708e Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 10:31:36 -0500 Subject: [PATCH 35/39] fix(reader): correct debug position() after word-at-a-time refill position() assumed bitCount <= 8 (one byte buffered), but the word reader can leave 56/48/... bits buffered, making the byte/bit split wrong. Derive the logical bit position from pos*8 - bitCount. Debug-only (called under the v(6) guard); no parsing impact. go test ./... green; benchstat flat vs P4.3. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 4 ++-- reader.go | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 4a1b183..09d8c4c 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -511,7 +511,7 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P4.1 deep-copy mutable baseline leaves | 0.755 s (~) | 389.4 MiB (+0.3%) | 4.478M (+1.5%) | PASS | | P4.2 clear reused tuple/pending buffers | 0.765 s (~thermal) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.3 guard skipBits underflow | 0.763 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | -| P4.4 fix debug position | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.4 fix debug position | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.5 lock value-changing decoders (tests+docs) | _tbd_ | _tbd_ | _tbd_ | _tbd_ | **Phase 2 (correctness) notes:** all 12 goals landed, full suite green with identical golden assertions, @@ -566,7 +566,7 @@ fixes is explicit. - **Issue:** `position()` (reader.go) still assumes `bitCount <= 8`, but the word reader can leave 56/48/… Wrong verbose-debug output only; no parsing impact. - **Fix:** compute the logical bit position as `pos*8 - bitCount`. -- **Result:** _(pending)_ +- **Result:** flat (debug-only; called only under the `v(6)` guard, off in bench/test). go test green. ✅ ### P4.5 — lock value-changing decoders + document Decision A - **Decision A (owned):** the branch deliberately changes what `.Get` returns for a few fields, to match diff --git a/reader.go b/reader.go index 0c848e1..cfce8a4 100644 --- a/reader.go +++ b/reader.go @@ -36,10 +36,14 @@ func (r *reader) remBits() uint32 { } func (r *reader) position() string { - if r.bitCount > 0 { - return fmt.Sprintf("%d.%d", r.pos-1, 8-r.bitCount) + // Logical bit position consumed so far. The word-at-a-time refill can buffer + // many bits (not just the current byte), so derive byte.bit from pos*8 minus + // the still-buffered bitCount rather than assuming bitCount <= 8. + bits := r.pos*8 - r.bitCount + if rem := bits % 8; rem != 0 { + return fmt.Sprintf("%d.%d", bits/8, rem) } - return fmt.Sprintf("%d", r.pos) + return fmt.Sprintf("%d", bits/8) } // remBytes calculates the number of unread bytes in the buffer From 1b3d8abdbc580ef6f4b6153fd32be15a3ee75796 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 10:35:09 -0500 Subject: [PATCH 36/39] test(decoder): lock value-changing decoder representations (Decision A) The branch deliberately changes what Entity.Get returns for a few fields, to match clarity's correct representation, and we are owning these as intentional API changes (Decision A, documented in IMPROVEMENTS.md): - int64 fields: int32 (truncated) -> int64 - HeroID_t: uint32 -> int32 (signed) - HSequence: uint32 -> int32, value - 1 (-1 = none) - BloodType: stayed uint64; only the fixed-8 encoding changed (value identical) Add decoder-level tests asserting the exact dynamic type AND value each produces (deterministic, not replay-dependent), plus a wiring test locking the fieldTypeDecoders map entries, so these intended downstream-visible values can't silently regress. Covers the inline-vs-boxed uint64 split including a >2^32 value (no truncation). go test ./... green; benchstat flat vs P4.4 (test-only). Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 12 +++++-- field_decoder_test.go | 83 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 field_decoder_test.go diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 09d8c4c..8c2751d 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -512,14 +512,16 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P4.2 clear reused tuple/pending buffers | 0.765 s (~thermal) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.3 guard skipBits underflow | 0.763 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.4 fix debug position | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | -| P4.5 lock value-changing decoders (tests+docs) | _tbd_ | _tbd_ | _tbd_ | _tbd_ | +| P4.5 lock value-changing decoders (tests+docs) | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | +| **End of Phase 4 vs P3** | flat sec (incl. drift) | +0.3% B/op | +1.5% allocs (baseline isolation) | PASS | **Phase 2 (correctness) notes:** all 12 goals landed, full suite green with identical golden assertions, and no perf regression (sec/op, B/op, allocs/op all statistically flat vs end of Phase 1). Highlights: P2.1 fixed the real string-table additive-index bug; P2.6–P2.9 added forward-compat decoders (CUtlBinaryBlock, Quaternion, int64-64bit, HSequence/HeroID_t/BloodType aligned to clarity, QAngle precise/noscale); P2.2/P2.3/P2.11/P2.12 hardened error paths. P2.7/P2.8 change live field *values* to -match clarity (not asserted by goldens) — flagged for review. +match clarity; these are owned as intentional API changes (Decision A) and locked by +decoder-representation tests in Phase 4 (P4.5). --- @@ -582,4 +584,8 @@ fixes is explicit. (deterministic, not replay-dependent), so the intended downstream-visible values are locked in CI — this is what the review asked for ("prove values are what you intend, not just no-desync"). Also assert the inline-vs-boxed uint64 split round-trips, including a `> 2^32` value to prove no truncation. -- **Result:** _(pending)_ +- **Result:** added `field_decoder_test.go` with `TestDecoderRepresentations` (exact type+value for + hSequence `int32`−1, HeroID_t signed `int32`, int64 full `int64`, BloodType `uint64` fixed-8, inline + `uint64` incl. `0xFFFFFFFF`, and a `>2^32` steamid + fixed64 with no truncation) and + `TestValueChangingDecoderWiring` (locks the `fieldTypeDecoders` map entries). Test-only; bench flat. + Decision A documented above. go test green. ✅ diff --git a/field_decoder_test.go b/field_decoder_test.go new file mode 100644 index 0000000..860e2b9 --- /dev/null +++ b/field_decoder_test.go @@ -0,0 +1,83 @@ +package manta + +import ( + "encoding/binary" + "testing" + + "github.com/stretchr/testify/assert" +) + +// encVarUint encodes v as an unsigned LEB128 varint, matching reader.readVarUint*. +func encVarUint(v uint64) []byte { + var b []byte + for v >= 0x80 { + b = append(b, byte(v)|0x80) + v >>= 7 + } + return append(b, byte(v)) +} + +// encVarInt encodes v as a zig-zag signed varint, matching reader.readVarInt*. +func encVarInt(v int64) []byte { + return encVarUint(uint64((v << 1) ^ (v >> 63))) +} + +// TestDecoderRepresentations locks the exact dynamic type AND value that the +// value-changing / representation-critical decoders produce through Entity.Get. +// These are the deliberate clarity-aligned changes we are owning (see +// IMPROVEMENTS.md P4.5 / Decision A): a regression here is a downstream-visible +// behavior change, which the replay golden tests would not necessarily catch. +// assert.Equal compares both the concrete type and value of the interface{}. +func TestDecoderRepresentations(t *testing.T) { + assert := assert.New(t) + + dec := func(d fieldDecoder, buf []byte) interface{} { + return d(newReader(buf)).iface() + } + + // HSequence: unsigned varint minus one, returned as a signed int32 so the + // "none" handle (wire 0) is -1 rather than a large unsigned value. + assert.Equal(int32(4), dec(hSequenceDecoder, encVarUint(5))) + assert.Equal(int32(-1), dec(hSequenceDecoder, encVarUint(0))) + + // HeroID_t: signed (zig-zag) varint int32. + assert.Equal(int32(21), dec(signedDecoder, encVarInt(21))) + assert.Equal(int32(-5), dec(signedDecoder, encVarInt(-5))) + + // int64: full 64-bit signed varint (not truncated to int32). + assert.Equal(int64(5_000_000_000), dec(signed64Decoder, encVarInt(5_000_000_000))) + + // BloodType: fixed 8-bit unsigned, returned as uint64. + assert.Equal(uint64(66), dec(bloodTypeDecoder, []byte{0x42})) + + // 32-bit-range unsigned handles: stored inline, returned as uint64. + assert.Equal(uint64(5), dec(unsignedDecoder, encVarUint(5))) + assert.Equal(uint64(0xFFFFFFFF), dec(unsignedDecoder, encVarUint(0xFFFFFFFF))) + + // Genuinely 64-bit unsigned values (e.g. steam IDs) must round-trip without + // truncation through the boxed path, returned as uint64. + assert.Equal(uint64(76561198140280423), dec(unsigned64Decoder, encVarUint(76561198140280423))) + fixedBuf := make([]byte, 8) + binary.LittleEndian.PutUint64(fixedBuf, 0x0123456789ABCDEF) + assert.Equal(uint64(0x0123456789ABCDEF), dec(fixed64Decoder, fixedBuf)) +} + +// TestValueChangingDecoderWiring locks that the field types whose representation +// changed are mapped to the intended decoders, so a future edit to the decoder +// tables cannot silently revert the behavior. +func TestValueChangingDecoderWiring(t *testing.T) { + assert := assert.New(t) + + wired := func(typeName string, buf []byte) interface{} { + d, ok := fieldTypeDecoders[typeName] + if !ok { + t.Fatalf("no decoder registered for %s", typeName) + } + return d(newReader(buf)).iface() + } + + assert.Equal(int32(-1), wired("HSequence", encVarUint(0))) + assert.Equal(int32(21), wired("HeroID_t", encVarInt(21))) + assert.Equal(int64(5_000_000_000), wired("int64", encVarInt(5_000_000_000))) + assert.Equal(uint64(66), wired("BloodType", []byte{0x42})) +} From 9d530d55a047ef81e7a7d56052aa3658ad7d9e2f Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 10:37:38 -0500 Subject: [PATCH 37/39] docs: phase 4 cumulative cost (cooled re-measure) Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 8c2751d..8c61ed0 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -513,7 +513,14 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P4.3 guard skipBits underflow | 0.763 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.4 fix debug position | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.5 lock value-changing decoders (tests+docs) | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | -| **End of Phase 4 vs P3** | flat sec (incl. drift) | +0.3% B/op | +1.5% allocs (baseline isolation) | PASS | +| **End of Phase 4 vs P3** (cooled re-measure) | 0.748 s (~, p=0.218) | 389.4 MiB (+0.3%) | 4.478M (+1.5%) | PASS | +| **End of Phase 4 vs P0** | **−50.9%** | **−50.8%** | **−78.4%** | PASS | + +**Phase 4 cost (cooled, deterministic):** the only real cost of the post-review correctness/robustness +fixes is **+1.54% allocs (+68K)** from P4.1's per-entity baseline mutable-leaf isolation; B/op +0.33%, +sec/op flat (the intermediate sec wobble was thermal drift across the back-to-back bench run, confirmed +by a cooled re-measure: 744→748 ms, p=0.218). The skipBits guard, buffer clearing, debug fix, and +decoder-lock tests are all free. **Phase 2 (correctness) notes:** all 12 goals landed, full suite green with identical golden assertions, and no perf regression (sec/op, B/op, allocs/op all statistically flat vs end of Phase 1). Highlights: From 4fbc1fe3bdc67afb5db498e7a70194e4cdaf489e Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 12:24:15 -0500 Subject: [PATCH 38/39] fix(parser): clear reused buffers on error paths too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P4.2 cleared p.pendingMsgBuf / p.entityTuples only after successful dispatch; on a handler/callByPacketType error the parser field still pointed at the just-refilled backing array, so packet buffers / entity pointers stayed live (bounded to one packet, freed at parser GC since the parse aborts, but a real inconsistency). Dispatch into a result variable and break on error, then run a single clear() + [:0] cleanup before a single return, so cleanup runs on success and error alike. Uses the result-variable pattern rather than a deferred closure on purpose: a defer closure captures the slice header (also stored on the Parser), forcing it to escape to the heap — a per-packet allocation in a path that runs thousands of times. The result-variable form is allocation-free. go test ./... green (identical golden values). benchstat (8552595443, 10x x10) vs P4.5: alloc- and B/op-neutral (4,478,290 allocs / 408.36M B identical), sec flat. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 14 ++++++++++++++ demo_packet.go | 13 +++++++------ entity.go | 13 ++++++++----- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 8c61ed0..19b2556 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -513,6 +513,7 @@ Verified zero occurrences across all 39 build replays, so safe insurance: | P4.3 guard skipBits underflow | 0.763 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.4 fix debug position | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | P4.5 lock value-changing decoders (tests+docs) | 0.761 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | +| P4.6 clear buffers on error paths (follow-up) | 0.732 s (flat) | 389.4 MiB (flat) | 4.478M (flat) | PASS | | **End of Phase 4 vs P3** (cooled re-measure) | 0.748 s (~, p=0.218) | 389.4 MiB (+0.3%) | 4.478M (+1.5%) | PASS | | **End of Phase 4 vs P0** | **−50.9%** | **−50.8%** | **−78.4%** | PASS | @@ -596,3 +597,16 @@ fixes is explicit. `uint64` incl. `0xFFFFFFFF`, and a `>2^32` steamid + fixed64 with no truncation) and `TestValueChangingDecoderWiring` (locks the `fieldTypeDecoders` map entries). Test-only; bench flat. Decision A documented above. go test green. ✅ + +### P4.6 — clear reused buffers on error paths too (follow-up review) +- **Issue:** P4.2 only cleared `p.pendingMsgBuf` / `p.entityTuples` after *successful* dispatch. If + `callByPacketType` or an entity handler returns an error, the parser field still pointed at the + just-refilled backing array, so packet buffers / entity pointers stayed live. Low severity (the parse + aborts on error, so it's freed at parser GC; bounded to one packet), but a real inconsistency. +- **Fix:** dispatch into a result variable (`break` / labeled `break dispatch` on error), then a single + `clear()` + `[:0]` cleanup before a single return — so cleanup runs on success *and* error. Used this + result-variable pattern rather than a `defer func(){…}()` closure on purpose: the closure captures the + slice header (also stored on the Parser), forcing it to escape to the heap — a per-packet allocation in a + path that runs thousands of times. The result-variable form is alloc-free. (Panic mid-dispatch still + skips cleanup, but that hits the parser's top-level recover and tears the parser down, same as today.) +- **Result:** alloc- and B/op-neutral (4,478,290 allocs / 408.36M B identical to P4.5); sec flat. go test green. ✅ diff --git a/demo_packet.go b/demo_packet.go index 3504a1e..0d809a3 100644 --- a/demo_packet.go +++ b/demo_packet.go @@ -83,19 +83,20 @@ func (p *Parser) onCDemoPacket(m *dota.CDemoPacket) error { // and avoids the reflection allocations of sort.Sort's interface path. sort.Stable(ms) - // Dispatch messages in order, returning on handler error. + // Dispatch messages in order, stopping on handler error. + var err error for i := range ms { - if err := p.Callbacks.callByPacketType(ms[i].t, ms[i].buf); err != nil { - return err + if err = p.Callbacks.callByPacketType(ms[i].t, ms[i].buf); err != nil { + break } } // Release the inner-packet buffer references and keep the slice at length - // zero so the reused backing array does not retain packet data until the - // next packet overwrites the slot. + // zero so the reused backing array does not retain packet data. This runs on + // every path (success or error), before the single return. clear(ms) p.pendingMsgBuf = ms[:0] - return nil + return err } // Internal parser for callback OnCDemoFullPacket. diff --git a/entity.go b/entity.go index 1cd9c63..f8fdb30 100644 --- a/entity.go +++ b/entity.go @@ -312,20 +312,23 @@ func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error tuples = append(tuples, entityOpTuple{e, op}) } + var err error +dispatch: for _, h := range p.entityHandlers { for i := range tuples { - if err := h(tuples[i].e, tuples[i].op); err != nil { - return err + if err = h(tuples[i].e, tuples[i].op); err != nil { + break dispatch } } } // Release the entity references and keep the buffer at length zero so a - // reused backing array does not retain (possibly deleted) entities and - // their state until the next packet happens to overwrite the slot. + // reused backing array does not retain (possibly deleted) entities and their + // state. This runs on every path (success or error), before the single + // return. clear(tuples) p.entityTuples = tuples[:0] - return nil + return err } // OnEntity registers an EntityHandler that will be called when an entity From 7f3f40395112707f9dfe32c390733c8f6d3aed7d Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Mon, 22 Jun 2026 13:23:03 -0500 Subject: [PATCH 39/39] chore: remove IMPROVEMENTS.md planning doc The working plan/benchmark log lived in the branch during development; the per-commit benchstat tables and the PR summary capture the durable record, so drop the scratch doc before merge. Co-Authored-By: Claude Opus 4.8 (1M context) --- IMPROVEMENTS.md | 612 ------------------------------------------------ 1 file changed, 612 deletions(-) delete mode 100644 IMPROVEMENTS.md diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md deleted file mode 100644 index 19b2556..0000000 --- a/IMPROVEMENTS.md +++ /dev/null @@ -1,612 +0,0 @@ -# Manta parser — correctness & performance improvement plan - -Borrowed from the Clarity (Java) reference parser at `../clarity`. Executed on branch -`jcoene/goal-perf`, one goal per commit, in `/goal` mode. - -This plan was produced by a 14-agent review (7 subsystem analyses, each adversarially -verified against both codebases with real CPU/alloc profiles of `replays/2159568145.dem`). -27 findings confirmed, 19 adjusted, 1 rejected. - -## Ground rules - -- **Strictly no public API change.** Internals only — no new exported symbols, no changed - signatures. The de-box work stays behind the existing `Entity.Get`/`GetFloat32` (box lazily - on read). The internal `fieldDecoder` type is unexported and is fair game. -- **One optimization per commit.** Each commit message carries its `benchstat` table vs the - previous commit + `go test ./...` PASS. The branch history is the story. -- **Commits only — do NOT open a PR, do NOT push.** Make local commits on `jcoene/goal-perf` and - report the cumulative results here for review. The user opens the PR. -- **Sequence:** safe high-ROI wins first, re-baseline, then the invasive typed-state rewrite. - -## Verification protocol (per goal) - -- **Correctness gate (non-negotiable):** `go test ./...` stays green with **identical** golden - values (manta_test.go runs ~48 real replays asserting exact `expectEntityEvents` counts, - `expectHeroEntityMana` floats, combat-log counts). A perf change that alters output is a regression. -- **Perf measure:** `go test -run=XXX -bench= -benchmem -benchtime=10x -count=10` before - and after; compare with `benchstat` (`go install golang.org/x/perf/cmd/benchstat@latest`). - Report `sec/op`, `B/op`, **`allocs/op`** (the near-deterministic leading indicator). -- **Profile confirmation:** `make memprofile` / `make cpuprofile` — confirm the *targeted* site - actually moved, not just the aggregate. -- **Canonical bench replay (held fixed for the whole branch):** **`8552595443`** (build 6601 — most - representative of current real-world replays). The review's profile evidence below was gathered on - `2159568145`; re-capture the alloc profile on `8552595443` at P0 to confirm the top sources before - optimizing (relative signal is expected to hold). - -### Profile evidence (gathered on `replays/2159568145.dem`, M4 Pro; canonical bench going forward is `8552595443`) -- ~11.0M allocs/op, ~315 MB/op. -- Streaming `*os.File`: 798 ms/op · `bufio`: 658 ms/op · in-memory `bytes.Reader`: 644 ms/op. -- Top alloc sources: `readFieldPaths` slice 56–60% · quantized-float box ~10–12% · QAngle - `[]float32` ~4.5–5.5% · signed/unsigned int box ~2–3% · `*pendingMessage` ~5% · `*outerMessage` ~5%. - ---- - -## Goal summary - -| ID | Goal | Type | Risk | Effort | Expected impact | -|----|------|------|------|--------|-----------------| -| **P0** | In-memory benchmark rig + recent-replay bench | infra | none | small | makes parse wins measurable (removes ~80% syscall noise) | -| **P1.1** | `pendingMessage` value-slice + typed `sort.Stable` (reuse buffer) | perf | low | small | −0.54M allocs, −20 MB; +determinism | -| **P1.2** | `outerMessage` by value | perf | low | small | −0.56M allocs, −17 MB | -| **P1.3** | snappy scratch-buffer reuse | perf | med | small | −9 MB/op | -| **P1.4** | modifier emit: early-return when no handler | perf | low | small | removes modifier unmarshal from hot path | -| **P1.5** | reuse per-packet `tuples` + entity-data reader | perf | low | small | −0.44M allocs | -| **P1.6** | hoist per-entity `fpCache`/`fpNoop` maps to class | perf | low | small | −2 maps/entity; smaller Entity | -| **P1.7** | hoist `v(6)` debug guard out of per-field loop | perf | low | small | −33M branch/calls (CPU) | -| **P1.8** | buffer `stream` IO (bufio when not byte-reader) | perf | low | small | 798→658 ms streaming path | -| **P2.1** | **string-table additive index fix** (real bug) | correctness | low | small | fixes 4,469 wrong indices | -| **P2.2** | string-table: fail loud, don't swallow panics | correctness | low | small | surfaces silent corruption | -| **P2.3** | `getEventKey` off-by-one (`>` → `>=`) | correctness | low | small | prevents panic on skewed events | -| **P2.4** | modifier: skip empty/deleted entries | correctness | low | small | no spurious zero events | -| **P2.5** | combat-log `Type()/TypeName()/String()` descriptor-driven | correctness | low | small | latent landmine fix | -| **P2.6** | decoder forward-compat additions (never-occur types) | correctness | low | small | CUtlBinaryBlock/Quaternion/int64/etc | -| **P2.7** | HSequence / HeroID_t decode parity (value-changing) | correctness | med | small | matches clarity; suite-gated | -| **P2.8** | BloodType fixed-8 decode (risky, suite-gated) | correctness | med | small | only if all goldens stay green | -| **P2.9** | QAngle precise/32/0-bit forward-compat | correctness | low | small | future builds | -| **P2.10** | mana/runetime patch sentinel guards | correctness | low | small | robustness/parity | -| **P2.11** | outer-message size sanity bound | correctness | low | small | OOM hardening on corrupt input | -| **P2.12** | field-path depth-7 guard + comment | correctness | low | small | loud failure vs cryptic panic | -| **P1.9** | **reusable field-path buffer** (value-type, no pool/copy/slice) | perf | med | medium | **−56–60% of all allocs** | -| **P1.10** | word-at-a-time bit reader | perf | med | small | top CPU win (after P0) | -| **P1.11** | varint + `readByte`/`readLeUintX` from accumulator | perf | low | small | compounds P1.10 | -| **P1.12** | flatten huffman tree to int arrays (no dispatch) | perf | low | medium | removes per-bit interface calls | -| **P1.13** | 8-bit field-op lookup table | perf | med | medium | resolves majority of ops in 1 index | -| **P1.14** | decode class baseline once, clone per entity | perf | med | medium | baseline re-decode is ~4× update decode | -| **P3.1** | **typed entity state (eliminate boxing)** | perf | med | large | ~20% of allocs (float+qangle+int) | -| — | string-table Items: map → dense slice | perf | med | medium | locality; depends on P2.1 | -| — | integrate `CDemoStringTables` full dumps | correctness | med | medium | seek/robustness; depends on P2.1 | -| **PARK** | combat-log name-resolution helper / `CombatLogEntry` | correctness | — | — | **blocked: new exported API** | -| **PARK** | S2 HLTV `CMsgDOTACombatLogEntry` path | correctness | — | — | **blocked: new exported API** | -| **PARK** | VTProtobuf zero-reflection unmarshal | perf | med | large | envelope decode only; large effort | - -Ordering note: P1.9–P1.14 are the bigger structural wins; they're listed after the trivial -P1.1–P1.8 alloc trims so we build a clean measured baseline first, exactly as agreed. - ---- - -## P0 — In-memory benchmark rig (foundational, no parser logic) - -**Why:** the current `BenchmarkMatch` (manta_test.go:609) streams from `*os.File` inside the timed -loop with no `b.ResetTimer()`; the review measured ~80% of CPU in per-byte-read syscalls (798 ms vs -644 ms in-memory). Parse optimizations are invisible under that noise, and `B/op` is polluted by IO. - -**Change:** add `BenchmarkMatch8552595443` (build 6601) as the canonical bench, and rewrite `bench()` to -read the replay into `[]byte` once before the loop, `b.ResetTimer()`, and parse via in-memory -`NewParser(buf)` in the loop. Point the Makefile `memprofile`/`cpuprofile` targets at `8552595443` too. -No parser code touched → cannot affect correctness. - -**Verify:** `go test ./...` unchanged; capture the P0 baseline (`sec/op`, `B/op`, `allocs/op`) and a fresh -`make memprofile` on `8552595443` — this is the reference all later commits compare against, and it -reconfirms the top alloc sources (expect `readFieldPaths` to dominate). - -**Result:** baseline **1.523 s/op ±1%, 791.5 MiB/op, 20.75M allocs/op** (8552595443, M4 Pro, 10x×10). -Alloc profile confirms `readFieldPaths` #1 at **56.6%**, then quantized-float box 5.4%, `onCDemoPacket` -pointer/tuple allocs 4.6%, noscale/signed boxes ~3% each, QAngle `[]float32` 2.2%. - ---- - -## Phase 1 — safe perf wins - -### P1.1 — `pendingMessage` value-slice + typed `sort.Stable` -- **Now:** `onCDemoPacket` builds `[]*pendingMessage`, allocating a pointer per embedded message - (~5.4M/run), then `sort.Sort` (non-stable) every packet (demo_packet.go:62-89). -- **Change:** make `pendingMessages` a `[]pendingMessage` value slice reusing a parser-level - `pendingMsgBuf[:0]`; change `priority()` to a value receiver; use **typed `sort.Stable`** (not - `sort.SliceStable`, which adds ~1.46M reflection allocs). Store the buffer back on both the error - and success return paths. -- **Clarity:** dispatches embedded messages in file order, no buffering (InputSourceProcessor.java:198-240). -- **Caveat:** manta deliberately reorders across priority buckets — `sort.Stable` gives - *file-order-within-bucket* determinism; do **not** describe it as full clarity parity, and do **not** - remove the priority sort (would change dispatch order and break golden counts). -- **Impact (measured):** 11.0M→10.47M allocs, 315→295 MB. **Reentrancy verified safe:** CDemoPacket is - only dispatched via `callByDemoType`, never nested in `callByPacketType`. -- _This is the change a review agent prototyped + reverted; reimplement cleanly here with a benchmark._ -- **Result:** sec/op 1.523→1.514 (−0.62%, p=0.000), B/op 791.5→763.8 MiB (−3.51%), allocs/op - 20.75M→20.06M (−3.33%, −0.69M). go test green. ✅ - -### P1.2 — `outerMessage` by value -- **Now:** `readOuterMessage` heap-allocates `&outerMessage{}` per message (parser.go:238); single - consumer is `Start()` (parser.go:142), value never retained past the iteration. -- **Change:** return by value (or reuse a `p.outerMsg` field); update the `var msg *outerMessage` decl. -- **Impact (measured):** −557K allocs, −17 MB. Zero risk. -- **Result:** allocs/op 20.06M→20.03M (−0.19%, −30K), B/op −0.15%; sec/op +0.32% (run-to-run thermal - noise, +5 ms abs). Smaller than the −557K review estimate — the 70 MB replay has far fewer outer - messages than 2159568145. go test green. ✅ - -### P1.3 — snappy scratch-buffer reuse -- **Now:** `snappy.Decode(nil, buf)` per compressed outer message (parser.go:232) allocates fresh. -- **Change:** pass `p.snappyScratch[:cap(...)]`; snappy@v0.0.3 reuses dst when `dLen <= len(dst)`. -- **Risk (keep medium):** the decompressed buffer becomes `msg.data` passed to handlers. If any - CDemo* handler retains a subslice across messages, reuse corrupts it. **The full `go test` suite is - the gate** — do not rely on assertion that string tables copy; verify empirically. -- **Impact (measured):** −9 MB/op. -- **Result:** B/op 762.6→700.6 MiB (**−8.14%** — snappy full-packet buffers), sec/op 1.519→1.511 - (−0.49%), allocs/op −0.12% (−30K). Full 48-replay suite green → reuse aliasing is safe on the corpus. ✅ - -### P1.4 — modifier emit early-return when no handler -- **Now:** `emitModifierTableEvents` (modifier.go:18) allocates + unmarshals a proto per ActiveModifiers - item per snapshot/delta even when `p.modifierTableEntryHandlers` is empty (the bench registers none). -- **Change:** `if len(p.modifierTableEntryHandlers) == 0 { return nil }` at the top of - `emitModifierTableEvents` (covers both call sites string_table.go:126,171). Also switch - `proto.NewBuffer(v).Unmarshal` → `proto.Unmarshal` so a reused msg is safe (Buffer.Unmarshal merges - without reset; only safe on fresh msg). -- **Impact:** removes all modifier unmarshal from the bench hot path. Verified ActiveModifiers-only - (never touches instancebaseline). -- **Result:** allocs/op 20.00M→18.74M (**−6.32%, −1.26M** — bigger than the profile top-12 suggested; - the cost was spread across proto internals), B/op −7.51%, sec/op −2.15%. Did the early-return only; - left per-item fresh-msg unmarshal on the handler path (no consumer retain risk). go test green. ✅ - -### P1.5 — reuse per-packet `tuples` + entity-data reader -- **Now:** `onCSVCMsg_PacketEntities` allocates `newReader(m.GetEntityData())` and - `tuples := make([]tuple, 0, updates)` per packet (entity.go:223,244). -- **Change:** parser-level `tuples[:0]` reset each call (preserves append order → preserves the - handlers-outer/tuples-inner emission order `expectEntityEvents` depends on); `reader.reset(buf)` - method for the entity-data reader. -- **Impact:** −0.44M allocs. (The baseline reader at entity.go:269 is handled by P1.14, not here.) -- **Result:** B/op 647.9→571.0 MiB (**−11.87%** — the per-packet tuples backing array was large: - updates×16B over many packets), allocs/op −0.40% (−80K), sec/op +1.03% (run variance; cumulative sec - still −1.9% vs P0). Hoisted tuple to package-level `entityOpTuple`; reused parser-level reader+tuples. go test green. ✅ - -### P1.6 — hoist per-entity `fpCache`/`fpNoop` maps to class -- **Now:** `newEntity` allocates two maps per entity (entity.go:69-70); name→fieldPath depends only on - `class.serializer`, so every entity of a class recomputes/re-stores the identical mapping. -- **Change:** move both to a single shared map on `*class` (single-goroutine parse → plain map, no - sync.Map). The class-level `*fieldPath` is read-only/never released (matches today's retain-on-hit). -- **Clarity:** resolves name→FieldPath on the DTClass, Entity holds only state (Entity.java:88-114). -- **Impact:** −2 maps/entity + smaller Entity struct. Golden-safe (resolved fieldPath is class-invariant; - `expectPlayer6Name`/`expectHeroEntityName` guard it). -- **Result:** allocs/op −0.16% (−30K, two maps/entity removed), B/op −0.27%, sec/op ~ (p=0.190, noise). - Caches now shared on `*class`; single-goroutine parse so safe. go test green. ✅ - -### P1.7 — hoist `v(6)` debug guard out of the per-field loop -- **Now:** `readFields` calls `v(6)` twice per field path in the hot loop (field_reader.go:13,23). -- **Change:** evaluate `dbg := v(6)` once at the top of `readFields` (per call — **not** higher; debug - tick mutates `debugLevel` mid-parse and `readFields` is called fresh per entity-update, so per-call - re-eval preserves behavior). -- **Impact:** −~33M calls/branches (CPU only, ~0 alloc when disabled). -- **Result:** sec/op 1.509→1.476 (**−2.18%**, p=0.002 — `v(6)` was 2 calls/field across millions of - fields), allocs/B unchanged (CPU-only). go test green. ✅ - -### P1.8 — buffer `stream` IO -- **Now:** `stream.readByte`→`readBytes(1)`→`io.ReadFull` per byte; 3 varints + payload per outer - message (stream.go, parser.go:198-227). -- **Change:** in `newStream`, wrap with `bufio.NewReaderSize` **only** when the reader is not already an - `io.ByteReader` (`*os.File` isn't; `*bytes.Reader` is → `NewParser` stays unwrapped, no double-buffer). -- **Impact:** 798→658 ms on the streaming path. **Note:** the canonical in-memory bench (P0) won't show - this — it's a real-world streaming-path win; verify with a streaming-reader bench variant. -- **Result:** canonical in-memory bench flat (+0.46%, noise — `bytes.Reader` left unwrapped, no - regression). Streaming `NewStreamParser(os.File)` path **1.612→1.507 s/op (−6.47%, p=0.002)**, measured - with a throwaway streaming bench (reverted before commit). go test green. ✅ - -### P1.9 — reusable field-path buffer ⭐ biggest single win -- **Now:** `readFieldPaths` (field_path.go:309-337) starts `paths := []*fieldPath{}` (cap 0) and - `append(paths, fp.copy())` per op; this slice growth is **56–60% of all allocations** (17M objects/op, - 309 MB), independently confirmed by two agents. `fp.copy()` itself allocs ~0 (sync.Pool). -- **Change (do the three coupled findings together):** (a) stop materializing a fresh `[]*fieldPath` per - call — reuse a buffer reset to len 0 each call; (b) make `fieldPath` carry a fixed `[7]int` value array - so a "copy" is a cheap value store, not a pool Get/Put; (c) since `readFields` immediately iterates + - releases, consider fusing decode inline (ops apply in stream order; decoder lookup depends only on the - current fp value, so interleaving = resolve-all-then-decode — verified equivalent). -- **Plumbing:** `readFieldPaths(r)` / `readFields(r,s,state)` are free functions with no Parser handle — - thread a buffer param or reuse the existing `fpPool` sync.Pool (already concurrency-safe; manta runs - independent Parsers, so a package-global mutable buffer is **unsafe**). Keep `*fieldPath` for the cold - `getFieldPaths()`/`Entity.Map()` paths. -- **Clarity:** one reused `S2ModifiableFieldPath` writing immutable snapshots into a fixed - `fieldPaths[MAX_PROPERTIES=0x3FFF]` (FieldReader.java:11-14, S2FieldReader.java:50-65). -- **Impact:** allocs/op ~11M → ~4–5M. **Keep the fixed depth 7** (`[7]int`). Golden-safe (only the - container changes; field-path values + decode order unchanged). -- **Result:** allocs/op 18.63M→10.65M (**−42.86%, −7.98M**), B/op 569.5→398.6 MiB (−30.0%), sec/op - 1.483→1.198 (**−19.22%**). The 56%-of-allocs `readFieldPaths` container is gone. Gotcha: first attempt - regressed B/op +31% because `&fp` escaped via the indirect closure call — borrowing the accumulator - from the pool fixed it. go test green. ✅ **(headline win)** - -### P1.10 — word-at-a-time bit reader -- **Now:** `readBits` refills the accumulator one byte at a time (reader.go:50-61) with a per-byte - bounds-check+panic in `nextByte`. `readBits` is the #1 CPU consumer. -- **Change:** refill a full `uint64` at once: - `r.bitVal |= binary.LittleEndian.Uint64(r.buf[r.pos:]) << r.bitCount; free := (64 - r.bitCount) >> 3; - r.pos += free; r.bitCount += free*8`, keeping a byte-loop for the final ≤8 bytes (protobuf-owned - buffers have unknown trailing capacity → tail guard `r.pos+8 <= r.size`). -- **Correction to the naive formula:** advance is `(64 - bitCount) >> 3` whole bytes, **not** - `8 - bitCount/8` (bitCount transiently reaches ~33–39 after a refill). -- **Safety invariant (empirically verified):** max `n` observed across real replays is exactly 32 - (`readBits(qfd.Bitcount)`, `noscaleDecoder` readBits(32), `readUBitVarFP` readBits(31)), so one uint64 - load always yields ≥32 valid bits. A 5,000-trial fuzz of byte-vs-word produced bit-identical output. -- **Add a build-time assert** that quantized `Bitcount <= 32` (quantizedfloat.go integer-encode loop can - raise it) so a pathological field can't overflow the accumulator. -- **Impact:** large CPU win (only visible after P0). 0 allocs delta. -- **Result:** sec/op 1.198→1.058 (**−11.64%**), allocs/B flat. Word refill masks before shifting (no - stale partial-byte bits). Added `realign()` so byte-aligned `readBytes`/`readByte` rewind the - buffered word and stay zero-copy — this extended the fast path beyond the old `bitCount==0`, making byte - reads zero-copy *more* often than before (hence no B/op regression). Added quantized `Bitcount<=32` - assert. go test green. ✅ (also satisfies the P-guard zero-copy invariant; P-guard adds the test.) - -### P1.11 — varints + `readByte`/`readLeUintX` from the accumulator -- **Now:** `readVarUint32/64` call `readByte` per byte (byte-aligned fast path → `nextByte`), each a - separate bounds-check (reader.go:102-140). -- **Change:** after P1.10, route `readByte` through `readBits(8)` so varint bytes share the loaded word; - fold the now-dead `bitCount==0` fast path; read `readLeUint32/64`/`readFloat` straight from the - accumulator instead of `binary.LittleEndian.UintXX(readBytes(N))` (the unaligned `readBytes` path - allocates). -- **Impact:** compounds P1.10. `BenchmarkReadVarUint32/64` (reader_test.go) measures it directly. -- **Guard:** preserve `readBytes` zero-copy aliasing (P-guard below). -- **Result:** **SKIPPED.** Routing `readByte`/`readLeUintX` through `readBits` makes them consume into - the read-ahead accumulator, leaving `r.pos` ahead of the logical position. The reader unit tests - (`TestReaderReplayBeginning`, `TestReaderVarints`) assert exact `r.pos` and manually set `r.pos = 0`, and - `r.pos`/`remBytes` are part of the reader contract. The marginal gain (avoiding a `make` on rare - *unaligned* `readLeUintX`) wasn't worth rewriting tests and risking the `pos` contract. P1.10's `realign` - already keeps these byte reads fast and zero-copy with `pos` accurate. Reverted. - -### P1.12 — flatten huffman tree to int arrays -- **Now:** `readFieldPaths` walks an interface pointer-tree per bit (`node.Right()/Left()/IsLeaf()/Value()` - — ~2–3 interface dispatches/bit, huffman.go:9-77, field_path.go:316-332). -- **Change:** flatten to parallel int arrays `tree[node][bit]` (negative = `-ordinal-1` leaf), walked with - no dispatch. **Build from manta's OWN `huffTree`** (manta's tie-break differs structurally from - clarity's; codes were verified bit-for-bit identical when built from manta's table — porting clarity's - numbering would desync). -- **Impact:** removes per-bit interface calls. 0 allocs. **Add a permanent decoder-equivalence test** - (interface-walk vs flat path → identical ordinal + bits-consumed across all 256 prefixes). -- **Result:** sec/op 1058.5→960.2 ms (**−9.28%**), allocs/B unchanged. Flat `int32` child arrays - (negative = leaf op) replace the per-bit interface walk; built from manta's own tree. Added - `TestHuffmanFlatMatchesTree` (flat ≡ interface tree). go test green. ✅ (under 1 s now.) - -### P1.13 — 8-bit field-op lookup table -- **Now:** even flattened, each op is a 3–8 iteration bit-walk. -- **Change:** precompute a 256-entry table (`bits0-7` = consumed count, `bits8-15` = op ordinal or - fall-back node) resolving most ops in one index (clarity FieldOpHuffmanTree.java:19-46, - BitStream64.java:53-84). Depends on P1.12. -- **Safety-critical correction:** manta's reader is a **consuming** stream — the peek must read only - available bits and **zero-pad** the rest, never over-read (`FieldPathEncodeFinish` = '10', 2 bits, fires - near buffer end and would panic `nextByte` past `r.size`). Add a `reader.peekBits(n)` primitive and - unit-test it at the <8-bits-remaining boundary. Also assert `huffTree` is never mutated - (`swapNodes`/`addNode`) after the table is built. -- **Note:** the "99.7%" is clarity's runtime figure; manta's static weights resolve ~98.4% within ≤8 bits - (28/40 ops have >8-bit codes, max 17) — still a large majority. Don't cite 99.7% as verified. -- **Result:** sec/op 960.2→796.0 ms (**−17.10%**), allocs/B unchanged. 256-entry lookup resolves most - ops in one index; `peekBits` zero-pads (never over-reads), flat-walk fallback for >8-bit codes. - `TestHuffmanLookupMatchesWalk` (5000 random streams) confirms lookup ≡ walk (op + bits consumed). go test green. ✅ - -### P1.14 — decode class baseline once, clone per entity -- **Now:** every entity CREATE re-parses raw baseline bytes from scratch - (`readFields(newReader(baseline), …)`, entity.go:269); profiled at ~2.05% — **~4× the cost of the - actual update decode** and more than `newEntity` itself. -- **Change:** decode each class's baseline once into a template state; clone into new entities. **Without - COW** (manta has none today) the clone is a recursive deep-copy of the `*fieldState` tree (must - faithfully clone nested `*fieldState` — 10 type-assert sites rely on leaf-vs-`*fieldState`; mutating one - entity must never corrupt the template/siblings; reproduce the `set` rule that won't overwrite a slot - holding a `*fieldState`). -- **Clarity:** caches decoded baseline per class, `getBaseline().copy()` with copy-on-write - (Entities.java:655-676, NestedArrayEntityState.java:28-43,219-224). -- **Impact:** net win = clone cheaper than re-decode (it is, especially after P3.1 enables cheap COW). - Golden-critical: cloned+overlaid state must be value-identical to today's fresh decode. -- **Result:** sec/op 796.0→766.5 ms (−3.71%), B/op 398.6→378.1 MiB (−5.14%), allocs/op 10.65M→10.26M - (−3.61%, −0.39M). Baseline decoded once per class into a template, `clone()`d per entity (shares - immutable leaf values, deep-copies nested states). Templates invalidated (`clear`) on instancebaseline - update for correctness. go test green. ✅ - -### P-guard — zero-copy `readBytes` invariant (correctness guard, do alongside P1.10/P1.11) -- `readBytes` aligned path returns `r.buf[r.pos-n:r.pos]` aliasing the protobuf buffer (reader.go:81); - `demo_packet.go` stores these into `pendingMessage` and parses them **later**, and sendtable/string - reads depend on the aliasing. A clarity-style padded-copy reader would corrupt this. Add an explicit - invariant + test that byte-aligned `readBytes` stays zero-copy after the word-reader rewrite. -- **Result:** Added `TestReaderReadBytesZeroCopy` — verifies byte-aligned `readBytes` aliases the buffer - (zero-copy) both at `bitCount==0` and through the word reader's `realign` path (non-zero multiple of 8). - Test-only; bench unchanged. The realign mechanism itself landed in P1.10. go test green. ✅ - ---- - -## Phase 2 — correctness (independent; can interleave with Phase 1) - -### P2.1 — string-table additive index fix ⭐ real latent bug -- **Now:** non-increment branch computes **absolute** `index = readVarUint32() + 1` (string_table.go:226). -- **Change:** **additive** `index += readVarUint32() + 2` (clarity S2StringTableEmitter.java:91; matches - manta's own entity decoder entity.go:247). -- **Evidence (experiment):** on the bench replay the additive scheme is strictly monotonic within every - blob; the current absolute scheme produces 2,309 impossible non-increasing steps and 4,469 divergent - indices (all in ActiveModifiers; e.g. manta=11 where correct=50). Invisible to goldens only because no - test resolves a delta-updated table by index. Applying the one-line fix: `TestParseStringTable*` and 5 - golden `TestMatch*` all pass with identical values. Init stays `-1`; do **not** switch to `readUBitVar`. -- **Result:** Applied the one-line additive fix. Full `go test ./...` green — all golden values identical - (no test resolves a delta-updated table by index). Fixes the latent ActiveModifiers mis-indexing. ✅ - -### P2.2 — string-table: fail loud -- `parseStringTable` defers `recover()` and returns partial `items` silently (string_table.go:181-186); - callers see success and apply a half-populated table. Propagate the error (callbacks already return - `error`). `recover` never fires on the bench replay → no healthy-replay regression. Add a truncated-blob - unit test. (Also reconcile the inconsistency: `UpdateStringTable` `_panicf`s on a missing table id at - string_table.go:139 — harsher than clarity, which skips unknown ids.) -- **Result:** `parseStringTable` now returns an error (recover → error); both callers propagate it. Added - `TestParseStringTableTruncated`. go test green (recover never fires on healthy replays). Left the - `UpdateStringTable` `_panicf`-on-missing-table as-is (separate behavior call). ✅ - -### P2.3 — `getEventKey` off-by-one -- `if f.i > len(e.m.GetKeys())` lets `f.i == len` pass → `GetKeys()[f.i]` panics (game_event.go:166). - Change to `>=`. One char; zero golden risk (well-formed descriptors never hit it). -- **Result:** Changed `>` to `>=` (game_event.go). go test green; zero golden change. ✅ - -### P2.4 — modifier: skip empty/deleted entries -- `emitModifierTableEvents` unmarshals every item including empty (`Value == []byte{}`), raising all-zero - messages to handlers. Add `if len(item.Value) == 0 { continue }` (modifier.go:19), matching clarity - `if (value != null)`. A real new modifier is never zero-length on the wire. -- **Result:** Added the empty-value skip. go test green; golden-neutral (the suite registers no modifier - handler). ✅ - -### P2.5 — combat-log `Type()/TypeName()/String()` descriptor-driven -- These hardcode `keys[0].GetValByte()` (game_event.go:37,41,46), bypassing the descriptor field map. Resolve - `type` via the descriptor (clarity S1CombatLogIndices.java:8) and route through `GetInt32`-style dispatch - (not raw `GetValByte`). On current descriptors `type` is index 0 → output unchanged; these methods aren't - called by any manta source/test → zero golden risk. **Fix all three** (the review noted `String()` too). -- **Result:** All three now resolve `type` via `GetInt32("type")` (descriptor + typed dispatch). go test - green — and `GetInt32("type")` is golden-guarded (the combat-log test asserts it returns no error on - every event), so this is value-identical on current descriptors. ✅ - -### P2.6 — decoder forward-compat additions (never-occur today → golden-neutral) -Verified zero occurrences across all 39 build replays, so safe insurance: -- `CUtlBinaryBlock`: no decoder → falls to varint (would desync). Add `n:=readVarUint32(); readBytes(n)` - (clarity CUtlBinaryBlockDecoder). -- `Quaternion`: add to **`fieldTypeFactories`** as `vectorFactory(4)` (128 bits; not `fieldTypeDecoders`). -- `int64`: maps to 32-bit `readVarInt32` (truncates); add `signed64Decoder → readVarInt64` (exists). Note: - changes stored dynamic type int32→int64 — a `.(int32)` consumer would break (forward-compat, not asserted). -- `ResourceId_t → unsigned64Decoder`, `CGlobalSymbol → stringDecoder`, `HSequence → readVarUint32()-1` - (see P2.7 for the value-changing nuance). **Drop** `CBaseVRHandAttachmentHandle` (already correct — no-op). -- Place each in the correct map; note `findDecoderByBaseType` (variable-array childDecoder) consults only - `fieldTypeDecoders`, not factories. -- **Result:** Added `CUtlBinaryBlock` (varint+bytes), `Quaternion` (vectorFactory4), `ResourceId_t` - (unsigned64), `CGlobalSymbol` (string), and `int64`→`signed64Decoder` (full 64-bit varint). The first - four never occur on the corpus (zero behavior change). `int64` affects only `m_nTotalDamageTaken` - (~175k decodes, ≤1 byte today so the value is unchanged, but `Get` now returns `int64` not `int32`). - Dropped CBaseVRHandAttachmentHandle (already correct). go test green (identical golden values). ✅ - -### P2.7 — HSequence / HeroID_t decode parity (value-changing, suite-gated) -- `HSequence` is REAL (644k decodes): manta stores `value`, clarity `value-1` → off-by-one on every - HSequence field today. `HeroID_t` REAL (1536× on builds 6600/6601): clarity uses signed varint (same bits, - different value for negative ids). Both change consumer-visible output but no golden asserts them. Decide - storage signedness (HSequence `value-1` underflows if stored unsigned at value 0). **Run the full suite - incl. 6600/6601 replays; accept only if all green.** -- **Result:** Full suite green (incl. 6600/6601 replays). HSequence → `int32(varuint)-1`, HeroID_t → - signed varint. Same bits consumed (no desync); neither is asserted, so goldens are identical. Changes - live consumer-visible values to match clarity. ✅ - -### P2.8 — BloodType fixed-8 decode (risky) -- `m_nBloodType` (109k× on builds 6600/6601, **in the golden suite**) currently a varint; clarity reads a - fixed 8-bit. Identical bits only when value < 128; if any ≥127, widths differ → desync → broken goldens. - **Suite-gated:** apply only if all goldens (esp. 6600/6601) stay green; otherwise drop. -- **Result:** **KEPT** — full suite green, so every `m_nBloodType` in the corpus is < 128 (fixed-8 ≡ - varint, no desync). Now matches clarity's encoding and is correct for future values ≥ 128. ✅ - -### P2.9 — QAngle precise/32/0-bit forward-compat -- `qangle_pitch_yaw` doesn't handle bitCount ∈ {0,32} (raw float); no `qangle_precise` (20-bit) handling → - would fall to the coord path and desync. Zero occurrences on the corpus (observed bitcounts {0,8,13}) → - pure forward-compat. Add the clarity special-cases (QAnglePitchYawOnly/Precise/NoScale decoders). -- **Result:** Rewrote `qangleFactory` from clarity's actual decoders: `qangle_pitch_yaw` bc∈{0,32}→raw - floats; `qangle_precise`→3 flags + 20-bit angles; general bc==32→raw floats. Current fields (bc {8,13}) - keep identical behavior. go test green; the new branches are dormant on the corpus (forward-compat). ✅ - -### P2.10 — mana/runetime patch sentinel guards -- Mana (builds ≤954) and simtime/runetime (all builds) patches apply unconditionally (field_patch.go:51-78); - clarity guards on the sentinel `±3.4028235e38` bounds. Adding the guards is a no-op on the corpus - (robustness/parity). **Keep manta's bespoke 4-bit `runeTimeDecoder`** (Outlanders case) and the `/30` - simtime API — guards only; touching the runetime decode math risks goldens. -- **Result:** Guarded the mana and runetime patches on the `±MaxFloat32` sentinel bounds (clarity-style); - kept the 4-bit runetime decoder and `/30` simtime. These fields always carry the sentinel, so the - guards always fire → go test green, identical goldens. Robustness/parity for edge-of-range fields. ✅ - -### P2.11 — outer-message size sanity bound -- `readOuterMessage` passes the size varint straight to `stream.readBytes` which does `make([]byte, n)` with - no bound (parser.go:219, stream.go:26) → a corrupt/huge varint can OOM before `io.ReadFull` errors. Add a - max-size guard (safely above the largest legitimate full packet) returning an error. Golden-neutral. -- **Result:** Added a 256 MiB `maxOuterMessageSize` guard before `readBytes` (every corpus replay is - ≤70 MB total, so no single message approaches it). go test green; only rejects corrupt sizes. ✅ - -### P2.12 — field-path depth-7 guard + comment -- `Push*` ops do `fp.last++` then index `fp.path[fp.last]` with no guard → depth>6 panics with a raw - out-of-range (field_path.go). Clarity bounds at 7 and fails loudly (S2LongFieldPathFormat.java:7-58). Keep - 7, add a cheap descriptive guard + comment. No behavior change (nothing exceeds 7 today). **Must stay `[7]int` - when P1.9 lands.** -- **Result:** Added `maxFieldPathDepth = 7` const + comment citing `S2LongFieldPathFormat`, used for the - `[maxFieldPathDepth]int` path array. The fixed array already fails loudly (recovered bounds-check) on - overflow, so no hot-path guard was added. No behavior change. go test green. ✅ - ---- - -## Phase 3 — invasive (after Phase 1 re-baseline) - -### P3.1 — typed entity state (eliminate per-field boxing) ⭐ -- **Now:** `fieldState.state` is `[]interface{}`; every decoder returns `interface{}` (field_decoder.go:7). - In Go, boxing float32 / `[]float32` / large uint64 / int32 (>255) into `interface{}` **always** heap-allocates, - on the hot write path. Measured: quantized float box ~10–12%, QAngle `[]float32` ~4.5–5.5%, signed int box - ~2%, unsigned int box — together **~20% of all allocs**. -- **Change (internal only):** store scalars unboxed — e.g. a tagged-union cell `{kind uint8; f float32; - i uint64; ref interface{}}` (strings/sub-state use `ref`); decoders write into the typed lane; box **lazily** - in `Entity.Get`/`GetFloat32` (reads are rare vs writes). For vectors, store per-element float32 cells in the - nested `fieldState` and reassemble the slice on `Get`. -- **Scope reality (verifier):** `*fieldState` shares the same `[]interface{}` slots as leaf values, distinguished - by `.(*fieldState)` in 10 sites (field.go, field_state.go) plus `Map()/getFieldPaths` — all must be reworked. - This is **not** a clarity mirror (clarity's `Object[]` also boxes; the JVM just amortizes via TLAB/escape - analysis) — it's a manta-specific optimization. risk medium, effort large. -- **Golden-critical:** `Get("m_flMaxMana").(float32)` (manta_test.go:694) must stay bit-identical — `Get`/ - `GetFloat32` must still return a boxed `float32`. Folds in P-decoder findings (quantized/qangle/int boxing) - which deliver 0 allocs alone and explicitly depend on this. Also unlocks cheap COW for P1.14. -- **Result:** Implemented as a **24-byte tagged-union `cell`** - `{ref interface{}; num uint32; kind cellKind}`. Scalars (float32/int32/uint32/bool/≤32-bit uint64 - handles) live inline in `num` — zero write-path alloc; reference values (string/`[]float32`/`[]byte`) and - the rare genuinely-64-bit ints (CStrongHandle/fixed64/int64/steamids) go in `ref`; nested tables in `ref` - as `*fieldState`. Decoders now return `cell`; values box lazily in `cell.iface()` only on `Get` (rare). - Public API unchanged — `Get` still returns `interface{}` with the exact dynamic type (entity_test's - `int32`/`uint64`/`bool`/`string` assertions and `expectHeroEntityMana` float all pass; the >32-bit steamid - proves 64-bit values aren't truncated). **vs P2: allocs −57.0% (10.26M→4.41M), sec −4.2%, B/op +2.6%** - (residual is the qangle/Vector `[]float32` backing arrays, left boxed to respect no-API-change on `Get`). - Tried a 32-byte `uint64`-num cell first (no 64-bit boxing) but it cost +12.9% B/op; the 24-byte variant - recovers that for just +1% allocs since 64-bit values are rare (profile-confirmed). go test ./... green. ✅ - ---- - -## Deferred / parked - -- **string-table Items map → dense slice** (perf, depends P2.1): indices become dense after the additive fix; - replicate clarity's `setValueForIndex(index bitCount` (caught by the parser recover → error), restoring - fail-fast. Add a huffman test that truncates an op stream and asserts a clean error. -- **Result:** all metrics flat (sec p=0.631, B/op p=0.971, allocs p=0.057 — the guard is free). - `TestReadFieldPathsTruncated` confirms a truncated stream now panics cleanly instead of looping - forever (an all-zero buffer = unbounded PlusOne run). go test green. ✅ - -### P4.4 — correct debug position() after word refill -- **Issue:** `position()` (reader.go) still assumes `bitCount <= 8`, but the word reader can leave 56/48/… - Wrong verbose-debug output only; no parsing impact. -- **Fix:** compute the logical bit position as `pos*8 - bitCount`. -- **Result:** flat (debug-only; called only under the `v(6)` guard, off in bench/test). go test green. ✅ - -### P4.5 — lock value-changing decoders + document Decision A -- **Decision A (owned):** the branch deliberately changes what `.Get` returns for a few fields, to match - clarity's correct representation. We are keeping these and owning them as intentional behavior changes: - - `int64` fields: `int32` (truncated >32 bits) → **`int64`** (fixes truncation; type change is forced by - the fix). - - `HeroID_t`: `uint32` → **`int32`** (signed, clarity parity). - - `HSequence`: `uint32` → **`int32`, value − 1** (−1 = "none" handle, clarity parity). - - `BloodType`: stayed `uint64`; only the *encoding* changed (fixed-8 vs varint), value identical on the - corpus — not an API change. - Downstream callers doing concrete type assertions on those specific fields may need updates. -- **Fix:** add decoder-level representation tests asserting the exact dynamic type **and** value for each - (deterministic, not replay-dependent), so the intended downstream-visible values are locked in CI — this - is what the review asked for ("prove values are what you intend, not just no-desync"). Also assert the - inline-vs-boxed uint64 split round-trips, including a `> 2^32` value to prove no truncation. -- **Result:** added `field_decoder_test.go` with `TestDecoderRepresentations` (exact type+value for - hSequence `int32`−1, HeroID_t signed `int32`, int64 full `int64`, BloodType `uint64` fixed-8, inline - `uint64` incl. `0xFFFFFFFF`, and a `>2^32` steamid + fixed64 with no truncation) and - `TestValueChangingDecoderWiring` (locks the `fieldTypeDecoders` map entries). Test-only; bench flat. - Decision A documented above. go test green. ✅ - -### P4.6 — clear reused buffers on error paths too (follow-up review) -- **Issue:** P4.2 only cleared `p.pendingMsgBuf` / `p.entityTuples` after *successful* dispatch. If - `callByPacketType` or an entity handler returns an error, the parser field still pointed at the - just-refilled backing array, so packet buffers / entity pointers stayed live. Low severity (the parse - aborts on error, so it's freed at parser GC; bounded to one packet), but a real inconsistency. -- **Fix:** dispatch into a result variable (`break` / labeled `break dispatch` on error), then a single - `clear()` + `[:0]` cleanup before a single return — so cleanup runs on success *and* error. Used this - result-variable pattern rather than a `defer func(){…}()` closure on purpose: the closure captures the - slice header (also stored on the Parser), forcing it to escape to the heap — a per-packet allocation in a - path that runs thousands of times. The result-variable form is alloc-free. (Panic mid-dispatch still - skips cleanup, but that hits the parser's top-level recover and tears the parser down, same as today.) -- **Result:** alloc- and B/op-neutral (4,478,290 allocs / 408.36M B identical to P4.5); sec flat. go test green. ✅