From 51cb552715653a0030e27fec5f46ac5de2195092 Mon Sep 17 00:00:00 2001 From: "glm-5.2" Date: Mon, 17 Aug 2026 14:18:23 +0000 Subject: [PATCH] Add review #004: read-path performance review Traces the ~400x read-path gap (alktty wire_vs_bast bench) to SequentialReader::read_field_at re-parsing BastDoc::new on every field read, with the self-referential lifetime constraint as the root cause. Findings: - H1: per-field BastDoc::new re-parse in sequential_reader.rs:262 - M1: same re-parse in four one-shot paths (LayoutBuilder::build, validate_bytes, engine read_field/write_field) - L1: dead _field_schema param + Value clones in SequentialReader - L2: ADR-007 Cost section + engine doc comment understate the re-parse - N1: carry-forward of review #003 N2 (no new action) Lays out fix options: Option A (owned typed tree, recommended, closes H1+M1, breaking), Option B (read-plan precompute, fallback, H1 only, non-breaking), Option C (borrow-from-engine, rejected, contradicts ADR-007). Verification: docs-only review; alktype source unchanged. cab4932 --- docs/reviews/004-performance-review.md | 412 +++++++++++++++++++++++++ 1 file changed, 412 insertions(+) create mode 100644 docs/reviews/004-performance-review.md diff --git a/docs/reviews/004-performance-review.md b/docs/reviews/004-performance-review.md new file mode 100644 index 0000000..c898ecc --- /dev/null +++ b/docs/reviews/004-performance-review.md @@ -0,0 +1,412 @@ +--- +status: open +last_updated: 2026-08-17 +reviewed_artifacts: + - src/sequential_reader.rs + - src/bast.rs + - src/engine.rs + - src/layout_builder.rs + - src/materialize.rs + - src/offset_map.rs + - src/data_access.rs + - src/lib.rs + - docs/architecture/decisions/007-packed-mode-read-factory.md + - ../@alkdev/alktty/benches/wire_vs_bast.rs +tool: manual source read + downstream criterion bench (`cargo bench --bench wire_vs_bast` in alktty) +reviewer: read-path performance review (triggered by alktty wire_vs_bast bench) +--- + +# Review #004 — Read-Path Performance: the `BastDoc` Re-Parse Gap + +## Purpose + +A downstream bench in `alktty` (`benches/wire_vs_bast.rs`) compared a +hand-rolled `ChunkHeader` codec against an alktype-driven codec built +from the same 2-field BAST struct (`stream_type: uint8`, +`length: uint32`, big-endian). The bench builds the engine / layout / +reader **once** outside the measured loop, then measures per-chunk read +and write over 1024 contiguous chunks. + +The write path is competitive (~2.8x at 64 B, ~1.1x at 4 KiB — the +per-chunk overhead is just `data_access::write_u8`/`write_u32` at +precomputed `PackedLayout` offsets, and the payload copy dominates at +4 KiB). The read path is **~400x slower per chunk** (2.27 µs/chunk vs +5.6 ns/chunk hand-rolled), and the cost is fixed — it dominates at 64 B +*and* at 4 KiB. + +This review traces the gap to its source, confirms it is an +implementation gap (not inherent to the design), and lays out the fix +options. The bench itself is honest — its in-file note +(`wire_vs_bast.rs:36-41`) already points at the root cause; this review +formalizes the finding and the remediation plan. + +## Methodology + +- Full read of the read-path code (`sequential_reader.rs`, `bast.rs`, + `engine.rs`), the write path (`layout_builder.rs`, `data_access.rs`), + and ADR-007 (the packed read factory decision). +- Cross-reference every `BastDoc::new` call site in `src/` to map the + full re-parse surface. +- Trace the lifetime/ownership constraint that forces the re-parse + (`BastDoc<'a>` borrows `&'a Value`; `SequentialReader` owns its + `Value` — self-referential struct, cannot cache the parsed tree). +- Read the downstream bench to confirm the measurement is honest (the + re-parse is inside the measured routine; the engine/reader are built + once outside it). +- Read `docs/reviews/003-code-review.md` for prior context — review #003 + did not flag the re-parse (it was a correctness/coverage pass, not a + performance pass). + +## Verification Baseline + +The bench numbers below are from the alktty downstream tree +(`benches/wire_vs_bast.rs`, criterion 0.7), run against alktype at +commit `cab4932` (v0.2.0). The alktype tree itself is unchanged — this +is a review of existing code, not a fix. + +| path | payload=64 B | payload=4 KiB | +|---|---|---| +| hand-rolled read | 5.7 µs (5.6 ns/chunk) | 12.1 µs (11.8 ns/chunk) | +| alktype read | 2.32 ms (2.27 µs/chunk) | 2.38 ms (2.33 µs/chunk) | +| hand-rolled write | 14.3 µs | 321 µs | +| alktype write | 39.8 µs | 355 µs | + +One-shot startup costs (paid once): `AlkTypeEngine::compile` 573 µs, +`engine.sequential_reader()` 2.84 µs, `LayoutBuilder::build` 1.30 µs. + +The read gap is ~400x per chunk and is **fixed** (does not shrink as +the payload grows), which is the signature of per-field overhead, not +payload-copy overhead. + +## Summary Statistics + +| Severity | Count | +|----------|------:| +| High | 1 (H1) | +| Medium | 1 (M1) | +| Low | 2 (L1, L2) | +| Nit | 1 (N1) | + +The High finding is the read-path re-parse — a severe performance +regression that blocks the primary intended use case (alktype as a +runtime codec for protocol wire formats). It is not a correctness bug; +the re-parse produces the same result every time, it is just +catastrophically wasteful. The Medium finding is the same root cause +manifesting in four one-shot paths. The Low/Nit findings are dead code +and a stale doc comment exposed while tracing the root cause. + +--- + +## Findings + +### H1. `SequentialReader::read_field_at` re-parses the BAST typed tree on every field read + +**File**: `src/sequential_reader.rs:262` + +**Problem**: `read_field_at` calls `BastDoc::new(&self.doc_value, +&self.root_name)?` on every field read. `BastDoc::new` +(`src/bast.rs:69`) recursively parses the root def: `lookup_def_raw` +(hash lookup into the `serde_json` Map), `BastDef::parse` → +`BastStruct::parse` → allocates a `Vec`, iterates fields, +`BastField::parse` each (several `node.get().as_str()`/`as_u64()` +calls + a `format!` allocation for the field path), `BastType::parse` +each. For the 2-field `ChunkHeader`, that is ~1 µs per call. + +`read_next` calls `read_field_at` once per field, so a 2-field header +incurs **two** `BastDoc::new` calls per chunk = ~2.27 µs/chunk, matching +the bench. For an N-field struct, reading all fields is O(N²) in parse +work (each of N reads re-parses all N fields) — the gap widens with +struct width. + +The re-parse is purely redundant: `SequentialReader::new` +(`src/sequential_reader.rs:130`) already parsed the `BastDoc` once at +construction and extracted the field list. `read_field_at` re-derives +the same `field_node` (the `BastField` at `index`) and the same `doc` +that were already in hand at construction time. + +**Root cause**: a lifetime/ownership tension, not a logic error. +`BastDoc<'a>` borrows `&'a Value` and `&'a str` throughout +(`src/bast.rs:51-55`). `SequentialReader` **owns** a cloned `Value` +(`doc_value`, `src/sequential_reader.rs:111`). To cache the parsed +`BastDoc` in the reader, the `BastDoc` would have to borrow from +`self.doc_value` — a self-referential struct, which Rust's borrow +checker forbids and safe Rust cannot express without a self-referential +crate (`self_cell`/`ouroboros`, both rejected: new dep per AGENTS.md +§7, and `self_cell`'s soundness relies on `unsafe` the crate avoids per +AGENTS.md §11). So the only way to get a `BastDoc` at read time is to +re-parse from the owned `Value`. The engine's own doc comment +(`src/engine.rs:112-115`) acknowledges this design explicitly: + +> "The engine retains a clone of the BAST `Value` so that +> `sequential_reader` and `read_field` can re-parse the typed tree on +> demand without lifetime entanglement with the caller's `Value`." + +The "re-parse on demand" framing was a lifetime-entanglement workaround +that did not anticipate the hot-loop cost. ADR-007's "Cost" section +(`decisions/007-packed-mode-read-factory.md:64-70`) argues construction +is cheap ("a `Vec<(String, Value)>` of the `properties` entries ... a +struct has a small number of fields") — true for *construction* (once), +but the decision did not account for a per-field re-parse inside the +read loop. + +**Why the write path is fine**: `LayoutBuilder::build` +(`src/layout_builder.rs:190`) also re-parses `BastDoc::new`, but it is +called **once** per write — the resulting `PackedLayout` offsets are +cached and reused. The write loop then does only `data_access::write_*` +at fixed offsets. There is no per-field re-parse on the write hot path, +which is why write is ~1.1x at 4 KiB. + +**Fix**: cache the parsed typed tree so the read path does not re-parse. +See "Fix Options" below for the three approaches and the +recommendation. + +**Lift**: closes a ~400x read-path gap and unblocks alktype as a runtime +codec for protocol wire formats (its stated purpose per ADR-001). This +is the difference between "plausible runtime codec" and "not viable." +Large effort depending on the chosen option. + +--- + +### M1. The same re-parse pattern exists in four one-shot paths + +**Files**: `src/layout_builder.rs:190`, `src/engine.rs:284,334,467` + +**Problem**: `BastDoc::new(&self.doc_value, &self.root_name)?` / +`BastDoc::new(&self.bast_doc, &self.root_name)?` is re-called in: + +- `LayoutBuilder::build` (`src/layout_builder.rs:190`) — once per + `build()` call. Re-parsing on each build is wasteful if a builder is + reused across writes, but the typical pattern is build-once-reuse, + so this is mild. +- `AlkTypeEngine::validate_bytes` (`src/engine.rs:284`) — once per + validate call. For a stream of buffers, this is a per-buffer re-parse. +- `AlkTypeEngine::read_field` (`src/engine.rs:334`, aligned mode) — once + per field read. Same class as H1 but for aligned random access, and + one parse per field read (not the O(N²) of the sequential reader). +- `AlkTypeEngine::write_field` (`src/engine.rs:467`, aligned mode) — + once per field write. + +These are less acute than H1 (one parse per operation, not per-field- +in-a-loop), but they share the same root cause: the engine/builder own +a `Value` and cannot cache a borrowing `BastDoc`. Any fix that makes +`BastDoc` cacheable on an owning struct (Fix Option A) closes these for +free; a read-path-only fix (Option B) leaves them as-is, which is +acceptable since they are not hot loops. + +**Lift**: removes redundant parse work on the validate/aligned paths. +Free with Option A; deferred with Option B. + +--- + +### L1. `read_field_value` carries a dead `_field_schema` parameter; `fields` stores dead `Value` clones + +**Files**: `src/sequential_reader.rs:114,294` + +**Problem**: `SequentialReader` stores `fields: Vec<(String, Value)>` +where the `Value` is `f.source().clone()` per field +(`src/sequential_reader.rs:145`). `read_field_at` passes this as +`_field_schema` to `read_field_value` (`src/sequential_reader.rs:294`), +where it is unused (prefixed `_`). The raw `Value` clone per field is +dead weight — only the `String` name is used (for `read_next`'s return +and `read_field`'s lookup). This is a minor allocation cost on top of +H1's re-parse, and it will be removed naturally when the read plan is +precomputed (Option B) or the `BastDoc` is cached (Option A), since +both replace `Vec<(String, Value)>` with typed/owned field data. + +**Lift**: trivial; falls out of the H1 fix. + +--- + +### L2. ADR-007 "Cost" section and the engine doc comment understate the re-parse + +**Files**: `docs/architecture/decisions/007-packed-mode-read-factory.md:64-70`, +`src/engine.rs:112-115` + +**Problem**: ADR-007's "Cost" section argues `sequential_reader()` is +cheap because it clones a small `Vec` of field schemas. That is true for +the factory call (once). But the decision did not anticipate that +`read_field_at` would re-parse `BastDoc::new` per field — the cost that +actually dominates. The engine doc comment at `src/engine.rs:112-115` +explicitly frames re-parse-on-demand as the intended design ("re-parse +the typed tree on demand without lifetime entanglement"), which is the +root cause H1 traces. + +**Fix**: whichever fix option is chosen, update ADR-007's "Cost" / +"Consequences" section and the engine doc comment to reflect that the +parsed tree is now cached (Option A) or precomputed into a read plan +(Option B), and that the "re-parse on demand" framing is retired. + +**Lift**: documentation accuracy; prevents the same framing from +misleading a future edit. + +--- + +### N1. `BastType::alk_kind()` returns `Struct` for any `$ref` (carry-forward from review #003 N2) + +**File**: `src/bast.rs:715-725` + +**Problem**: flagged in review #003 N2 and left as "defer unless it +bites." It does not bite here — `read_field_value` always calls +`doc.resolve_typeref(ty)` before matching on `BastType`, so the +misreporting `alk_kind` is never consulted on a `Ref`. Noting it only +because this review re-read the same path; no new action beyond review +#003's deferral. + +--- + +## Fix Options + +The core constraint: `BastDoc<'a>` borrows `&'a Value` / `&'a str`; an +owning struct (`SequentialReader`, `AlkTypeEngine`, `LayoutBuilder`) +cannot store a `BastDoc` that borrows from its own `Value` field +(self-referential). Three ways to break the constraint: + +### Option A — Make the typed tree own its data (principled fix) + +Change `BastDoc<'a>` → `BastDoc` (no lifetime), `&'a str` → `Arc` +(or `String`), `&'a Value` → `Arc` (or `Value`). Then +`SequentialReader`, `AlkTypeEngine`, and `LayoutBuilder` each hold a +`BastDoc` directly (built once at construction), and `read_field_at` +uses `&self.doc` — no re-parse, anywhere. + +- **Closes**: H1, M1 (all four one-shot paths), and the engine/reader + lifetime entanglement that ADR-007 worked around. The engine's + `bast_doc: Value` clone (`src/engine.rs:85`) becomes redundant with + the owned `BastDoc`. +- **Tradeoff**: broad refactor. Touches `bast.rs` (every typed node) + and every consumer (`layout_builder`, `offset_map`, `materialize`, + `sequential_reader`, `tunion`, `bast_validation`, `engine`). The + `Bast*` types are re-exported in `lib.rs:57-60`, so this is a + **breaking public-API change** — `BastDoc<'a>` becomes `BastDoc`, + and every method signature that took `&'a` changes. Per AGENTS.md, + this is semver-relevant and would need a version bump (0.2.0 → 0.3.0). +- **Dependency cost**: `Arc`/`Arc` add `alloc` (already in + use via `Vec`/`String`); no new external deps. Stays wasm-clean. The + `preserve_order` serde_json feature remains load-bearing (AGENTS.md + §8) — owning the `Value` does not change field-order semantics. +- **Effort**: large but mechanical. The borrow-based design was chosen + for "allocation-free beyond the small typed nodes" (`src/bast.rs:11- + 18`), but the re-parse-per-field already defeats that goal by + allocating a fresh `Vec` per read. Owning the data makes + the "parse once, walk many times" invariant actually hold. + +### Option B — Precompute an owned read plan in `SequentialReader::new` (surgical fix) + +Keep `BastDoc<'a>` borrowing for the other consumers. In +`SequentialReader::new`, parse the `BastDoc` once, resolve all `$ref`s +eagerly, and build a flat, owned tree of read instructions +(`Vec`) that the read loop walks with no `BastDoc` +involvement. Each `FieldPlan` carries the field name, the resolved +`AlkTypeKind`, the effective `Endian`, and for composites a nested +plan (struct → sub-plans; union → discriminator + per-variant plans; +array → element plan + count; record → value plan). + +- **Closes**: H1 only. M1 (the one-shot re-parses) remains, which is + acceptable since they are not hot loops. +- **Tradeoff**: non-breaking (internal to `sequential_reader.rs`; the + public `SequentialReader` type and its methods keep their + signatures). Duplicates some of the `BastType` matching logic that + `read_field_value`/`read_union_value`/etc. already encode, so there + are two parallel walkers to maintain. +- **Effort**: medium. Self-contained in one file but non-trivial + (composites require recursively resolving and pre-flattening the + type tree, including `$ref` chains into `$defs`). + +### Option C — Cache `BastDoc` on the engine, reader borrows (rejected) + +Have the engine own the parsed `BastDoc` and return a +`SequentialReader<'_>` that borrows from `&self`. This requires +`BastDoc` to be owned (Option A prerequisite) *and* changes +`sequential_reader() -> Option` to +`-> Option>` — a breaking public-API change that +also contradicts ADR-007's "owned fresh reader" decision. Strictly +worse than Option A (same refactor cost, more API churn, contradicts +an ADR). Rejected. + +### Recommendation + +**Option A**, given the publisher's stated willingness to make breaking +changes ("no one is using this except us yet; ... we can change things +now"). It is the only option that closes H1 *and* M1 and retires the +lifetime-entanglement workaround that caused both. The refactor is +broad but mechanical (lifetime removal, not logic rewrites), and the +crate is pre-1.0 with only two in-house downstream consumers +(`alktty`, `alkcall`), so the breakage cost is bounded and known. + +Option B is the fallback if the Option A refactor is deferred — it +closes the acute H1 gap non-breakingly while leaving M1 for later. It +is not the recommended path because it leaves a second parallel type +walker in the crate and does not address the root cause (the borrow- +based `BastDoc` design), which will keep forcing re-parses anywhere a +new owning consumer wants to cache the parsed tree. + +Regardless of the chosen option, ADR-007's "Cost"/"Consequences" +section and the `src/engine.rs:112-115` doc comment should be updated to +retire the "re-parse on demand" framing (L2). + +--- + +## What's Good + +- **The bench is honest.** The alktty bench builds the engine/reader + once outside the measured loop and correctly isolates the per-chunk + logic. Its in-file note (`wire_vs_bast.rs:36-41`) already points at + the `read_field_at` re-parse and labels it "the honest current cost of + the alktype read path, not a bench bug." This review confirms that + assessment. +- **The write path is already competitive.** Once `PackedLayout` is + built, the write loop is just `data_access::write_*` at fixed offsets + — no schema walk, no re-parse. This validates the "build once, reuse" + pattern that the read path should also adopt. +- **`resolve_typeref` for primitives is cheap.** `BastType::Primitive` + is `Copy` (`AlkTypeKind: Copy`, `src/schema.rs:25`), so + `resolve_typeref` (`src/bast.rs:117-125`) returns `other.clone()` + without allocation for the common case. The re-parse cost is entirely + in `BastDoc::new`, not in the per-field type resolution — so caching + the `BastDoc` alone closes the gap without restructuring + `resolve_typeref`. +- **Overflow safety and error attribution are unaffected.** The + `checked_add` / `usize::try_from` discipline (AGENTS.md §4) and the + `field_path`-carrying errors (review #003 "What's Good") are in the + read functions, not the parser — a caching fix preserves them. + +--- + +## Recommended Order + +1. **H1 + M1 (Option A)** — the owned-typed-tree refactor. Decide + first (this is a one-way door: breaking public-API change, version + bump to 0.3.0). If approved, this is one refactor that closes both. +2. **L2** — update ADR-007 and the engine doc comment in the same + commit as the H1 fix, since the "re-parse on demand" framing is + being retired. +3. **L1** — falls out of the H1 fix (the dead `Value` clones are + replaced by the cached/owned field data). +4. **N1** — remains deferred per review #003. + +If Option A is deferred, **H1 (Option B)** is the standalone +alternative — non-breaking, closes the acute gap only. + +--- + +## Notes + +- All line numbers refer to the tree at commit `cab4932` (v0.2.0, the + BAST pivot release). +- The bench is in the `alktty` downstream repo + (`/workspace/@alkdev/alktty/benches/wire_vs_bast.rs`), not in alktype. + alktty depends on alktype as a path dev-dep for the bench only; it + does not use alktype at runtime. The path dep means + `cargo publish --dry-run` for alktty would complain (the bench is + exploratory and uncommitted in alktty; the alktype crate itself has + no bench dependency). +- This review does not cover the wasm build (`cargo build --target + wasm32-unknown-unknown`) because the fix is not yet implemented; the + verification block for the fix should include it per AGENTS.md, as + the typed-tree ownership change touches `bast.rs` which is + wasm-relevant. +- Review #003 (post-BAST-pivot correctness review) did not flag the + re-parse — its scope was correctness, coverage, and panic safety, not + performance. The re-parse is not a correctness regression; the parsed + tree is identical across calls. This review complements #003 by + adding the performance axis. \ No newline at end of file