Accept ADR-011: compiled read plan for packed mode
Tighten framing per pre-acceptance review (no decision changes): - Be precise about M1 coverage: closes the packed-side site (engine.rs:284 validate_bytes); the aligned-side M1 sites (engine.rs:334,467, layout_builder.rs:190) are a deliberate reversible bet, not a non-issue. - Replace the 'two type walkers is symmetric with OffsetMap/ PackedLayout' spin with an honest 'parallel typed tree, permanent maintenance tax, justified by ~20-50x composite-dispatch win on SFTP-shaped union-with-$ref-variants packets.' - Move the 'determinism enables fingerprinting/cache/handshake' future work out of the positives list into a dedicated 'Future capabilities' section — it is a forward reference, not a current win. - Clarify bast_doc: Value is retained unconditionally (unused in packed mode, still needed in aligned mode); add a Send+Sync test note for Arc<ReadPlan> shared from the Send+Sync engine. - Tighten the 'no format! allocations' claim to 'no resolve_typeref, no BastDef::parse, no JSON node access on the happy path' (error- path format! remains, and is not the cost being removed). - Add a 'POC coverage' section recording that the readplan-poc branch walked every BastType arm and confirmed ReadPlan covers all cases, including the union Byte/Field split and the array variable-stride (element_stride = 0) case. Status flipped Proposed -> Accepted. README ADR table updated. Verification: docs-only change; cargo test --release, cargo clippy --all-targets -- -D warnings, cargo doc --no-deps unchanged (no source touched).
This commit is contained in:
glm-5.2 committed
1 parent
3184818c08
commit
1037e68091
2 files changed
+91
-30
No files matched your search
@@ -45,7 +45,7 @@ format definition; the engine is generic.
|
||||
| [008](decisions/008-reject-tunion-in-aligned-mode.md) | Reject TUnion in Aligned Mode for v1 | Unions are the protocol pattern; aligned-mode union semantics were broken |
|
||||
| [009](decisions/009-builder-api.md) | Builder API for Schema Construction | Fluent Rust API producing `serde_json::Value`; covers BAST kinds + standard JSON Schema; resolves OQ-003. *Output format amended to BAST / standard JSON Schema by ADR-BAST.* |
|
||||
| [010](decisions/010-generalized-validation-validate-bytes.md) | Generalized Validation — `validate_bytes` on `AlkTypeEngine` | Single-call binary-buffer validation; materialize `Value` from bytes, then validate. *Validation step amended to the BAST-native validator by ADR-VAL-SPLIT.* |
|
||||
| [011](decisions/011-compiled-read-plan-for-packed-mode.md) | Compiled Read Plan for Packed Mode | `ReadPlan` — the packed read-side compiled form, symmetric to `OffsetMap` (aligned) and `PackedLayout` (packed write). Closes review #004's 400x read-path gap; retires ADR-007's "re-parse on demand" framing. *Proposed.* |
|
||||
| [011](decisions/011-compiled-read-plan-for-packed-mode.md) | Compiled Read Plan for Packed Mode | `ReadPlan` — the packed read-side compiled form, symmetric to `OffsetMap` (aligned) and `PackedLayout` (packed write). Closes review #004's 400x read-path gap; retires ADR-007's "re-parse on demand" framing. *Accepted.* |
|
||||
|
||||
## Relevant Open Questions
|
||||
|
||||
|
||||
@@ -2,8 +2,11 @@
|
||||
|
||||
## Status
|
||||
|
||||
Proposed — for review. Closes review #004 H1 + M1 + L1 + L2;
|
||||
retires the "re-parse on demand" framing from ADR-007.
|
||||
Accepted. Closes review #004 H1 + M1 (packed side) + L1 + L2;
|
||||
retires the "re-parse on demand" framing from ADR-007. A derisking
|
||||
POC on branch `readplan-poc` confirmed the `ReadPlan` shape covers
|
||||
every `BastType` arm in the current read loop before implementation
|
||||
began (see "POC coverage" at the end).
|
||||
|
||||
## Context
|
||||
|
||||
@@ -108,8 +111,10 @@ A pre-resolved tree of read instructions. Every `$ref` is resolved, every
|
||||
endianness is computed (field override or container default), every
|
||||
union variant is inlined. The read loop indexes into a `Vec`, matches
|
||||
on a `ReadKind`, and calls `data_access::read_*` with a precomputed
|
||||
`Endian` — no `resolve_typeref`, no JSON node access, no `format!`
|
||||
allocations.
|
||||
`Endian` — no `resolve_typeref`, no `BastDef::parse`, no JSON node
|
||||
access on the happy path. (`format!` for error-path attribution may
|
||||
still occur on the error path; it does not run on the happy path and
|
||||
is not the cost being removed here.)
|
||||
|
||||
```rust
|
||||
pub struct ReadPlan {
|
||||
@@ -187,10 +192,18 @@ as `AlkTypeError::Schema` — the same untrusted-input discipline
|
||||
stores `Arc<ReadPlan>`. `sequential_reader()` returns
|
||||
`SequentialReader { plan: Arc::clone(&self.plan), .. }` — an owned
|
||||
reader (ADR-007's factory decision is retained; the reader owns its
|
||||
cursor, shares the immutable plan). The `bast_doc: Value` clone
|
||||
(`src/engine.rs:85`) is no longer needed on the packed read path; the
|
||||
engine retains it only for `validate_bytes`'s materialize-then-validate
|
||||
flow if the plan doesn't subsume that path (see Scope below).
|
||||
cursor, shares the immutable plan).
|
||||
|
||||
`bast_doc: Value` (`src/engine.rs:85`) is retained on the engine
|
||||
unconditionally. In packed mode it becomes unused by the read path
|
||||
(both `sequential_reader` and `validate_bytes` consume the plan);
|
||||
in aligned mode it is still needed for `read_field`/`write_field`/
|
||||
`validate_bytes`. Keeping it always avoids a mode-conditional field
|
||||
and costs only a `Value` clone paid once at `compile`. `ReadPlan`
|
||||
must be `Send + Sync` so `Arc<ReadPlan>` can be shared from the
|
||||
`Send + Sync` engine (ADR-007); this falls out naturally from the
|
||||
plan being immutable owned data, but the implementation should add a
|
||||
`static` bound assertion test to lock it in.
|
||||
|
||||
### Public API change (breaking — version bump to 0.3.0)
|
||||
|
||||
@@ -266,9 +279,13 @@ The crate is pre-1.0 with two in-house downstream consumers
|
||||
precomputed `Endian`." The expected per-chunk cost is in the
|
||||
hand-rolled codec's ballpark (the `data_access` calls are the same
|
||||
ones the hand-rolled codec makes).
|
||||
- **Closes M1's packed-side re-parses for free.**
|
||||
`validate_bytes` (packed) and `materialize_packed` both consume the
|
||||
`ReadPlan` — no re-parse on either path.
|
||||
- **Closes M1's packed-side re-parse (`engine.rs:284`, `validate_bytes`).**
|
||||
The aligned-side M1 sites (`engine.rs:334,467`,
|
||||
`layout_builder.rs:190`) are deliberately left as-is — they are not
|
||||
hot for the packed-codec use case, and Option A remains available as
|
||||
an additive later fix if an aligned-mode hot loop ever emerges. This
|
||||
is a reversible bet, not a claim that the aligned-side M1 is a
|
||||
non-issue.
|
||||
- **Unifies the two packed read-side consumers on one compiled form.**
|
||||
`SequentialReader` and `materialize_packed` walk the same `ReadPlan`,
|
||||
mirroring how `OffsetMap` unifies the aligned read and write sides.
|
||||
@@ -286,14 +303,14 @@ The crate is pre-1.0 with two in-house downstream consumers
|
||||
(refcount bump). The plan is immutable and shared across all readers
|
||||
from one engine. ADR-007's "owned fresh reader" decision is retained;
|
||||
the reader owns its cursor, shares the plan.
|
||||
- **Determinism.** `ReadPlan::compile` is a pure function of the BAST
|
||||
document + root name — same input, same plan. This makes the
|
||||
"compiled form" visibly deterministic, opening a future capability:
|
||||
fingerprinting the plan (e.g., `#[derive]` a hash) for cross-run
|
||||
caching, disk-cached compiled plans, or schema-version handshakes
|
||||
for `alkcall`'s hub/spoke topology. Not implemented now, but the
|
||||
compiled form makes it possible; a re-parse-per-field design makes
|
||||
it invisible.
|
||||
- **Deterministic compile.** `ReadPlan::compile` is a pure function of
|
||||
the BAST document + root name — same input, same plan. This makes the
|
||||
"compiled form" visibly deterministic, which is a prerequisite for
|
||||
future capabilities (fingerprinting the plan for cross-run caching,
|
||||
disk-cached compiled plans, or schema-version handshakes for
|
||||
`alkcall`'s hub/spoke topology). Not implemented in this ADR and not
|
||||
needed to justify the decision; listed here only so a future ADR
|
||||
doesn't re-derive the prerequisite. See "Future capabilities" below.
|
||||
- **Retires the "re-parse on demand" framing (L2).** ADR-007's "Cost"
|
||||
section and `src/engine.rs:112-115`'s doc comment framed re-parse as
|
||||
the intended design. With the plan, the read path never re-parses;
|
||||
@@ -312,15 +329,18 @@ The crate is pre-1.0 with two in-house downstream consumers
|
||||
in-house downstream consumers, both updated with the bump. The
|
||||
breakage is smaller than review #004's Option A (no `Bast*` type
|
||||
changes — `BastDoc` stays borrowed, stays the validation-side tree).
|
||||
- **Two type walkers in the crate.** `BastDoc` (validation, aligned
|
||||
one-shot paths) and `ReadPlan` (packed read path) coexist. This is
|
||||
the same structural split as `OffsetMap`/`PackedLayout` (different
|
||||
compiled forms for different modes/sides), not a duplication: each
|
||||
compiled form serves its mode/side pair, and `BastDoc` serves the
|
||||
paths that don't have a compiled form yet (validation, aligned
|
||||
one-shot reads/writes). A future cleanup can give the aligned
|
||||
one-shot paths their own compiled form; it is additive, not
|
||||
blocking.
|
||||
- **A parallel typed tree, not a flat lookup table.** This is the
|
||||
honest cost. `OffsetMap` and `PackedLayout` are flat `(path, range)`/
|
||||
`(path, position)` projections of `BastType`; `ReadPlan` is a full
|
||||
parallel hierarchy (`CompositePlan` mirrors `BastType`'s
|
||||
Struct/Union/Array/Record). The maintenance tax is real and higher
|
||||
than those: when schema semantics change, `BastDoc`/`BastType` and
|
||||
`ReadPlan`/`CompositePlan` move together. It is worth it because the
|
||||
perf win on composite-heavy schemas (the SFTP-shaped union-with-`$ref`
|
||||
-variants packet) justifies it — see the next bullet. This is a
|
||||
permanent tax accepted in exchange for a ~20–50x composite-dispatch
|
||||
win on top of the 400x re-parse fix, not a structural symmetry with
|
||||
the flat compiled forms.
|
||||
- **Eager `$ref` resolution at compile time.** `ReadPlan::compile`
|
||||
resolves all `$ref`s eagerly, including union variant refs. This is
|
||||
correct (the schema is fixed at compile time) and matches the
|
||||
@@ -426,4 +446,45 @@ closes L2. L1 falls out at step 2. The aligned-side M1 paths
|
||||
access-time check
|
||||
- [ADR-010](010-generalized-validation-validate-bytes.md) —
|
||||
`validate_bytes` (packed) consumes the `ReadPlan` via
|
||||
`materialize_packed`
|
||||
`materialize_packed`
|
||||
|
||||
## Future capabilities (not part of this ADR)
|
||||
|
||||
The deterministic-compile property of `ReadPlan` is a prerequisite for
|
||||
several capabilities that are explicitly out of scope here but worth
|
||||
naming so a future ADR doesn't re-derive the prerequisite:
|
||||
|
||||
- Fingerprinting the plan (e.g. `#[derive(Hash)]`) for cross-run
|
||||
caching of compiled plans.
|
||||
- Disk-cached compiled plans (skip `compile` on warm start).
|
||||
- Schema-version handshakes for `alkcall`'s hub/spoke topology —
|
||||
peers exchange plan fingerprints instead of full BAST documents.
|
||||
- A `WritePlan` and/or `ValidationPlan` that follow the same
|
||||
compile-once-walk-many pattern for the deferred M1 sites
|
||||
(`engine.rs:334,467`, `layout_builder.rs:190`, `bast_validation`).
|
||||
|
||||
None of these justify this ADR; the 400x read-path gap does. They are
|
||||
listed only as forward references.
|
||||
|
||||
## POC coverage
|
||||
|
||||
Before this ADR was accepted, a derisking POC on branch `readplan-poc`
|
||||
walked the read loop against every `BastType` arm in
|
||||
`src/sequential_reader.rs:303-381` (and the parallel arms in
|
||||
`materialize.rs`) and confirmed the `ReadPlan`/`CompositePlan`/
|
||||
`ReadKind`/`DiscriminatorPlan` shape covers all cases, including the
|
||||
two spots where a plan arm could subtly miss a case:
|
||||
|
||||
- **Union discriminator split (`Byte` vs `Field`).** Both are covered:
|
||||
`DiscriminatorPlan::Byte { offset, disc_type }` and
|
||||
`DiscriminatorPlan::Field { name, field_index }`. The field-name case
|
||||
pre-resolves the discriminator field's `ReadKind` so dispatch reads
|
||||
it from the plan, not from a re-parsed `BastField`.
|
||||
- **Array variable-element-stride (`element_stride = 0`).** Covered:
|
||||
`CompositePlan::Array { element, count, element_stride }` preserves
|
||||
the `0`-signals-variable convention, and the read loop walks
|
||||
sequentially when `element_stride == 0` (matching today's
|
||||
`walk_variable_array_size`).
|
||||
|
||||
The POC also confirmed `ReadPlan: Send + Sync` holds for the planned
|
||||
shape (immutable owned data, no interior mutability, no lifetimes).
|
||||
Reference in new issue
Block a user