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
This commit is contained in:
glm-5.2 committed 2026-08-17 14:18:23 +00:00
1 parent cab493206c
commit 51cb552715
1 file changed
+412
+412
View File
@@ -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<BastField>`, 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<str>`
(or `String`), `&'a Value` → `Arc<Value>` (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<str>`/`Arc<Value>` 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<BastField>` 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<FieldPlan>`) 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<SequentialReader>` to
`-> Option<SequentialReader<'_>>` — 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.