From d4954b3845e1441077ddd89e2a3c93384818c78e Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:35:27 -0500 Subject: [PATCH 01/11] P5.0: Phase 5 plan + baseline (profile-driven, post-#180) Fresh CPU/alloc profiles on master show a new profile shape after the Phase 1-4 work: unaligned readBytes in onCDemoPacket (13.5% of allocs), readBitsAsBytes growth (11.6%), protobuf envelope decode (~30% of allocs, 32.7% of alloc space in consumeBytes), fieldState.clone (19% of space), and the p.entities map (~9% CPU). Baseline (BenchmarkMatch8552595443, benchtime=10x, count=10, M4 Pro): sec/op 876.7m +/- 8% B/op 389.4Mi +/- 0% allocs 4.478M +/- 0% Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 659 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 659 insertions(+) create mode 100644 IMPROVEMENTS.md diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md new file mode 100644 index 0000000..e1f9b4c --- /dev/null +++ b/IMPROVEMENTS.md @@ -0,0 +1,659 @@ +# 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. ✅ + +--- + +## Phase 5 — post-merge profile-driven round (branch `jcoene/goal-perf-phase5`) + +Fresh CPU/alloc profiles were captured on master (post-#180) on the canonical replay +`8552595443`. The Phase 1–4 dominators (field-path slice, boxing, huffman walk) are gone; +the profile has a new shape. Same ground rules as before: no public API change, one +optimization per commit with benchstat vs the previous commit, full `go test ./...` +golden gate, commits only (no push, no PR). + +### Profile evidence (master @ 0efe7e1, `TestMatchNew8552595443`, M4 Pro) + +- **Alloc objects:** unaligned `reader.readBytes` 13.5% (all from `onCDemoPacket` embedded + messages — the inner stream is bit-shifted after the leading 6-bit `readUBitVar`, so every + message body takes the `make([]byte,n)` + per-byte loop) · `readBitsAsBytes` 11.6% (cap-0 + append growth per string-table value) · qangle `[]float32` 11.5% (known residual) · + protobuf `reflect.New` + `consume*Ptr` + `consumeBytes` ~30% combined · `parseStringTable` + 7.0% flat (`&stringTableItem` per item + items growth) · `fieldState.clone` 6.7% + + `newFieldState` 2.5% (baseline clone per entity create). +- **Alloc space:** protobuf `consumeBytes` **32.7%** (128 MB — every `bytes` field is copied: + `CDemoPacket.data`, `PacketEntities.entity_data`, `UpdateStringTable.string_data`) · + `fieldState.clone` 19.1% · unaligned `readBytes` 12.7%. +- **CPU:** `fieldState.set` 8.3% · `memclrNoHeapPointers` 8.3% (zeroing the fresh unaligned + `readBytes` buffers) · `mapaccess1_fast32` ~9% incl. inlined key probe (the `p.entities` + map in the PacketEntities update loop) · `readBits` 6.2% · decoder-lookup recursion ~3%. + +### Goal summary + +| ID | Goal | Type | Risk | Effort | Expected impact | +|----|------|------|------|--------|-----------------| +| P5.0 | re-baseline bench on master | infra | none | none | reference point | +| P5.1 | `readBitsAsBytes` exact prealloc + word fill | perf | low | small | −~10% allocs | +| P5.2 | demo-packet arena + word-copy unaligned `readBytes` | perf | med | small | −13% allocs, −12% B/op, CPU | +| P5.3 | `p.entities` map → dense slice | perf | low | small | −5–9% CPU | +| P5.4 | string-table item slab + prealloc + ring history + single lookup | perf | low | small | −~7% allocs | +| P5.5 | reader smalls: `readLeUintX` via accumulator when unaligned, `readString` prealloc | perf | low | small | −~1% allocs | +| P5.6 | hand-rolled envelope decode for internal-only hot messages | perf | med | medium | −25–30% B/op, −10%+ allocs | +| P5.7 | slab-allocated baseline clone | perf | med | medium | −~8% allocs | +| P5.8 | game-event eventid wire-peek, skip unmarshal when unhandled | perf | low | small | workload-dependent | +| P5.9 | prefix cache for `set`/decoder walk across sorted field paths | perf | med | medium | −5–8% CPU (attempt) | +| P5.10 | experiment: inline vec3 cell for qangle/vector residual | perf | med | small | measure; likely reject (P3.1 saw +12.9% B/op at 32B cells) | + +### Cumulative results (vs P5.0 baseline) — filled in during execution + +| After goal | sec/op | B/op | allocs/op | go test | +|------------|--------|------|-----------|---------| From 4f75f1e402be2907053814674d44901000c46839 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:38:00 -0500 Subject: [PATCH 02/11] P5.1: readBitsAsBytes exact prealloc + word fill Allocate the exact (n+7)/8 result up front instead of growing a cap-0 slice byte-at-a-time, and fill via readBits(32) words with a byte/bit tail. String-table values retain the slice, so one fresh allocation per value is the floor; this removes the append-growth allocations on top. benchstat vs P5.0 (BenchmarkMatch8552595443, 10x, count=10): sec/op 876.7m +/- 8% -> 756.3m +/- 1% -13.74% (p=0.000)* B/op 389.4Mi +/- 0% -> 384.4Mi +/- 0% -1.29% (p=0.000) allocs 4.478M +/- 0% -> 4.181M +/- 0% -6.64% (p=0.000) * P5.0 sec/op was thermally inflated (+/-8%); allocs/B are the reliable signal for this change. go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 15 +++++++++++++++ reader.go | 16 +++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index e1f9b4c..5cb147f 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -657,3 +657,18 @@ golden gate, commits only (no push, no PR). | After goal | sec/op | B/op | allocs/op | go test | |------------|--------|------|-----------|---------| +| P5.0 baseline (master 0efe7e1) | 876.7m ±8% | 389.4 MiB | 4.478M | PASS | +| P5.1 readBitsAsBytes prealloc | 756.3m ±1% (−13.7%*) | 384.4 MiB (−1.3%) | 4.181M (−6.6%) | PASS | + +\* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the +reliable P5.1 signal and 756m ±1% as the true current sec/op level. + +### P5.1 — `readBitsAsBytes` exact prealloc + word fill +- **Was:** `tmp := make([]byte, 0)` grown byte-at-a-time via `append(tmp, r.readByte())` — + several growth allocations per string-table value (362K objects, 11.6% of allocs). +- **Change:** allocate the exact `(n+7)/8` result up front (callers retain the slice, so a + fresh allocation per value is required) and fill via `readBits(32)` words, then a byte/bit + tail. Bit-stream equivalence: bits are consumed LSB-first, so four `readBits(8)` calls and + one LE-decoded `readBits(32)` yield identical bytes. +- **Result:** allocs/op 4.478M→4.181M (**−6.64%, −297K**), B/op −1.29%, sec −13.7% vs the + noisy P5.0 run (true level ~756m ±1%). go test green. ✅ diff --git a/reader.go b/reader.go index cfce8a4..c943cb8 100644 --- a/reader.go +++ b/reader.go @@ -390,13 +390,23 @@ func (r *reader) read3BitNormal() []float32 { // readBitsAsBytes reads the given number of bits in groups of bytes func (r *reader) readBitsAsBytes(n uint32) []byte { - tmp := make([]byte, 0) + // Allocate the exact result size up front (the caller retains the slice, so + // a fresh allocation is required) and fill a 32-bit word at a time instead + // of growing a cap-0 slice byte by byte. + tmp := make([]byte, (n+7)/8) + i := 0 + for n >= 32 { + binary.LittleEndian.PutUint32(tmp[i:], r.readBits(32)) + i += 4 + n -= 32 + } for n >= 8 { - tmp = append(tmp, r.readByte()) + tmp[i] = r.readByte() + i++ n -= 8 } if n > 0 { - tmp = append(tmp, byte(r.readBits(n))) + tmp[i] = byte(r.readBits(n)) } return tmp } From b26bf90b873dc970aa009975991e329ba168eae8 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:41:08 -0500 Subject: [PATCH 03/11] P5.2: demo-packet arena + word-copy unaligned readBytes The onCDemoPacket inner stream is bit-shifted after the leading 6-bit readUBitVar, so nearly every embedded message body hit the unaligned readBytes slow path: a fresh zeroed make([]byte, n) filled one readBits(8) at a time (422K allocs, 49.5MB, plus most of the 8% memclrNoHeapPointers CPU). New reader.readBytesInto(dst) copies unaligned data a 32-bit word at a time (readBytes' slow path routes through it), and onCDemoPacket carves message buffers from a single parser-level arena sized to the packet and reused across packets. Buffer lifetime matches pendingMsgBuf: dispatch only, and the protobuf unmarshal copies what it keeps. benchstat vs P5.1 (BenchmarkMatch8552595443, 10x, count=10): sec/op 756.3m +/- 1% -> 676.7m +/- 1% -10.52% (p=0.000) B/op 384.4Mi +/- 0% -> 336.3Mi +/- 0% -12.52% (p=0.000) allocs 4.181M +/- 0% -> 3.757M +/- 0% -10.13% (p=0.000) go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 14 ++++++++++++++ demo_packet.go | 25 ++++++++++++++++++++++--- parser.go | 1 + reader.go | 33 ++++++++++++++++++++++++++++++--- 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 5cb147f..1a18eeb 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -659,10 +659,24 @@ golden gate, commits only (no push, no PR). |------------|--------|------|-----------|---------| | P5.0 baseline (master 0efe7e1) | 876.7m ±8% | 389.4 MiB | 4.478M | PASS | | P5.1 readBitsAsBytes prealloc | 756.3m ±1% (−13.7%*) | 384.4 MiB (−1.3%) | 4.181M (−6.6%) | PASS | +| P5.2 demo-packet arena + word copy | 676.7m ±1% (−22.8%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.2 — demo-packet arena + word-copy unaligned `readBytes` +- **Was:** the `onCDemoPacket` inner stream is bit-shifted after the leading 6-bit + `readUBitVar`, so nearly every embedded message body hit the unaligned `readBytes` slow + path: a fresh zeroed `make([]byte, n)` (422K objects, 49.5 MB, plus most of the 8% + `memclrNoHeapPointers` CPU) filled one `readBits(8)` at a time. +- **Change:** (a) new `reader.readBytesInto(dst)` copies unaligned data a 32-bit word at a + time; `readBytes`'s slow path routes through it. (b) `onCDemoPacket` carves message + buffers from a single parser-level arena sized to `len(m.GetData())` (headers guarantee + the payload total fits) and reused across packets. Lifetime matches `pendingMsgBuf`: the + buffers only live until dispatch, and the protobuf unmarshal copies what it keeps. +- **Result:** sec/op 756.3m→676.7m (**−10.52%**, p=0.000), B/op 384.4→336.3 MiB + (**−12.52%**), allocs/op 4.181M→3.757M (**−10.13%, −424K**). go test green. ✅ + ### P5.1 — `readBitsAsBytes` exact prealloc + word fill - **Was:** `tmp := make([]byte, 0)` grown byte-at-a-time via `append(tmp, r.readByte())` — several growth allocations per string-table value (362K objects, 11.6% of allocs). diff --git a/demo_packet.go b/demo_packet.go index 0d809a3..5b1bce9 100644 --- a/demo_packet.go +++ b/demo_packet.go @@ -67,14 +67,33 @@ func (p *Parser) onCDemoPacket(m *dota.CDemoPacket) error { // and avoids a heap allocation per embedded message. ms := p.pendingMsgBuf[:0] + // The inner stream is bit-shifted after the leading 6-bit readUBitVar, so + // message bodies almost never sit on a byte boundary and must be copied out. + // Carve those copies from a single reused arena sized to the packet instead + // of allocating per message: the buffers only live until dispatch below (the + // protobuf unmarshal copies what it keeps), so reusing the arena across + // packets is safe for the same reason reusing pendingMsgBuf is. Message + // headers take space too, so the payload total always fits in len(data). + data := m.GetData() + if cap(p.packetArena) < len(data) { + p.packetArena = make([]byte, 0, len(data)) + } + arena := p.packetArena[: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. - r := newReader(m.GetData()) + r := newReader(data) for r.remBytes() > 0 { t := int32(r.readUBitVar()) size := r.readVarUint32() - buf := r.readBytes(size) - ms = append(ms, pendingMessage{p.Tick, t, buf}) + start := len(arena) + end := start + int(size) + if end > cap(arena) { + _panicf("onCDemoPacket: message size %d exceeds packet buffer", size) + } + arena = arena[:end] + r.readBytesInto(arena[start:end]) + ms = append(ms, pendingMessage{p.Tick, t, arena[start:end:end]}) } // Sort messages to ensure dependencies are met. For example, we need to diff --git a/parser.go b/parser.go index 2fdd228..7c1471f 100644 --- a/parser.go +++ b/parser.go @@ -52,6 +52,7 @@ type Parser struct { modifierTableEntryHandlers []ModifierTableEntryHandler serializers map[string]*serializer pendingMsgBuf pendingMessages + packetArena []byte snappyScratch []byte entityReader reader entityTuples []entityOpTuple diff --git a/reader.go b/reader.go index c943cb8..9f734f2 100644 --- a/reader.go +++ b/reader.go @@ -173,12 +173,39 @@ func (r *reader) readBytes(n uint32) []byte { } buf := make([]byte, n) - for i := uint32(0); i < n; i++ { - buf[i] = byte(r.readBits(8)) - } + r.readBytesInto(buf) return buf } +// readBytesInto fills dst with the next len(dst) bytes of the stream. Unlike +// readBytes it never allocates: the caller provides the destination, and the +// unaligned path copies a 32-bit word at a time rather than byte by byte. +func (r *reader) readBytesInto(dst []byte) { + n := uint32(len(dst)) + + // Fast path if we're byte aligned + if r.bitCount&7 == 0 { + if r.bitCount != 0 { + r.realign() + } + r.pos += n + if r.pos > r.size { + _panicf("readBytesInto: insufficient buffer (%d of %d)", r.pos, r.size) + } + copy(dst, r.buf[r.pos-n:r.pos]) + return + } + + i := 0 + for int(n)-i >= 4 { + binary.LittleEndian.PutUint32(dst[i:], r.readBits(32)) + i += 4 + } + for ; i < int(n); i++ { + dst[i] = byte(r.readBits(8)) + } +} + // readLeUint32 reads an little-endian uint32 func (r *reader) readLeUint32() uint32 { return binary.LittleEndian.Uint32(r.readBytes(4)) From e578771c9e8c97b30509fd51df4ffc40e94ba78c Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:43:54 -0500 Subject: [PATCH 04/11] P5.3: entities map -> dense slice Entity indices are 14-bit by the handle encoding, and the PacketEntities update loop did 1-3 map lookups per entity update (~9% CPU in mapaccess1_fast32). Replace map[int32]*Entity with a dense []*Entity of 1< 624.8m +/- 2% -7.66% (p=0.000) B/op 336.3Mi flat, allocs 3.757M flat (CPU-only change) go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 13 +++++++++++++ entity.go | 5 ++++- parser.go | 4 ++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 1a18eeb..2ccd62e 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -660,10 +660,23 @@ golden gate, commits only (no push, no PR). | P5.0 baseline (master 0efe7e1) | 876.7m ±8% | 389.4 MiB | 4.478M | PASS | | P5.1 readBitsAsBytes prealloc | 756.3m ±1% (−13.7%*) | 384.4 MiB (−1.3%) | 4.181M (−6.6%) | PASS | | P5.2 demo-packet arena + word copy | 676.7m ±1% (−22.8%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | +| P5.3 entities map → dense slice | 624.8m ±2% (−28.7%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.3 — `p.entities` map → dense slice +- **Was:** `p.entities` was a `map[int32]*Entity`; the PacketEntities update loop does 1–3 + lookups per entity update (~9% CPU in `mapaccess1_fast32` + inlined key probes). Deletion + stored `nil` *into* the map, so `FilterEntity` could pass nil entities to user callbacks — + a latent crash. +- **Change:** dense `[]*Entity` sized `1<= len(p.entities) { + return nil + } return p.entities[index] } @@ -207,7 +210,7 @@ func (p *Parser) FindEntityByHandle(handle uint64) *Entity { func (p *Parser) FilterEntity(fb func(*Entity) bool) []*Entity { entities := make([]*Entity, 0, 0) for _, et := range p.entities { - if fb(et) { + if et != nil && fb(et) { entities = append(entities, et) } } diff --git a/parser.go b/parser.go index 7c1471f..13009ca 100644 --- a/parser.go +++ b/parser.go @@ -42,7 +42,7 @@ type Parser struct { classesByName map[string]*class classIdSize uint32 classInfo bool - entities map[int32]*Entity + entities []*Entity entityFullPackets int entityHandlers []EntityHandler gameEventHandlers map[string][]GameEventHandler @@ -81,7 +81,7 @@ func NewStreamParser(r io.Reader) (*Parser, error) { classBaselineStates: make(map[int32]*fieldState), classesById: make(map[int32]*class), classesByName: make(map[string]*class), - entities: make(map[int32]*Entity), + entities: make([]*Entity, 1< Date: Wed, 1 Jul 2026 15:46:08 -0500 Subject: [PATCH 05/11] P5.4: string-table item slab + prealloc + ring history + single lookup parseStringTable allocated a &stringTableItem per item plus cap-0 items append growth, shifted the whole 32-entry key-history window per item once full, and the UpdateStringTable apply loop did up to four map lookups per item. Preallocate items and back them with a single []stringTableItem slab (cap bounded at 4096 so a corrupt numUpdates cannot OOM; pointer identity survives slab regrowth because items are only touched through the taken pointers). The key history becomes a fixed ring buffer with histCount-based indexing that reproduces the shift-window semantics exactly. The apply loop hoists to a single map lookup. benchstat vs P5.3 (BenchmarkMatch8552595443, 10x, count=10): sec/op 624.8m +/- 2% -> 618.7m +/- 4% -0.98% (p=0.043) B/op 336.3Mi +/- 0% -> 335.0Mi +/- 0% -0.38% (p=0.000) allocs 3.757M +/- 0% -> 3.597M +/- 0% -4.26% (p=0.000) go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 14 ++++++++++++ string_table.go | 58 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 2ccd62e..23e9639 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -661,10 +661,24 @@ golden gate, commits only (no push, no PR). | P5.1 readBitsAsBytes prealloc | 756.3m ±1% (−13.7%*) | 384.4 MiB (−1.3%) | 4.181M (−6.6%) | PASS | | P5.2 demo-packet arena + word copy | 676.7m ±1% (−22.8%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | | P5.3 entities map → dense slice | 624.8m ±2% (−28.7%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | +| P5.4 string-table slab + ring history | 618.7m ±4% (−29.4%) | 335.0 MiB (−14.0%) | 3.597M (−19.7%) | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.4 — string-table item slab + prealloc + ring history + single lookup +- **Was:** `parseStringTable` allocated a `&stringTableItem` per item plus cap-0 `items` + append growth (217K objects); the 32-entry key history shifted the whole window with a + `copy` per item once full; the `UpdateStringTable` apply loop did up to four map lookups + per item. +- **Change:** preallocate `items` and back it with a single `[]stringTableItem` slab + (bounded at 4096 cap so corrupt `numUpdates` can't OOM; pointer identity survives slab + regrowth since items are only touched through the taken pointers). Key history becomes a + fixed ring buffer (`histCount`-based indexing reproduces the shift-window semantics + exactly). Apply loop hoists to one lookup. +- **Result:** allocs/op 3.757M→3.597M (**−4.26%, −160K**), B/op −0.38%, sec −0.98% + (p=0.043). go test green (string-table goldens + full corpus). ✅ + ### P5.3 — `p.entities` map → dense slice - **Was:** `p.entities` was a `map[int32]*Entity`; the PacketEntities update loop does 1–3 lookups per entity update (~9% CPU in `mapaccess1_fast32` + inlined key probes). Deletion diff --git a/string_table.go b/string_table.go index 6d6a5c4..fb5d872 100644 --- a/string_table.go +++ b/string_table.go @@ -154,16 +154,15 @@ func (p *Parser) onCSVCMsg_UpdateStringTable(m *dota.CSVCMsg_UpdateStringTable) // Apply the updates to the parser state for _, item := range items { - index := item.Index - if _, ok := t.Items[index]; ok { - if item.Key != "" && item.Key != t.Items[index].Key { - t.Items[index].Key = item.Key + if existing, ok := t.Items[item.Index]; ok { + if item.Key != "" && item.Key != existing.Key { + existing.Key = item.Key } if len(item.Value) > 0 { - t.Items[index].Value = item.Value + existing.Value = item.Value } } else { - t.Items[index] = item + t.Items[item.Index] = item } } @@ -193,7 +192,16 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b } }() - items = make([]*stringTableItem, 0) + // Preallocate the result and back the items with a single slab so a large + // update costs two allocations instead of one per item plus append growth. + // The cap is bounded so a corrupt numUpdates cannot trigger a huge + // allocation; append simply grows past it if a table is legitimately larger. + prealloc := int(numUpdates) + if prealloc > 4096 { + prealloc = 4096 + } + items = make([]*stringTableItem, 0, prealloc) + slab := make([]stringTableItem, 0, prealloc) // Create a reader for the buffer r := newReader(buf) @@ -202,8 +210,13 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b // If the first item is at index 0 it will use a incr operation. index := int32(-1) - // Maintain a list of key history - keys := make([]string, 0, stringtableKeyHistorySize) + // Maintain a ring buffer of key history. histCount is the total number of + // keys ever appended; the ring holds the most recent + // stringtableKeyHistorySize of them, so history position pos (counted from + // the oldest retained key) lives at (histCount-size+pos) mod size once the + // ring is full, and at pos before that. + var keyHist [stringtableKeyHistorySize]string + histCount := 0 // Some tables have no data if len(buf) == 0 { @@ -252,10 +265,18 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b pos := r.readBits(5) size := r.readBits(5) - if int(pos) >= len(keys) { + retained := histCount + if retained > stringtableKeyHistorySize { + retained = stringtableKeyHistorySize + } + if int(pos) >= retained { key += r.readString() } else { - s := keys[pos] + abs := int(pos) + if histCount > stringtableKeyHistorySize { + abs = histCount - stringtableKeyHistorySize + int(pos) + } + s := keyHist[abs%stringtableKeyHistorySize] if int(size) > len(s) { key += s + r.readString() } else { @@ -266,12 +287,8 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b key = r.readString() } - if len(keys) >= stringtableKeyHistorySize { - copy(keys[0:], keys[1:]) - keys[len(keys)-1] = "" - keys = keys[:len(keys)-1] - } - keys = append(keys, key) + keyHist[histCount%stringtableKeyHistorySize] = key + histCount++ } // Some entries have a value. @@ -302,7 +319,12 @@ func parseStringTable(buf []byte, numUpdates int32, name string, userDataFixed b } } - items = append(items, &stringTableItem{index, key, value}) + // Append to the slab and take the address of the slab element. If the + // slab regrows, previously taken pointers keep referencing the old + // backing array; that is safe because items are only ever read and + // mutated through those pointers, never through the slab again. + slab = append(slab, stringTableItem{index, key, value}) + items = append(items, &slab[len(slab)-1]) } return items, nil From fbcd6799bb3ccfd7f9a74cd735b37f8c91614311 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:48:08 -0500 Subject: [PATCH 06/11] P5.5: reader smalls: readLeUintX via accumulator, readString prealloc Unaligned readLeUint32/64 allocated through the readBytes slow path (24K objects via fixed64Decoder and friends). Read them straight from the bit accumulator instead: bits are consumed LSB-first from little-endian bytes, so readBits(32) equals the LE decode of the next four stream bytes; readLeUint64 uses two accumulator words (readBits is capped at 32 bits per call). Aligned paths keep the zero-copy readBytes fast path and the exact pos contract the reader tests assert. readString starts at cap 32 instead of growing a cap-0 buffer per byte. benchstat vs P5.4 (BenchmarkMatch8552595443, 10x, count=10): sec/op 618.7m +/- 4% -> 618.8m +/- 2% ~ (p=0.481) B/op 335.0Mi +/- 0% -> 334.5Mi +/- 0% -0.16% (p=0.000) allocs 3.597M +/- 0% -> 3.540M +/- 0% -1.59% (p=0.000) go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 12 ++++++++++++ reader.go | 17 ++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 23e9639..0f32504 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -662,10 +662,22 @@ golden gate, commits only (no push, no PR). | P5.2 demo-packet arena + word copy | 676.7m ±1% (−22.8%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | | P5.3 entities map → dense slice | 624.8m ±2% (−28.7%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | | P5.4 string-table slab + ring history | 618.7m ±4% (−29.4%) | 335.0 MiB (−14.0%) | 3.597M (−19.7%) | PASS | +| P5.5 reader smalls (readLeUintX, readString) | 618.8m ±2% (−29.4%) | 334.5 MiB (−14.1%) | 3.540M (−20.9%) | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.5 — reader smalls: `readLeUintX` via accumulator, `readString` prealloc +- **Was:** unaligned `readLeUint32/64` allocated through the `readBytes` slow path (24K + objects via `fixed64Decoder` etc.); `readString` grew a cap-0 buffer byte by byte. +- **Change:** unaligned `readLeUint32` reads `readBits(32)` straight from the accumulator + (bit-stream LSB-first == LE byte decode); `readLeUint64` uses two accumulator words + (readBits is capped at 32). Aligned paths keep the zero-copy `readBytes` fast path and + the exact `pos` contract the reader tests assert. `readString` starts at cap 32 (stack- + allocatable when it doesn't escape). +- **Result:** allocs/op 3.597M→3.540M (**−1.59%, −57K**), B/op −0.16%, sec ~ (p=0.481). + go test green. ✅ + ### P5.4 — string-table item slab + prealloc + ring history + single lookup - **Was:** `parseStringTable` allocated a `&stringTableItem` per item plus cap-0 `items` append growth (217K objects); the 32-entry key history shifted the whole window with a diff --git a/reader.go b/reader.go index 9f734f2..e820652 100644 --- a/reader.go +++ b/reader.go @@ -208,11 +208,24 @@ func (r *reader) readBytesInto(dst []byte) { // readLeUint32 reads an little-endian uint32 func (r *reader) readLeUint32() uint32 { + // Unaligned: read straight from the bit accumulator. Bits are consumed + // LSB-first from little-endian bytes, so this equals the LE decode of the + // next four stream bytes without the readBytes slow-path allocation. The + // aligned path keeps the zero-copy readBytes fast path. + if r.bitCount&7 != 0 { + return r.readBits(32) + } return binary.LittleEndian.Uint32(r.readBytes(4)) } // readLeUint64 reads a little-endian uint64 func (r *reader) readLeUint64() uint64 { + // See readLeUint32: two accumulator words replace the unaligned readBytes + // allocation (readBits is capped at 32 bits per call). + if r.bitCount&7 != 0 { + lo := uint64(r.readBits(32)) + return lo | uint64(r.readBits(32))<<32 + } return binary.LittleEndian.Uint64(r.readBytes(8)) } @@ -324,7 +337,9 @@ func (r *reader) readStringN(n uint32) string { // readString reads a null terminated string func (r *reader) readString() string { - buf := make([]byte, 0) + // Most strings here are short field/table keys; a small starting capacity + // avoids the repeated growth of a cap-0 append chain. + buf := make([]byte, 0, 32) for { b := r.readByte() if b == 0 { From 62ab8b605c63ab3422a6f0ec9f61c5b7e9c3eda4 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:51:41 -0500 Subject: [PATCH 07/11] P5.6: hand-rolled envelope decode for hot internal messages Every CDemoPacket / CSVCMsg_PacketEntities / CSVCMsg_UpdateStringTable / CNETMsg_Tick went through the reflective protobuf unmarshal: reflect.New per message, a pointer alloc per scalar field, and - 32.7% of all allocated bytes - a fresh copy of every bytes payload. The replay's data was effectively copied twice through the proto layer. envelope_fast.go decodes these four envelopes by hand with protowire, aliasing the payload instead of copying, and calls scalar-arg core methods split out of the internal handlers. Gating: NewParser registers exactly one internal handler per list before returning, so len(list) == 1 means no user callbacks are registered; any user registration reverts that type to the full protobuf path, where the user-visible message owns its own copies as before. Aliasing lifetimes: entity_data/string_data alias the packet arena (stable through the dispatch loop, consumed synchronously; string-table values are copied out by readBitsAsBytes). CDemoPacket.data aliases the outer-message buffer (stable until the next readOuterMessage; processDemoPacket copies bodies into the arena before dispatch). benchstat vs P5.5 (BenchmarkMatch8552595443, 10x, count=10): sec/op 618.8m +/- 2% -> 580.5m +/- 0% -6.18% (p=0.000) B/op 334.5Mi +/- 0% -> 192.7Mi +/- 0% -42.39% (p=0.000) allocs 3.540M +/- 0% -> 2.615M +/- 0% -26.13% (p=0.000) go test ./... PASS (identical goldens; the goldens exercise the fast path since tests register OnEntity/OnGameEvent, not raw message callbacks) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 21 +++++ demo_packet.go | 16 +++- entity.go | 13 ++- envelope_fast.go | 214 +++++++++++++++++++++++++++++++++++++++++++++++ parser.go | 5 +- string_table.go | 16 +++- 6 files changed, 273 insertions(+), 12 deletions(-) create mode 100644 envelope_fast.go diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 0f32504..19b3da5 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -663,10 +663,31 @@ golden gate, commits only (no push, no PR). | P5.3 entities map → dense slice | 624.8m ±2% (−28.7%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | | P5.4 string-table slab + ring history | 618.7m ±4% (−29.4%) | 335.0 MiB (−14.0%) | 3.597M (−19.7%) | PASS | | P5.5 reader smalls (readLeUintX, readString) | 618.8m ±2% (−29.4%) | 334.5 MiB (−14.1%) | 3.540M (−20.9%) | PASS | +| **P5.6 hand-rolled envelope decode** | **580.5m ±0% (−33.8%)** | **192.7 MiB (−50.5%)** | **2.615M (−41.6%)** | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.6 — hand-rolled envelope decode for hot internal messages ⭐ headline win +- **Was:** every CDemoPacket / CSVCMsg_PacketEntities / CSVCMsg_UpdateStringTable / + CNETMsg_Tick went through the reflective protobuf unmarshal: `reflect.New` per message, + a pointer alloc per scalar field, and — 32.7% of all allocated bytes — a fresh copy of + every `bytes` payload. The replay's data was effectively copied twice through the proto + layer (outer `CDemoPacket.data`, then inner `entity_data`/`string_data`). +- **Change:** `envelope_fast.go` decodes these four envelopes by hand with `protowire`, + aliasing the payload instead of copying, and calls new scalar-arg core methods + (`processDemoPacket`/`processPacketEntities`/`processUpdateStringTable`) split out of the + internal handlers. **Gating:** NewParser registers exactly one internal handler per list + before returning, so `len(list) == 1` means no user callbacks; any user registration + reverts that type to the full protobuf path (user-visible messages own their copies, as + before). Aliasing lifetimes: `entity_data`/`string_data` alias the packet arena (stable + through the dispatch loop, consumed synchronously — string-table values are copied out by + `readBitsAsBytes`); `CDemoPacket.data` aliases the outer-message buffer (stable until the + next `readOuterMessage`; `processDemoPacket` copies bodies into the arena before dispatch). +- **Result:** B/op 334.5→192.7 MiB (**−42.39%**), allocs/op 3.540M→2.615M (**−26.13%, + −925K**), sec/op 618.8m→580.5m (**−6.18%**, p=0.000). go test green — the goldens + exercise the fast path (tests register OnEntity/OnGameEvent, not raw message callbacks). ✅ + ### P5.5 — reader smalls: `readLeUintX` via accumulator, `readString` prealloc - **Was:** unaligned `readLeUint32/64` allocated through the `readBytes` slow path (24K objects via `fixed64Decoder` etc.); `readString` grew a cap-0 buffer byte by byte. diff --git a/demo_packet.go b/demo_packet.go index 5b1bce9..c9fb219 100644 --- a/demo_packet.go +++ b/demo_packet.go @@ -60,6 +60,13 @@ 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 { + return p.processDemoPacket(m.GetData()) +} + +// processDemoPacket is the core of the CDemoPacket handler. It takes the raw +// packet payload so the fast envelope path (envelope_fast.go) can call it +// without materializing a proto message. +func (p *Parser) processDemoPacket(data []byte) error { // 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 @@ -74,7 +81,6 @@ func (p *Parser) onCDemoPacket(m *dota.CDemoPacket) error { // protobuf unmarshal copies what it keeps), so reusing the arena across // packets is safe for the same reason reusing pendingMsgBuf is. Message // headers take space too, so the payload total always fits in len(data). - data := m.GetData() if cap(p.packetArena) < len(data) { p.packetArena = make([]byte, 0, len(data)) } @@ -102,10 +108,12 @@ 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, stopping on handler error. + // Dispatch messages in order, stopping on handler error. dispatchPacket + // takes the fast envelope path for hot internal-only message types and + // falls back to the full protobuf callback path otherwise. var err error for i := range ms { - if err = p.Callbacks.callByPacketType(ms[i].t, ms[i].buf); err != nil { + if err = p.dispatchPacket(ms[i].t, ms[i].buf); err != nil { break } } @@ -129,7 +137,7 @@ func (p *Parser) onCDemoFullPacket(m *dota.CDemoFullPacket) error { // Then the CDemoPacket. if m.Packet != nil { - if err := p.onCDemoPacket(m.GetPacket()); err != nil { + if err := p.processDemoPacket(m.GetPacket().GetData()); err != nil { return err } } diff --git a/entity.go b/entity.go index de7c419..5c7d6fb 100644 --- a/entity.go +++ b/entity.go @@ -226,18 +226,25 @@ type entityOpTuple struct { // Internal Callback for OnCSVCMsg_PacketEntities. func (p *Parser) onCSVCMsg_PacketEntities(m *dota.CSVCMsg_PacketEntities) error { + return p.processPacketEntities(m.GetEntityData(), m.GetUpdatedEntries(), m.GetLegacyIsDelta()) +} + +// processPacketEntities is the core of the PacketEntities handler. It takes +// the three envelope fields the parser actually uses so the fast envelope +// path (envelope_fast.go) can call it without materializing a proto message. +func (p *Parser) processPacketEntities(entityData []byte, updatedEntries int32, isDelta bool) error { r := &p.entityReader - r.reset(m.GetEntityData()) + r.reset(entityData) var index = int32(-1) - var updates = int(m.GetUpdatedEntries()) + var updates = int(updatedEntries) var cmd uint32 var classId int32 var serial int32 var e *Entity var op EntityOp - if !m.GetLegacyIsDelta() { + if !isDelta { if p.entityFullPackets > 0 { return nil } diff --git a/envelope_fast.go b/envelope_fast.go new file mode 100644 index 0000000..597c903 --- /dev/null +++ b/envelope_fast.go @@ -0,0 +1,214 @@ +package manta + +import ( + "github.com/dotabuff/manta/dota" + "google.golang.org/protobuf/encoding/protowire" +) + +// Hand-rolled envelope decoders for the hottest internal messages. +// +// The reflective protobuf unmarshal allocates the message, a pointer per +// scalar field, and — dominating the profile — a fresh copy of every bytes +// field (CDemoPacket.data, CSVCMsg_PacketEntities.entity_data, +// CSVCMsg_UpdateStringTable.string_data effectively copy the whole replay a +// second time). The parser's internal handlers only need a few scalar fields +// plus the payload, and consume the payload synchronously, so when a message +// type has no user callbacks registered we can decode the envelope by hand +// with protowire and alias the payload instead of copying it. +// +// Gating: NewParser registers exactly one internal handler per relevant list +// before returning, and users can only register afterwards, so +// len(list) == 1 means "internal handler only". Any user registration makes +// the list longer and permanently reverts that type to the full protobuf +// path, where the user-visible message owns its own copies as before. +// +// Aliasing lifetimes: entity_data and string_data alias the demo-packet arena +// (p.packetArena), which is stable for the duration of the packet's dispatch +// loop; both are fully consumed before the handler returns (string-table +// values are copied out by readBitsAsBytes/readString). CDemoPacket.data +// aliases the outer-message buffer, which is stable until the next +// readOuterMessage; processDemoPacket copies message bodies into the arena +// before dispatching. + +// dispatchDemo routes an outer demo message, taking the fast envelope path +// for CDemoPacket/CDemoSignonPacket when only the internal handler is +// registered. +func (p *Parser) dispatchDemo(t int32, buf []byte) error { + switch t { + case int32(dota.EDemoCommands_DEM_Packet): + if len(p.Callbacks.onCDemoPacket) == 1 { + return p.fastDemoPacket(buf) + } + case int32(dota.EDemoCommands_DEM_SignonPacket): + if len(p.Callbacks.onCDemoSignonPacket) == 1 { + return p.fastDemoPacket(buf) + } + } + return p.Callbacks.callByDemoType(t, buf) +} + +// dispatchPacket routes an embedded packet message, taking the fast envelope +// path for the hot types when only the internal handler is registered. +func (p *Parser) dispatchPacket(t int32, buf []byte) error { + switch t { + case int32(dota.NET_Messages_net_Tick): + if len(p.Callbacks.onCNETMsg_Tick) == 1 { + return p.fastNetTick(buf) + } + case int32(dota.SVC_Messages_svc_UpdateStringTable): + if len(p.Callbacks.onCSVCMsg_UpdateStringTable) == 1 { + return p.fastUpdateStringTable(buf) + } + case int32(dota.SVC_Messages_svc_PacketEntities): + if len(p.Callbacks.onCSVCMsg_PacketEntities) == 1 { + return p.fastPacketEntities(buf) + } + } + return p.Callbacks.callByPacketType(t, buf) +} + +// fastDemoPacket decodes the CDemoPacket envelope: data = field 3 (bytes). +func (p *Parser) fastDemoPacket(buf []byte) error { + var data []byte + for len(buf) > 0 { + num, typ, n := protowire.ConsumeTag(buf) + if n < 0 { + return _errorf("fastDemoPacket: invalid tag") + } + buf = buf[n:] + if num == 3 && typ == protowire.BytesType { + v, n := protowire.ConsumeBytes(buf) + if n < 0 { + return _errorf("fastDemoPacket: invalid data field") + } + data = v + buf = buf[n:] + continue + } + n = protowire.ConsumeFieldValue(num, typ, buf) + if n < 0 { + return _errorf("fastDemoPacket: invalid field %d", num) + } + buf = buf[n:] + } + return p.processDemoPacket(data) +} + +// fastNetTick decodes the CNETMsg_Tick envelope: tick = field 1 (varint). +func (p *Parser) fastNetTick(buf []byte) error { + var tick uint64 + for len(buf) > 0 { + num, typ, n := protowire.ConsumeTag(buf) + if n < 0 { + return _errorf("fastNetTick: invalid tag") + } + buf = buf[n:] + if num == 1 && typ == protowire.VarintType { + v, n := protowire.ConsumeVarint(buf) + if n < 0 { + return _errorf("fastNetTick: invalid tick field") + } + tick = v + buf = buf[n:] + continue + } + n = protowire.ConsumeFieldValue(num, typ, buf) + if n < 0 { + return _errorf("fastNetTick: invalid field %d", num) + } + buf = buf[n:] + } + p.NetTick = uint32(tick) + return nil +} + +// fastUpdateStringTable decodes the CSVCMsg_UpdateStringTable envelope: +// table_id = 1 (varint), num_changed_entries = 2 (varint), +// string_data = 3 (bytes). +func (p *Parser) fastUpdateStringTable(buf []byte) error { + var tableId, numChanged int32 + var data []byte + for len(buf) > 0 { + num, typ, n := protowire.ConsumeTag(buf) + if n < 0 { + return _errorf("fastUpdateStringTable: invalid tag") + } + buf = buf[n:] + switch { + case num == 1 && typ == protowire.VarintType: + v, n := protowire.ConsumeVarint(buf) + if n < 0 { + return _errorf("fastUpdateStringTable: invalid table_id") + } + tableId = int32(v) + buf = buf[n:] + case num == 2 && typ == protowire.VarintType: + v, n := protowire.ConsumeVarint(buf) + if n < 0 { + return _errorf("fastUpdateStringTable: invalid num_changed_entries") + } + numChanged = int32(v) + buf = buf[n:] + case num == 3 && typ == protowire.BytesType: + v, n := protowire.ConsumeBytes(buf) + if n < 0 { + return _errorf("fastUpdateStringTable: invalid string_data") + } + data = v + buf = buf[n:] + default: + n = protowire.ConsumeFieldValue(num, typ, buf) + if n < 0 { + return _errorf("fastUpdateStringTable: invalid field %d", num) + } + buf = buf[n:] + } + } + return p.processUpdateStringTable(tableId, numChanged, data) +} + +// fastPacketEntities decodes the CSVCMsg_PacketEntities envelope: +// updated_entries = 2 (varint), legacy_is_delta = 3 (varint bool), +// entity_data = 7 (bytes). +func (p *Parser) fastPacketEntities(buf []byte) error { + var updatedEntries int32 + var isDelta bool + var entityData []byte + for len(buf) > 0 { + num, typ, n := protowire.ConsumeTag(buf) + if n < 0 { + return _errorf("fastPacketEntities: invalid tag") + } + buf = buf[n:] + switch { + case num == 2 && typ == protowire.VarintType: + v, n := protowire.ConsumeVarint(buf) + if n < 0 { + return _errorf("fastPacketEntities: invalid updated_entries") + } + updatedEntries = int32(v) + buf = buf[n:] + case num == 3 && typ == protowire.VarintType: + v, n := protowire.ConsumeVarint(buf) + if n < 0 { + return _errorf("fastPacketEntities: invalid legacy_is_delta") + } + isDelta = v != 0 + buf = buf[n:] + case num == 7 && typ == protowire.BytesType: + v, n := protowire.ConsumeBytes(buf) + if n < 0 { + return _errorf("fastPacketEntities: invalid entity_data") + } + entityData = v + buf = buf[n:] + default: + n = protowire.ConsumeFieldValue(num, typ, buf) + if n < 0 { + return _errorf("fastPacketEntities: invalid field %d", num) + } + buf = buf[n:] + } + } + return p.processPacketEntities(entityData, updatedEntries, isDelta) +} diff --git a/parser.go b/parser.go index 13009ca..3bd2ae2 100644 --- a/parser.go +++ b/parser.go @@ -162,7 +162,10 @@ func (p *Parser) Start() (err error) { p.Tick = msg.tick - if err = p.Callbacks.callByDemoType(msg.typeId, msg.data); err != nil { + // dispatchDemo takes the fast envelope path for CDemoPacket when only + // the internal handler is registered, falling back to the full + // protobuf callback path otherwise. + if err = p.dispatchDemo(msg.typeId, msg.data); err != nil { return } } diff --git a/string_table.go b/string_table.go index fb5d872..f8378a7 100644 --- a/string_table.go +++ b/string_table.go @@ -136,18 +136,26 @@ func (p *Parser) onCSVCMsg_CreateStringTable(m *dota.CSVCMsg_CreateStringTable) // Internal callback for CSVCMsg_UpdateStringTable. func (p *Parser) onCSVCMsg_UpdateStringTable(m *dota.CSVCMsg_UpdateStringTable) error { + return p.processUpdateStringTable(m.GetTableId(), m.GetNumChangedEntries(), m.GetStringData()) +} + +// processUpdateStringTable is the core of the UpdateStringTable handler. It +// takes the three envelope fields the parser actually uses so the fast +// envelope path (envelope_fast.go) can call it without materializing a proto +// message. +func (p *Parser) processUpdateStringTable(tableId, numChanged int32, data []byte) error { // TODO: integrate - t, ok := p.stringTables.Tables[m.GetTableId()] + t, ok := p.stringTables.Tables[tableId] if !ok { - _panicf("missing string table %d", m.GetTableId()) + _panicf("missing string table %d", tableId) } if v(5) { - _debugf("tick=%d name=%s changedEntries=%d size=%d", p.Tick, t.name, m.GetNumChangedEntries(), len(m.GetStringData())) + _debugf("tick=%d name=%s changedEntries=%d size=%d", p.Tick, t.name, numChanged, len(data)) } // Parse the updates out of the string table data - items, err := parseStringTable(m.GetStringData(), m.GetNumChangedEntries(), t.name, t.userDataFixedSize, t.userDataSizeBits, t.flags, t.varintBitCounts) + items, err := parseStringTable(data, numChanged, t.name, t.userDataFixedSize, t.userDataSizeBits, t.flags, t.varintBitCounts) if err != nil { return err } From f015082675f1d2fb70c86314f5f79154ea679425 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:53:30 -0500 Subject: [PATCH 08/11] P5.7: slab-allocated baseline clone clone() allocated a *fieldState plus a []cell array per nested state on every entity create (210K objects on the profile). A counting pre-pass now sizes two slabs ([]fieldState, []cell) and the copy carves from them; vector/blob leaves keep their individual deep copies (P4.1 semantics). Cell slices are cap-limited so a later set() grow allocates a fresh array instead of stomping a neighbouring state's arena region; each entity's slabs are private and die with the entity. benchstat vs P5.6 (BenchmarkMatch8552595443, 10x, count=10): sec/op 580.5m +/- 0% -> 574.5m +/- 1% -1.03% (p=0.001) B/op 192.7Mi +/- 0% -> 194.4Mi +/- 0% +0.92% (p=0.000) allocs 2.615M +/- 0% -> 2.508M +/- 0% -4.10% (p=0.000) The small B/op cost is slab retention overhead, traded for object count (which drives GC scan work). go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 11 +++++++++ field_state.go | 59 ++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 19b3da5..f0f6398 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -664,10 +664,21 @@ golden gate, commits only (no push, no PR). | P5.4 string-table slab + ring history | 618.7m ±4% (−29.4%) | 335.0 MiB (−14.0%) | 3.597M (−19.7%) | PASS | | P5.5 reader smalls (readLeUintX, readString) | 618.8m ±2% (−29.4%) | 334.5 MiB (−14.1%) | 3.540M (−20.9%) | PASS | | **P5.6 hand-rolled envelope decode** | **580.5m ±0% (−33.8%)** | **192.7 MiB (−50.5%)** | **2.615M (−41.6%)** | PASS | +| P5.7 slab-allocated baseline clone | 574.5m ±1% (−34.5%) | 194.4 MiB (−50.1%) | 2.508M (−44.0%) | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.7 — slab-allocated baseline clone +- **Was:** `clone()` allocated a `*fieldState` plus a `[]cell` array per nested state on + every entity create (210K objects, 19% of alloc space before P5.6). +- **Change:** a counting pre-pass sizes two slabs (`[]fieldState`, `[]cell`) and the copy + carves from them; vector/blob leaves keep their individual deep copies (P4.1 semantics). + Cell slices are cap-limited so a later `set` grow allocates fresh instead of stomping a + neighbour's arena region; each entity's slabs are private and die with it. +- **Result:** allocs/op 2.615M→2.508M (**−4.10%, −107K**), sec −1.03% (p=0.001), B/op + +0.92% (slab retention overhead — a good trade for object count). go test green. ✅ + ### P5.6 — hand-rolled envelope decode for hot internal messages ⭐ headline win - **Was:** every CDemoPacket / CSVCMsg_PacketEntities / CSVCMsg_UpdateStringTable / CNETMsg_Tick went through the reflective protobuf unmarshal: `reflect.New` per message, diff --git a/field_state.go b/field_state.go index 5a5e830..854d5f6 100644 --- a/field_state.go +++ b/field_state.go @@ -142,20 +142,63 @@ func (s *fieldState) set(fp *fieldPath, c cell) { // []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. +// +// The copy is carved from two slab allocations (one []fieldState, one []cell) +// sized by a counting pre-pass, instead of allocating a struct and a cell +// array per nested state. Cell slices are cap-limited to their exact length, +// so a later grow in set() allocates a fresh array rather than stomping a +// neighbouring state's arena region. An entity's clone shares its slabs with +// no other entity, so the slabs die with the entity. func (s *fieldState) clone() *fieldState { - c := &fieldState{state: make([]cell, len(s.state))} - copy(c.state, s.state) - for i := range c.state { - switch v := c.state[i].ref.(type) { + nStates, nCells := s.cloneSize() + a := cloneArena{ + states: make([]fieldState, nStates), + cells: make([]cell, nCells), + } + root := &a.states[0] + a.states = a.states[1:] + s.cloneInto(root, &a) + return root +} + +// cloneArena holds the remaining unassigned slab space during a clone. +type cloneArena struct { + states []fieldState + cells []cell +} + +// cloneSize counts the nested states and total cells reachable from s. +func (s *fieldState) cloneSize() (nStates, nCells int) { + nStates, nCells = 1, len(s.state) + for i := range s.state { + if sub, ok := s.state[i].sub(); ok { + cs, cc := sub.cloneSize() + nStates += cs + nCells += cc + } + } + return +} + +// cloneInto deep-copies s into dst, carving all nested storage from the arena. +func (s *fieldState) cloneInto(dst *fieldState, a *cloneArena) { + n := len(s.state) + dst.state = a.cells[:n:n] + a.cells = a.cells[n:] + copy(dst.state, s.state) + for i := range dst.state { + switch v := dst.state[i].ref.(type) { case *fieldState: - c.state[i].ref = v.clone() + sub := &a.states[0] + a.states = a.states[1:] + v.cloneInto(sub, a) + dst.state[i].ref = sub case []float32: - c.state[i].ref = append([]float32(nil), v...) + dst.state[i].ref = append([]float32(nil), v...) case []byte: - c.state[i].ref = append([]byte(nil), v...) + dst.state[i].ref = append([]byte(nil), v...) } } - return c } func max(a, b int) int { From 0dc7b704771cc85e0ce6f96e67b2c68f16daafec Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 15:55:44 -0500 Subject: [PATCH 09/11] P5.8: game-event eventid wire-peek Every CMsgSource1LegacyGameEvent was fully unmarshaled (message plus a reflect.New per key) before the internal handler checked whether any handler is registered for the event name and usually returned nil. skipGameEvent peeks the eventid varint (field 2) with protowire; a known event with no registered handlers skips the unmarshal entirely. Unknown ids and absent fields fall back to the full path, preserving its error behaviour exactly. benchstat vs P5.7 (BenchmarkMatch8552595443, 10x, count=10): sec/op 574.5m +/- 1% -> 572.3m +/- 1% ~ (p=0.052) B/op 194.4Mi +/- 0% -> 193.8Mi +/- 0% -0.32% (p=0.000) allocs 2.508M +/- 0% -> 2.492M +/- 0% -0.63% (p=0.000) Modest on this bench because it registers a combat-log handler and combat-log events dominate the event stream; the skip is worth more to consumers that register fewer (or no) game-event handlers. go test ./... PASS (identical goldens) Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 12 ++++++++++++ envelope_fast.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index f0f6398..b6c2051 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -665,10 +665,22 @@ golden gate, commits only (no push, no PR). | P5.5 reader smalls (readLeUintX, readString) | 618.8m ±2% (−29.4%) | 334.5 MiB (−14.1%) | 3.540M (−20.9%) | PASS | | **P5.6 hand-rolled envelope decode** | **580.5m ±0% (−33.8%)** | **192.7 MiB (−50.5%)** | **2.615M (−41.6%)** | PASS | | P5.7 slab-allocated baseline clone | 574.5m ±1% (−34.5%) | 194.4 MiB (−50.1%) | 2.508M (−44.0%) | PASS | +| P5.8 game-event eventid wire-peek | 572.3m ±1% (−34.7%) | 193.8 MiB (−50.2%) | 2.492M (−44.3%) | PASS | \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.8 — game-event eventid wire-peek +- **Was:** every `CMsgSource1LegacyGameEvent` was fully unmarshaled (message + a + `reflect.New` per key via `consumeMessageSliceInfo`) before the internal handler checked + whether any handler is registered for the event name and usually returned nil. +- **Change:** `skipGameEvent` peeks the eventid varint (field 2) with protowire; a known + event with no registered handlers skips the unmarshal entirely. Unknown ids and absent + fields fall back to the full path, preserving its error behaviour exactly. +- **Result:** allocs/op −0.63%, B/op −0.32%, sec ~ (p=0.052). Modest on this bench because + it registers a combat-log handler and combat-log events dominate the event stream; the + skip is worth more to consumers that register fewer (or no) game-event handlers. ✅ + ### P5.7 — slab-allocated baseline clone - **Was:** `clone()` allocated a `*fieldState` plus a `[]cell` array per nested state on every entity create (210K objects, 19% of alloc space before P5.6). diff --git a/envelope_fast.go b/envelope_fast.go index 597c903..4c0bc89 100644 --- a/envelope_fast.go +++ b/envelope_fast.go @@ -63,10 +63,47 @@ func (p *Parser) dispatchPacket(t int32, buf []byte) error { if len(p.Callbacks.onCSVCMsg_PacketEntities) == 1 { return p.fastPacketEntities(buf) } + case int32(dota.EBaseGameEvents_GE_Source1LegacyGameEvent): + if len(p.Callbacks.onCMsgSource1LegacyGameEvent) == 1 && p.skipGameEvent(buf) { + return nil + } } return p.Callbacks.callByPacketType(t, buf) } +// skipGameEvent peeks the eventid (field 2, varint) of a +// CMsgSource1LegacyGameEvent and reports whether the event is a known type +// with no registered handlers, in which case the full unmarshal (a message +// plus a reflect.New per key) can be skipped. The internal handler returns +// nil for exactly that case; unknown event ids fall back to the full path so +// its error behaviour is preserved. +func (p *Parser) skipGameEvent(buf []byte) bool { + for len(buf) > 0 { + num, typ, n := protowire.ConsumeTag(buf) + if n < 0 { + return false + } + buf = buf[n:] + if num == 2 && typ == protowire.VarintType { + v, n := protowire.ConsumeVarint(buf) + if n < 0 { + return false + } + name, ok := p.gameEventNames[int32(v)] + if !ok { + return false + } + return p.gameEventHandlers[name] == nil + } + n = protowire.ConsumeFieldValue(num, typ, buf) + if n < 0 { + return false + } + buf = buf[n:] + } + return false +} + // fastDemoPacket decodes the CDemoPacket envelope: data = field 3 (bytes). func (p *Parser) fastDemoPacket(buf []byte) error { var data []byte From 3bb37d06fd858289541116192c6a82742acf8061 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 16:03:10 -0500 Subject: [PATCH 10/11] P5.9/P5.10: document skipped and rejected experiments; Phase 5 summary P5.9 (prefix cache for the set/decoder walk across sorted field paths) measured statistically flat on sec/op (p=0.481 vs a verified-clean P5.8 baseline) with identical allocs/B - most field paths in this workload are depth-1, where the cache does nothing. Reverted per the keep-only-if-confirmed rule. P5.10 (widen cell to 32 bytes with inline vec2/vec3 float lanes) cut allocs -17.9% but cost +12.0% B/op and +1.87% sec/op (p=0.000): the wider cell inflates every state array and clone slab, and the extra copy bytes outweigh the GC win, consistent with P3.1's earlier 32-byte-cell result. Reverted. Phase 5 final (verified-clean re-measure at P5.8): sec/op 573.9m +/- 1% (vs ~750-756m credible pre-phase level: ~-24%) B/op 193.8Mi (-50.2% vs P5.0) allocs 2.492M (-44.3% vs P5.0) Cumulative vs the original pre-Phase-1 baseline: sec -62%, B/op -75%, allocs -88%. Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index b6c2051..5471743 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -666,10 +666,49 @@ golden gate, commits only (no push, no PR). | **P5.6 hand-rolled envelope decode** | **580.5m ±0% (−33.8%)** | **192.7 MiB (−50.5%)** | **2.615M (−41.6%)** | PASS | | P5.7 slab-allocated baseline clone | 574.5m ±1% (−34.5%) | 194.4 MiB (−50.1%) | 2.508M (−44.0%) | PASS | | P5.8 game-event eventid wire-peek | 572.3m ±1% (−34.7%) | 193.8 MiB (−50.2%) | 2.492M (−44.3%) | PASS | +| P5.9 prefix cache | _skipped (measured flat vs verified-clean baseline)_ | — | — | — | +| P5.10 inline vec2/vec3 cell | _rejected (allocs −17.9% but B/op +12.0%, sec +1.9%)_ | — | — | — | +| **End of Phase 5 (verified-clean re-measure)** | **573.9m ±1%** | **193.8 MiB** | **2.492M** | PASS | + +**Phase 5 totals.** vs the P5.0 nominal baseline: sec −34.5%, B/op −50.2%, allocs −44.3%. +The P5.0 sec sample was thermally inflated (±8%); against the credible pre-phase level of +~750–756m (P4's cooled measure / the P5.1 run), the honest sec/op win is **~−24%**. The +B/op and allocs columns are ±0% deterministic throughout. Cumulative vs the original P0 +baseline (pre-Phase-1: 1.523 s, 791.5 MiB, 20.75M allocs): **sec −62%, B/op −75%, +allocs −88%**. + +**Remaining targets (final profile, post-P5.8).** Alloc objects: qangle `[]float32` boxes +now #1 at 20.8% (only fixable by widening the cell — measured a net loss in P5.10 — or by +an API change); residual protobuf unmarshal ~25% (messages users register for: game-event +keys `consumeStringPtr`/`reflect.New`, user-message types — shrinkable only by extending +the fast-envelope set); `readBitsAsBytes` 7.7% (irreducible: one alloc per retained +string-table value); `newFieldState` in `set` 4.6%; `cloneInto` slabs 3.9%. CPU (in-memory +bench): `readBits` ~10%, `fieldState.set` ~4.5%, `readFieldPaths` ~10% cum — the bit-reader +and field-path machinery are now the floor. Parked ideas unchanged from earlier phases: +string-table Items map→dense slice, CDemoStringTables reconcile, COW baseline clone +(slab clone reduced the pressure; COW would also cut the copy bytes). \* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the reliable P5.1 signal and 756m ±1% as the true current sec/op level. +### P5.10 — inline vec2/vec3 cell — **REJECTED (measured)** +- **Attempted:** widen `cell` from 24 to 32 bytes (two extra float lanes) so QAngle/Vector/ + Vector2D/normal values store inline instead of boxing a `[]float32` per write, boxing + lazily on `Get` (fresh slice per read). +- **Measured:** allocs −17.9% (−447K) — but B/op **+12.0%** (+23 MiB; the wider cell + inflates every state array and clone slab) and sec/op **+1.87% slower** (p=0.000): the + extra copy/clone bytes outweigh the GC win, consistent with P3.1's 32-byte-cell attempt + (+12.9% B/op). Reverted; the vector residual (now ~21% of remaining allocs) stays the + known cost of the no-API-change constraint. + +### P5.9 — prefix cache for `set`/decoder walk — **SKIPPED** +- **Attempted:** cache the resolved depth-1 sub-state across consecutive sorted field paths + sharing `path[0]` (`setDepth`/`subAt` split), skipping the depth-0 walk step per field. +- **Measured flat:** sec/op ~ (p=0.481 vs a verified-clean P5.8 baseline), allocs/B + identical. Most field paths in this workload are depth-1 (`last == 0`), where the cache + does nothing, and the depth-0 walk step it skips is already just a bounds+kind check. + Reverted per the keep-only-if-confirmed rule. + ### P5.8 — game-event eventid wire-peek - **Was:** every `CMsgSource1LegacyGameEvent` was fully unmarshaled (message + a `reflect.New` per key via `consumeMessageSliceInfo`) before the internal handler checked From b5a6645e842871a98ac173eecec60037d846d606 Mon Sep 17 00:00:00 2001 From: Jason Coene Date: Wed, 1 Jul 2026 18:10:06 -0500 Subject: [PATCH 11/11] Remove IMPROVEMENTS.md from the repository The Phase 5 plan and results are preserved in the commit messages and kept locally; the tracking document does not belong in the repo. Co-Authored-By: Claude Fable 5 --- IMPROVEMENTS.md | 810 ------------------------------------------------ 1 file changed, 810 deletions(-) delete mode 100644 IMPROVEMENTS.md diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md deleted file mode 100644 index 5471743..0000000 --- a/IMPROVEMENTS.md +++ /dev/null @@ -1,810 +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. ✅ - ---- - -## Phase 5 — post-merge profile-driven round (branch `jcoene/goal-perf-phase5`) - -Fresh CPU/alloc profiles were captured on master (post-#180) on the canonical replay -`8552595443`. The Phase 1–4 dominators (field-path slice, boxing, huffman walk) are gone; -the profile has a new shape. Same ground rules as before: no public API change, one -optimization per commit with benchstat vs the previous commit, full `go test ./...` -golden gate, commits only (no push, no PR). - -### Profile evidence (master @ 0efe7e1, `TestMatchNew8552595443`, M4 Pro) - -- **Alloc objects:** unaligned `reader.readBytes` 13.5% (all from `onCDemoPacket` embedded - messages — the inner stream is bit-shifted after the leading 6-bit `readUBitVar`, so every - message body takes the `make([]byte,n)` + per-byte loop) · `readBitsAsBytes` 11.6% (cap-0 - append growth per string-table value) · qangle `[]float32` 11.5% (known residual) · - protobuf `reflect.New` + `consume*Ptr` + `consumeBytes` ~30% combined · `parseStringTable` - 7.0% flat (`&stringTableItem` per item + items growth) · `fieldState.clone` 6.7% + - `newFieldState` 2.5% (baseline clone per entity create). -- **Alloc space:** protobuf `consumeBytes` **32.7%** (128 MB — every `bytes` field is copied: - `CDemoPacket.data`, `PacketEntities.entity_data`, `UpdateStringTable.string_data`) · - `fieldState.clone` 19.1% · unaligned `readBytes` 12.7%. -- **CPU:** `fieldState.set` 8.3% · `memclrNoHeapPointers` 8.3% (zeroing the fresh unaligned - `readBytes` buffers) · `mapaccess1_fast32` ~9% incl. inlined key probe (the `p.entities` - map in the PacketEntities update loop) · `readBits` 6.2% · decoder-lookup recursion ~3%. - -### Goal summary - -| ID | Goal | Type | Risk | Effort | Expected impact | -|----|------|------|------|--------|-----------------| -| P5.0 | re-baseline bench on master | infra | none | none | reference point | -| P5.1 | `readBitsAsBytes` exact prealloc + word fill | perf | low | small | −~10% allocs | -| P5.2 | demo-packet arena + word-copy unaligned `readBytes` | perf | med | small | −13% allocs, −12% B/op, CPU | -| P5.3 | `p.entities` map → dense slice | perf | low | small | −5–9% CPU | -| P5.4 | string-table item slab + prealloc + ring history + single lookup | perf | low | small | −~7% allocs | -| P5.5 | reader smalls: `readLeUintX` via accumulator when unaligned, `readString` prealloc | perf | low | small | −~1% allocs | -| P5.6 | hand-rolled envelope decode for internal-only hot messages | perf | med | medium | −25–30% B/op, −10%+ allocs | -| P5.7 | slab-allocated baseline clone | perf | med | medium | −~8% allocs | -| P5.8 | game-event eventid wire-peek, skip unmarshal when unhandled | perf | low | small | workload-dependent | -| P5.9 | prefix cache for `set`/decoder walk across sorted field paths | perf | med | medium | −5–8% CPU (attempt) | -| P5.10 | experiment: inline vec3 cell for qangle/vector residual | perf | med | small | measure; likely reject (P3.1 saw +12.9% B/op at 32B cells) | - -### Cumulative results (vs P5.0 baseline) — filled in during execution - -| After goal | sec/op | B/op | allocs/op | go test | -|------------|--------|------|-----------|---------| -| P5.0 baseline (master 0efe7e1) | 876.7m ±8% | 389.4 MiB | 4.478M | PASS | -| P5.1 readBitsAsBytes prealloc | 756.3m ±1% (−13.7%*) | 384.4 MiB (−1.3%) | 4.181M (−6.6%) | PASS | -| P5.2 demo-packet arena + word copy | 676.7m ±1% (−22.8%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | -| P5.3 entities map → dense slice | 624.8m ±2% (−28.7%) | 336.3 MiB (−13.6%) | 3.757M (−16.1%) | PASS | -| P5.4 string-table slab + ring history | 618.7m ±4% (−29.4%) | 335.0 MiB (−14.0%) | 3.597M (−19.7%) | PASS | -| P5.5 reader smalls (readLeUintX, readString) | 618.8m ±2% (−29.4%) | 334.5 MiB (−14.1%) | 3.540M (−20.9%) | PASS | -| **P5.6 hand-rolled envelope decode** | **580.5m ±0% (−33.8%)** | **192.7 MiB (−50.5%)** | **2.615M (−41.6%)** | PASS | -| P5.7 slab-allocated baseline clone | 574.5m ±1% (−34.5%) | 194.4 MiB (−50.1%) | 2.508M (−44.0%) | PASS | -| P5.8 game-event eventid wire-peek | 572.3m ±1% (−34.7%) | 193.8 MiB (−50.2%) | 2.492M (−44.3%) | PASS | -| P5.9 prefix cache | _skipped (measured flat vs verified-clean baseline)_ | — | — | — | -| P5.10 inline vec2/vec3 cell | _rejected (allocs −17.9% but B/op +12.0%, sec +1.9%)_ | — | — | — | -| **End of Phase 5 (verified-clean re-measure)** | **573.9m ±1%** | **193.8 MiB** | **2.492M** | PASS | - -**Phase 5 totals.** vs the P5.0 nominal baseline: sec −34.5%, B/op −50.2%, allocs −44.3%. -The P5.0 sec sample was thermally inflated (±8%); against the credible pre-phase level of -~750–756m (P4's cooled measure / the P5.1 run), the honest sec/op win is **~−24%**. The -B/op and allocs columns are ±0% deterministic throughout. Cumulative vs the original P0 -baseline (pre-Phase-1: 1.523 s, 791.5 MiB, 20.75M allocs): **sec −62%, B/op −75%, -allocs −88%**. - -**Remaining targets (final profile, post-P5.8).** Alloc objects: qangle `[]float32` boxes -now #1 at 20.8% (only fixable by widening the cell — measured a net loss in P5.10 — or by -an API change); residual protobuf unmarshal ~25% (messages users register for: game-event -keys `consumeStringPtr`/`reflect.New`, user-message types — shrinkable only by extending -the fast-envelope set); `readBitsAsBytes` 7.7% (irreducible: one alloc per retained -string-table value); `newFieldState` in `set` 4.6%; `cloneInto` slabs 3.9%. CPU (in-memory -bench): `readBits` ~10%, `fieldState.set` ~4.5%, `readFieldPaths` ~10% cum — the bit-reader -and field-path machinery are now the floor. Parked ideas unchanged from earlier phases: -string-table Items map→dense slice, CDemoStringTables reconcile, COW baseline clone -(slab clone reduced the pressure; COW would also cut the copy bytes). - -\* the P5.0 sec/op baseline was thermally inflated (±8%); treat allocs/B as the -reliable P5.1 signal and 756m ±1% as the true current sec/op level. - -### P5.10 — inline vec2/vec3 cell — **REJECTED (measured)** -- **Attempted:** widen `cell` from 24 to 32 bytes (two extra float lanes) so QAngle/Vector/ - Vector2D/normal values store inline instead of boxing a `[]float32` per write, boxing - lazily on `Get` (fresh slice per read). -- **Measured:** allocs −17.9% (−447K) — but B/op **+12.0%** (+23 MiB; the wider cell - inflates every state array and clone slab) and sec/op **+1.87% slower** (p=0.000): the - extra copy/clone bytes outweigh the GC win, consistent with P3.1's 32-byte-cell attempt - (+12.9% B/op). Reverted; the vector residual (now ~21% of remaining allocs) stays the - known cost of the no-API-change constraint. - -### P5.9 — prefix cache for `set`/decoder walk — **SKIPPED** -- **Attempted:** cache the resolved depth-1 sub-state across consecutive sorted field paths - sharing `path[0]` (`setDepth`/`subAt` split), skipping the depth-0 walk step per field. -- **Measured flat:** sec/op ~ (p=0.481 vs a verified-clean P5.8 baseline), allocs/B - identical. Most field paths in this workload are depth-1 (`last == 0`), where the cache - does nothing, and the depth-0 walk step it skips is already just a bounds+kind check. - Reverted per the keep-only-if-confirmed rule. - -### P5.8 — game-event eventid wire-peek -- **Was:** every `CMsgSource1LegacyGameEvent` was fully unmarshaled (message + a - `reflect.New` per key via `consumeMessageSliceInfo`) before the internal handler checked - whether any handler is registered for the event name and usually returned nil. -- **Change:** `skipGameEvent` peeks the eventid varint (field 2) with protowire; a known - event with no registered handlers skips the unmarshal entirely. Unknown ids and absent - fields fall back to the full path, preserving its error behaviour exactly. -- **Result:** allocs/op −0.63%, B/op −0.32%, sec ~ (p=0.052). Modest on this bench because - it registers a combat-log handler and combat-log events dominate the event stream; the - skip is worth more to consumers that register fewer (or no) game-event handlers. ✅ - -### P5.7 — slab-allocated baseline clone -- **Was:** `clone()` allocated a `*fieldState` plus a `[]cell` array per nested state on - every entity create (210K objects, 19% of alloc space before P5.6). -- **Change:** a counting pre-pass sizes two slabs (`[]fieldState`, `[]cell`) and the copy - carves from them; vector/blob leaves keep their individual deep copies (P4.1 semantics). - Cell slices are cap-limited so a later `set` grow allocates fresh instead of stomping a - neighbour's arena region; each entity's slabs are private and die with it. -- **Result:** allocs/op 2.615M→2.508M (**−4.10%, −107K**), sec −1.03% (p=0.001), B/op - +0.92% (slab retention overhead — a good trade for object count). go test green. ✅ - -### P5.6 — hand-rolled envelope decode for hot internal messages ⭐ headline win -- **Was:** every CDemoPacket / CSVCMsg_PacketEntities / CSVCMsg_UpdateStringTable / - CNETMsg_Tick went through the reflective protobuf unmarshal: `reflect.New` per message, - a pointer alloc per scalar field, and — 32.7% of all allocated bytes — a fresh copy of - every `bytes` payload. The replay's data was effectively copied twice through the proto - layer (outer `CDemoPacket.data`, then inner `entity_data`/`string_data`). -- **Change:** `envelope_fast.go` decodes these four envelopes by hand with `protowire`, - aliasing the payload instead of copying, and calls new scalar-arg core methods - (`processDemoPacket`/`processPacketEntities`/`processUpdateStringTable`) split out of the - internal handlers. **Gating:** NewParser registers exactly one internal handler per list - before returning, so `len(list) == 1` means no user callbacks; any user registration - reverts that type to the full protobuf path (user-visible messages own their copies, as - before). Aliasing lifetimes: `entity_data`/`string_data` alias the packet arena (stable - through the dispatch loop, consumed synchronously — string-table values are copied out by - `readBitsAsBytes`); `CDemoPacket.data` aliases the outer-message buffer (stable until the - next `readOuterMessage`; `processDemoPacket` copies bodies into the arena before dispatch). -- **Result:** B/op 334.5→192.7 MiB (**−42.39%**), allocs/op 3.540M→2.615M (**−26.13%, - −925K**), sec/op 618.8m→580.5m (**−6.18%**, p=0.000). go test green — the goldens - exercise the fast path (tests register OnEntity/OnGameEvent, not raw message callbacks). ✅ - -### P5.5 — reader smalls: `readLeUintX` via accumulator, `readString` prealloc -- **Was:** unaligned `readLeUint32/64` allocated through the `readBytes` slow path (24K - objects via `fixed64Decoder` etc.); `readString` grew a cap-0 buffer byte by byte. -- **Change:** unaligned `readLeUint32` reads `readBits(32)` straight from the accumulator - (bit-stream LSB-first == LE byte decode); `readLeUint64` uses two accumulator words - (readBits is capped at 32). Aligned paths keep the zero-copy `readBytes` fast path and - the exact `pos` contract the reader tests assert. `readString` starts at cap 32 (stack- - allocatable when it doesn't escape). -- **Result:** allocs/op 3.597M→3.540M (**−1.59%, −57K**), B/op −0.16%, sec ~ (p=0.481). - go test green. ✅ - -### P5.4 — string-table item slab + prealloc + ring history + single lookup -- **Was:** `parseStringTable` allocated a `&stringTableItem` per item plus cap-0 `items` - append growth (217K objects); the 32-entry key history shifted the whole window with a - `copy` per item once full; the `UpdateStringTable` apply loop did up to four map lookups - per item. -- **Change:** preallocate `items` and back it with a single `[]stringTableItem` slab - (bounded at 4096 cap so corrupt `numUpdates` can't OOM; pointer identity survives slab - regrowth since items are only touched through the taken pointers). Key history becomes a - fixed ring buffer (`histCount`-based indexing reproduces the shift-window semantics - exactly). Apply loop hoists to one lookup. -- **Result:** allocs/op 3.757M→3.597M (**−4.26%, −160K**), B/op −0.38%, sec −0.98% - (p=0.043). go test green (string-table goldens + full corpus). ✅ - -### P5.3 — `p.entities` map → dense slice -- **Was:** `p.entities` was a `map[int32]*Entity`; the PacketEntities update loop does 1–3 - lookups per entity update (~9% CPU in `mapaccess1_fast32` + inlined key probes). Deletion - stored `nil` *into* the map, so `FilterEntity` could pass nil entities to user callbacks — - a latent crash. -- **Change:** dense `[]*Entity` sized `1<