diff --git a/docs/architecture/README.md b/docs/architecture/README.md index ad24fae..3d5dc0e 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -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 diff --git a/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md b/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md index bacdb98..30d912b 100644 --- a/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md +++ b/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md @@ -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`. `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` 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` \ No newline at end of file + `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). \ No newline at end of file