Skip to content

docs(arch): ARCH-01..15 — review issues on the architecture baseline - #140

Merged
orionpapadakis merged 2 commits into
refactor/framework-abstractionsfrom
review/arch-issues
Jul 31, 2026
Merged

docs(arch): ARCH-01..15 — review issues on the architecture baseline#140
orionpapadakis merged 2 commits into
refactor/framework-abstractionsfrom
review/arch-issues

Conversation

@mikepapadim

Copy link
Copy Markdown
Member

Review of the architecture baseline (7ee6f86) as 15 ARCH issues, one topic each, in
docs/architecture/review/ARCH-issues-mikepapadim.md. Branched off the baseline commit, so the diff is
the review file only.

Nothing here is a change to the architecture documents. Each issue is Status: Open until we go through
them together and mark it Accept / Reject / Postpone; only accepted ones should be folded into
docs/architecture/.

Framing

The baseline is strong on the library axis, and better specified than what I had written separately:
the 15 dependency rules with ArchUnit forms and a shrink-only allowlist, ADR-001 (four lifetimes),
ADR-002 (program vs compiled program), the terminology doc, and the P1–P10 pressure points — P5, P6, P7,
P9 and P10 match what I found independently, and P9's count of 26 files importing
uk.ac.manchester.tornado.* outside tornadovm/** is the concrete allowlist seed.

Most of my issues are on the product axis: the serving path (continuous batching, paged KV, prefix
cache), quantization, and the numbers that decide whether a phase regressed anything. Measured across
all 13 documents: 0 occurrences of continuous batching, PagedAttention, prefix cache, KV cache manager,
scheduler, server/OpenAI; 0 of Q4/Q8_0/FP4/MXFP4; 0 of tensor core/MMA; 1 of CUDA graph.

Two issues are conflicts between a stated rule and code that already exists, so they are worth settling
before phase 1 lands the ArchUnit module — otherwise both become permanent allowlist entries, which
weakens every other rule:

  • ARCH-01 — Rule 7 plus the GenerationSession javadoc ("Owns KV cache") put cache ownership in the
    session, which makes a shared block pool a violation and shared prefix blocks impossible. Suggested
    fix keeps the intent (never on the model) and moves ownership up: an engine-scoped manager owns
    blocks, sessions hold leases.
  • ARCH-03 — Rule 8's enforceable form forbids ..backend....generation.., but device-side
    sampling is already on main (LogitsFP16Layer, InferenceEngine, LlamaApp.guardDeviceSample).
    Suggested fix separates generation policy (stays out of the backend) from sampling as an
    operation (may run on the device).

The 15

# Title Severity
ARCH-01 KV cache ownership as stated prevents paged and shared-prefix KV Critical
ARCH-02 No engine layer: continuous batching has no home in the layering Critical
ARCH-03 Rule 8 forbids the on-device sampler that already exists on main High
ARCH-04 Roadmap has no phase for promoting the batched-decode engine (#129) High
ARCH-05 Quantization scheme excluded from scope although P6 identifies it High
ARCH-06 Phase 1 has no numerical regression net High
ARCH-07 GenerationSession cannot express concurrent serving High
ARCH-08 Prefix / prompt cache has no owner in the layering Medium
ARCH-09 Backend SPI has no device-memory allocation seam Medium
ARCH-10 Backend SPI does not express hybrid native library dispatch (#131) Medium
ARCH-11 Rule 13 has no benchmark gate wired to it Medium
ARCH-12 Model provider SPI lands at phase 8, after the pain (#120) Medium
ARCH-13 In-flight PRs are not sequenced against the phases Medium
ARCH-14 Multi-device execution absent from the target architecture Low
ARCH-15 Public API does not expose quantization of a loaded model Low

Each issue follows the ARCH template: Observation quotes the current wording, Suggestion says what
should change, Rationale says why, Impact names the files and PRs affected.

Base is refactor/framework-abstractions so the review sits next to the documents it reviews. Happy to
split any issue further if it turns out to be carrying more than one decision.

Copilot AI review requested due to automatic review settings July 28, 2026 15:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mikepapadim

Copy link
Copy Markdown
Member Author

Added four observability issues (ARCH-16..19), so the file now carries 19.

Measured across the 13 baseline documents: 10 × "metric", 6 × "diagnostic", 1 × "observab", and 0 × telemetry, tracing, logging, counters. On main: no logging dependency in pom.xml, 65 System.out/System.err occurrences across 20 main-source files, and zero references to ProfilerMode / getProfilerResult / TornadoProfiler — so TornadoVM's device timings are available but unused.

  • ARCH-16 (High) — metrics have the opposite dependency direction to everything else: produced at the bottom (device), consumed at the top (API/server). Rule 8's form forbids backend → api, so a metrics facility in the API layer is un-callable from where the data originates. Needs a sink interface placed below the producers.
  • ARCH-17 (Medium) — observability is phase 10 of 10, yet phases 3/5/6/7 all move execution-path code and the server already exists. Split it: seam + basic timers early, exporters and memory planning late. Distinct from ARCH-06/ARCH-11, which are CI gates rather than runtime telemetry.
  • ARCH-18 (Medium) — no logging policy for a library whose stated audience is embedders; stdout printing is unroutable and unsilenceable in a server.
  • ARCH-19 (Medium) — no serving quantities (TTFT, queue wait, batch occupancy, KV utilisation, preemptions); these have to be designed with the scheduler, not retrofitted.

Copilot AI review requested due to automatic review settings July 28, 2026 15:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH responses — positions on the baseline review

Responses to ARCH-issues-mikepapadim.md (review of baseline commit 7ee6f86).

One position per issue: Accept, Accept with modification, Reject, Postpone. Positions are
proposals for the joint review, not decisions — nothing here changes docs/architecture/ until we agree
per issue and fold the accepted ones in.

Every position is grounded in a TornadoVM capability that exists, or names the capability that does not
exist and would have to be proposed upstream. API references were checked against the local TornadoVM
tree (~/TornadoVM, 5.2.1-jdk21-dev); pom.xml pins tornadovm.base.version 5.0.0. Where the two
could differ, the position says so.

Note on scope: the PR body and title say 15 issues (ARCH-01..15); the file contains 19 — its own
header table lists ARCH-16..19 (metrics seam, observability ordering, logging policy, serving metrics).
The four extra are substantive and answered here. ARCH-16 is the best issue in the set.

Summary

# Title Severity (his) Position Severity (mine)
ARCH-01 KV ownership prevents paged/shared-prefix KV Critical Accept Critical
ARCH-02 No engine layer for continuous batching Critical Accept Critical
ARCH-03 Rule 8 forbids the existing on-device sampler High Accept High
ARCH-04 No phase promotes the batched-decode engine High Accept High
ARCH-05 Quantization/dtype seam excluded from scope High Accept with modification High
ARCH-06 Phase 1 has no numerical regression net High Accept with modification High
ARCH-07 GenerationSession cannot express concurrent serving High Accept with modification High
ARCH-08 Prefix cache has no owner Medium Accept High
ARCH-09 Backend SPI has no device-memory seam Medium Accept High
ARCH-10 SPI does not express native library dispatch Medium Accept (narrowed) Low
ARCH-11 Rule 13 has no benchmark gate Medium Accept with modification Medium
ARCH-12 Provider SPI lands at phase 8 Medium Accept with modification Medium
ARCH-13 In-flight PRs not sequenced Medium Accept Medium
ARCH-14 Multi-device absent Low Accept (seam only) Medium
ARCH-15 API does not expose quantization Low Accept Low
ARCH-16 No metrics seam High Accept High
ARCH-17 Observability scheduled last Medium Accept Medium
ARCH-18 No logging policy Medium Accept with modification Medium
ARCH-19 Serving metrics undefined Medium Accept Medium

Tally: 12 Accept, 6 Accept with modification, 1 Accept-narrowed, 0 Reject. The modifications carry
partial rejections of specific sub-claims — flagged inline as Reject the sub-claim.

Two severity changes: ARCH-08 and ARCH-09 up to High (both are preconditions for ARCH-01/02, so they are
not optional extras); ARCH-10 down to Low (#131 measured parity for the decode path); ARCH-14 up to
Medium (TornadoVM already has the multi-device API, so the SPI shape decision is live now, not later).


TornadoVM capability ledger

The review's asks divide cleanly into three groups. This is the grounding for every position below.

Already available — GPULlama simply does not use it

Capability TornadoVM API Serves Used in main today?
Execution profiling TornadoExecutionPlan.withProfiler(ProfilerMode), TornadoExecutionResult.getProfilerResult()TornadoProfilerResult.getDeviceKernelTime(), getDeviceWriteTime(), getDeviceReadTime(), getTotalBytesCopyIn/Out(), getTotalDeviceMemoryUsage(), getCompileTime() ARCH-16, 17, 19, 11 No — 0 references
Device enumeration TornadoDeviceMap (getNumBackends, getAllBackends, getDevicesByType, getBackendsWithDevicePredicate) ARCH-14, DeviceSelector No
Device memory query TornadoTargetDevice.getDeviceGlobalMemorySize(), getDeviceMaxAllocationSize(), getDeviceLocalMemorySize() ARCH-09, memory planning No
Memory limits withMemoryLimit(String) / withoutMemoryLimit(); memory/TornadoMemoryProvider, XPUBuffer, DeviceBufferState ARCH-09 No
Explicit device placement withDevice(TornadoDevice), per-task withDevice(String taskName, TornadoDevice), withConcurrentDevices() ARCH-14 No
Native library tasks TaskGraph.task(...) with a library binding factory (CuBlas::cublasSgemv style); modules tornado-cublas, tornado-cudnn ARCH-10 Only in PR #131
Tensor-core MMA enums/MMAShapeM16N8K16 (fp16, bf16), M16N8K32 (int8, FP8 E4M3/E5M2) ARCH-05 Yes, in the MMA layer classes
Narrow dtypes types/arrays/HalfFloatArray, BFloat16Array, Int8Array, FP8Array, ByteArray ARCH-05, #120 BF16 FP16/Q8_0 paths
CUDA graph capture withCUDAGraph() ARCH-09 (stable addresses) Yes (llama.cudaGraphs)
Selective graph execution withGraph(int), withAllGraphs() today's phase skipping Yes

The single most useful fact in this ledger: every metric ARCH-16, 17 and 19 ask for is already produced
by TornadoVM and thrown away.
No upstream work is needed — only a sink interface on our side.

Needs no TornadoVM capability at all — pure host-side Java

Missing — would have to be proposed upstream

Gap Why it matters Position
Concurrent independent execution plans on one device. withIntraPlanConcurrency() is intra-plan; there is no documented support for two independent TornadoExecutionPlans sharing a device concurrently. Decides whether multi-session concurrency comes from parallel plans or from batching inside one plan. Design for batching inside one plan (ARCH-07). Raise the parallel-plan question upstream only if batching proves insufficient — do not design against a capability that does not exist.
FP4 / MXFP4 / NVFP4 array type and MMA shape. types/arrays/ stops at FP8Array; MMAShape stops at M16N8K32 (int8, FP8). ARCH-05 names MXFP4/NVFP4 as the flagship performance goal. Needs a new native array type + MMA shape + PTX codegen in TornadoVM. Propose upstream as a separate piece of work; keep it out of the roadmap's critical path (ARCH-05).
Per-task profiler attribution at the granularity we would want for a tok/s gate. TornadoProfilerResult gives plan-level device kernel time; per-kernel attribution today comes from nsys. ARCH-11's gate would be sharper with per-task times. Not blocking — plan-level time plus perf-history.jsonl is enough for a gate. Worth an upstream feature request, not a dependency.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-01 — KV cache ownership prevents paged and shared-prefix KV

Position: Accept. Critical, and settle before phase 1 lands the ArchUnit module.

He is right and my Rule 7 is wrong as written. I wrote it against a real problem — cache on the model,
ordering-dependent correctness — and then over-constrained the fix by pushing ownership down into the
session instead of up into a manager. "KV cache types are reachable only from session state" makes a
shared block pool a rule violation and shared prefix blocks unrepresentable.

Grounding. Lease semantics need nothing from TornadoVM: PR #129's paged mode already allocates one
pooled array and indexes blockTable[b][pos/blockSize] in-kernel, and reports the attention kernel
required no change to walk shared-then-private blocks. So this is a wording problem in my document, not a
capability problem.

What changes.

  • Rule 7 → "the KV cache is never global model state; KV storage is owned by a cache manager scoped to
    the engine and leased to sessions." Keep the model prohibition; that part was correct.
  • public-api.md GenerationSession javadoc: "owns KV cache" → "holds a KV lease".
  • ADR-001's four lifetimes gain a fifth owner (engine-scoped storage) — cleanest as a new
    ADR-005: KV cache ownership, block pools and leases, superseding the relevant part of ADR-001
    rather than editing accepted text.
  • Phase 3 scope changes: split lease from storage rather than move the cache wholesale into the session.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-02 — No engine layer: continuous batching has no home

Position: Accept. Critical.

My layering has no tier owning scheduling across sequences, so it can only express "one request, one
session, one device" — which is the throughput ceiling the project is actively removing. #129 measures
continuous batching at 1972 gen tok/s and 56.2 req/s with 82.2% slot utilization against 41× aggregate
over single-stream. An architecture that has no place for that is describing a different project.

Grounding. The engine tier is pure host-side Java — admission, batch composition, block accounting.
Execution stays exactly one compiled program invoked with B slots, which is what #129 does. No TornadoVM
capability is required, and specifically no second compiler — this does not touch ADR-003.

What changes.

  • Layering diagram: insert Engine between "Public API and generation" and "Models and sessions",
    owning LLMEngine (addRequest, step), Scheduler, KvCacheManager, BlockPool, PrefixCache.
  • terminology.md: define Engine, Scheduler, KV cache manager, Block pool, Block table, Lease, Slot,
    Prefix cache, Admission. Add "engine" to the terms-to-use-carefully table — it now collides with the
    existing InferenceEngine class, which is a generation loop, not this.
  • Dependency rules: engine may depend on models/sessions/programs; sessions must not depend on the
    engine.
  • Sessions become handles the engine schedules, not the unit of execution.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-03 — Rule 8 forbids the on-device sampler that already exists

Position: Accept. Settle before phase 1.

Verified on main: LogitsFP16Layer.DEVICE_SAMPLE runs argmax on device and writes
state.sampledToken; InferenceEngine.sampleTokenGpu reads it instead of transferring the vocab row;
LlamaApp.guardDeviceSample exists to police the preconditions. My Rule 8 enforceable form forbids
..backend....generation.., so the faster path is a violation. He is right about the consequence:
a permanent allowlist entry weakens every other rule, and rules that lose arguments with shipped code
get ignored.

Grounding. On-device sampling is an ordinary TornadoVM task in the logits graph — a kernel writing
one IntArray element. Nothing about it is a layering violation; my rule conflated what sampling is
with where the generation loop lives.

What changes.

  • Split Rule 8 into 8a (generation policy — loop, stop conditions, transport, console — stays out of
    the backend; keep the current enforceable form minus sampling) and 8b (sampling is an operation
    that may have a backend implementation).
  • program/op vocabulary in phase 5 gains Sample / ArgMax as first-class operations.
  • Rule 14's list stays as-is: core abstractions still must not require a sampler.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-04 — No phase promotes the batched-decode engine

Position: Accept. High.

My "what is deliberately not scheduled" section excluded "new performance work… must not block them",
and #129 fell through that gap. But #129 is not a performance tweak — it is continuous batching, paged
attention and prefix caching, which is the engine tier of ARCH-02 with its most demanding consumer
already written. If the refactor completes without a phase for it, phases 3 and 7 will fix session shape
and execution policy without its requirements in the room, and it gets re-implemented.

Grounding. #129 runs today on TornadoVM 5.x using MMAShape MMA tasks and withCUDAGraph().
Promotion is a host-side restructuring of bench/BatchedDecodeEngine plus the two batch-decode layer
classes; no upstream capability is needed. Its -Dbatch.decode.* properties are the same
class of process-global configuration phase 7 replaces with ExecutionPolicy, so the two phases should
be designed together.

What changes. New phase 3b — promote the batched-decode engine, after the session/state split
and before the server/API work, mapping #129's capabilities onto named components: continuous batching →
Scheduler; block pool + per-slot block table → KvCacheManager; prefix reuse → PrefixCache;
on-device sampling → an operation. bench/BatchedDecodeAttentionBench and BatchedProjectionBench
stay benchmarks.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-05 — Quantization/dtype seam excluded from scope

Position: Accept with modification. High.

Accept the distinction: new formats are correctly out of scope, the dtype seam is in scope and
load-bearing. My phase 5 said "extract reusable operations" without mentioning dtype, which would
reproduce today's multiplication inside the new vocabulary. Counted: 36 classes under
tornadovm/layers/type/** (18 FP16, 18 Q8_0), and #129 adds two more. His "~32" was close.

Reject the sub-claim that the collapse can be "M operation templates × D schemes" all the way down.
It cannot, and the reason is TornadoVM-specific: kernels are compiled per concrete native array type
(FloatArray, HalfFloatArray, Int8Array, FP8Array), and Java has no generics over primitives, so
a single matmul kernel body cannot serve every scheme. The seam belongs in the dispatch and
component layer
— one operation description parameterized by DataType, resolved to a per-dtype
kernel implementation — not inside the kernel bodies. The win is real but smaller than stated: the
per-(model × dtype × mode × MMA) class explosion collapses; the per-dtype kernel set does not.

Grounding for the flagship goal. MXFP4/NVFP4 has no TornadoVM support: types/arrays/ stops at
FP8Array, and MMAShape offers M16N8K16 (fp16/bf16) and M16N8K32 (int8, FP8 E4M3/E5M2) only. So
FP4 needs a new upstream capability — native array type, MMA shape, PTX codegen. That is a TornadoVM
proposal, and it should be raised as one; it must not sit on this roadmap's critical path. Int8 and FP8
MMA, by contrast, are available now, so a Q8_0 tensor-core matmul needs no upstream work.

What changes. Phase 5 objective gains "operations are DataType-parameterized at the description and
dispatch level"; the invariant test becomes "adding a scheme adds at most k dispatch classes, and one
kernel set". Add a note that FP4 depends on upstream TornadoVM work.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-06 — Phase 1 has no numerical regression net

Position: Accept with modification. High. This is the issue I most regret omitting: I wrote
"performance is a correctness criterion" and then specified no correctness check at all, in a repository
with one test file.

Reject the sub-claim that goldens can be "bit-identical" as a blanket property. Bit-exactness holds
only within a pinned (device, driver, TornadoVM version, backend, build) tuple — reduction order,
withCUDAGraph(), MMA paths and driver changes all move the last bits, and the OpenCL/PTX backends will
not agree with each other. Precedent that config-pinned bit-exactness is achievable: #129 verified its
batched output bit-exact against the single-stream greedy reference.

Modified proposal.

  • Tier 1 (bit-exact, in phase 1): golden logits for one small model (Llama-3.2-1B) × {FP16, Q8_0},
    fixed prompt, greedy, pinned backend — asserted bit-identical. Small enough to commit; catches the
    state-motion regressions phases 3/5/6/7 risk.
  • Tier 2 (tolerance): CPU↔GPU and cross-backend parity with his stated bound
    (|got − ref| ≤ 1e-2·Σ|wᵢaᵢ| + 1e-3).
  • Tier 3 (behavioural): per-layer dumps generated on demand for bisecting, not committed.
  • Goldens are regenerated only by an explicit, reviewed commit — never silently refreshed on failure.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-07 — GenerationSession cannot express concurrent serving

Position: Accept with modification. High.

Accept: keep GenerationSession as the simple single-sequence path, add a non-blocking submission entry
point on the engine tier, and state explicitly that the server uses the engine API, not the session
API
. My blocking generate(...) as the only entry point would have pushed every server into
InferenceService's serialize-behind-a-lock shape.

Reject the sub-claim — or rather, correct the mechanism — that concurrency comes from several
in-flight requests "sharing a device" through parallel sessions. TornadoVM does not currently expose
concurrent independent execution plans on one device.
withIntraPlanConcurrency() is intra-plan;
withConcurrentDevices() is about multiple devices. So concurrency on one device must come from
batching inside one compiled program — which is exactly #129's model, and is why ARCH-02's engine
tier is the answer to ARCH-07 rather than a thread pool over sessions.

This is a genuine design constraint imposed by the runtime, and the documents should say so rather than
leave "can two sessions run concurrently?" as an open question implying it is our choice. If parallel-plan
execution is ever wanted, that is an upstream TornadoVM proposal — not something to design against today.

What changes. public-api.md gains the engine-tier submit API (handle or reactive stream) and a
sentence stating that device-level concurrency is achieved by batching, not by parallel plans. ADR-001's
concurrency open question resolves to: sessions are independent objects; invocation is batched by the
engine; parallel plans are out of scope pending upstream support.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-08 — Prefix cache has no owner

Position: Accept. Raising severity Medium → High, because it is a precondition for ARCH-01's
wording, not an addition to it.

Grounding. Already implemented and measured in #129: -Dbatch.decode.prefixCache=true (requires
paging), shared prefix blocks pointed at by block-table prefix rows, decode starting at
pos = sharedPrefixLen, no kernel change because attention walks the block table. Reported:
419 → 211 steps, 1307 → 2422 gen tok/s, 85.7% of prefix KV saved. So this is not a future feature to
leave room for — it exists, and my baseline has nowhere to put it.

What changes. PrefixCache named in the engine tier, keyed by token-prefix hash over blocks held by
KvCacheManager. Document that prefix identity must include model, dtype and position offset, plus
eviction and refcounting. Session reset() semantics in public-api.md must state what happens to a
held lease.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-09 — Backend SPI has no device-memory allocation seam

Position: Accept. Raising severity Medium → High: an admission scheduler cannot exist without
capacity reasoning, so this blocks ARCH-02.

Grounding — this is better supported than the issue claims. TornadoVM already exposes everything
needed to make lifetime and capacity explicit: TornadoTargetDevice.getDeviceGlobalMemorySize(),
getDeviceMaxAllocationSize(), getDeviceLocalMemorySize() for capacity;
withMemoryLimit(String) / withoutMemoryLimit() for bounding a plan; the memory/ package
(TornadoMemoryProvider, XPUBuffer, DeviceBufferState) for buffer state; DataTransferMode for
residency across executions; withCUDAGraph() which is precisely the case that needs addresses stable
across replays. What is missing is only our SPI concept, not a runtime capability.

Note for the design: TornadoVM has no user-facing sub-buffer allocator, so a block pool is one large
array with in-kernel indexing (as #129 does). The seam should therefore describe pools and lifetimes,
not per-block device allocations.

What changes. Backend SPI gains buffer lifetime classes — model lifetime (weights), engine lifetime
(KV blocks), invocation lifetime (activations) — plus a capacity query. Phases 9 and 10 both reference
it; the engine tier consumes the capacity query for admission.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-10 — Backend SPI does not express native library dispatch

Position: Accept, narrowed. Severity Medium → Low.

Accept the one-sentence addition to ADR-003: an operation implementation may be backed either by a
compiled program or by a native library task, chosen by the backend from device capabilities. It is
cheap to state and it is true.

Grounding. The mechanism exists in TornadoVM — TaskGraph.task(...) accepts a library binding
factory (CuBlas::cublasSgemv style), with tornado-cublas and tornado-cudnn modules — and #131 ships
it behind -Dllama.logitsLib={jit,gemmEx,lt}. So this is not speculative.

Why narrowed. #131's own conclusion is parity, not a win: the JIT matvec at 553.8 µs/call and ~948
GB/s (94% of peak) versus cuBLAS gemvx at 550.6 µs — 0.6%, a statistical tie end-to-end, because n=1
projections are bandwidth-bound. Libraries matter in the batched/MMA paths (#127, #129), where the
work is compute-bound. So the SPI should admit library dispatch, and the roadmap should not schedule
work for it: the seam is one sentence in ADR-003 plus a note in phase 5's operation vocabulary.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-11 — Rule 13 has no benchmark gate

Position: Accept with modification. Medium.

Accept binding Rule 13 to existing machinery rather than inventing any: llama-tornado --bench
bench/LlamaBench, recording to docs/perf-history.jsonl, which already holds 1366 entries. Keep the
compiled-program-identity test as the structural half.

Reject the sub-claim that "within 3% of the previous recorded run on the same machine and model" is
expressible today. I checked the schema: entries carry timestamp, commit, short_commit, branch,
run_id, run_number, workflow, backend, model, quantization, configuration, load_duration,
eval_rate, prompt_eval_rate, total_rate, counts and durations — and no machine or runner
identity
. Records come from CI workflow runs across whatever runner was assigned. A 3% gate over
heterogeneous runners is noise, and would either fire constantly or be tuned until it never fires.

Modified proposal.

  1. Add a machine / runner identity field to the record (and GPU name — the numbers in Static batched decode (LLaMA + Qwen3): B independent sequences per step, up to 41x aggregate throughput #129/Hybrid CUDA library tasks for single-token decode: parity, not a win — measured findings + llama.logitsLib switch #131 are
    RTX 4090 specific).
  2. Gate on the tuple (machine, model, quantization, backend, configuration); compare against the last
    green run of that tuple.
  3. Tolerance stated per tuple, not globally — a 3% band is plausible on a pinned self-hosted runner and
    not on shared CI.
  4. Additionally record TornadoProfilerResult.getDeviceKernelTime() via withProfiler(...), which is a
    far less noisy signal than end-to-end tok/s and is free once ARCH-16's seam exists.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-12 — Model provider SPI lands at phase 8, after the pain

Position: Accept with modification. Medium.

Accept the problem: model support is the most frequent change here, #120 (Gemma 4) is in flight, and
each family added before the SPI enlarges the phase-8 migration and pays the P6 switch-editing cost.

Modification: split phase 8 rather than move it wholesale, because his premise ("depends mainly on
the model/session split, not on the backend SPI") is only half right.

  • Phase 8a — source detection and loading provider. Depends only on the phase 4 format/dtype work.
    Replaces ModelLoader.detectModelType string matching on general.name and ModelType.loadModel
    dispatch. Pure host-side, no TornadoVM involvement — genuinely movable early, and it is where the
    Gemma-4-shaped pain actually is. Land it right after phase 4.
  • Phase 8b — program/plan provider. Removes the ForwardPlanFactory family branches. This one
    really does depend on phase 6, because the factory produces plan components and casts State to
    family-specific subtypes; without the program layer there is nothing for a provider to return.

What changes. Phase 8 splits; 8a moves ahead of phases 5–7 in the dependency summary. Add the
explicit statement he asks for: families landing before 8a pay the switch cost and get migrated by 8a.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-13 — In-flight PRs are not sequenced against the phases

Position: Accept. Medium. My roadmap referenced no open PR, which is a real omission for a plan
whose first principle is "no rewrite".

Verified open: #120 (Gemma 4, CPU + GPU, BF16 and Q8_0), #129 (static batched decode), #131 (hybrid CUDA
library tasks), #138 (FP16 KV cache with packed half2 split-KV attention), plus #140 itself.

Proposed land order, with reasons rather than preferences.

  1. Static batched decode (LLaMA + Qwen3): B independent sequences per step, up to 41x aggregate throughput #129 first. It is the source for the engine tier (ARCH-02/04) and the only demonstrated consumer
    of paged/prefix KV. Refactoring session and cache ownership without it in the tree means deciding
    ARCH-01 twice.
  2. FP16 KV cache with packed half2 split-KV attention #138 next. It changes KV cache layout; phase 3 changes KV cache ownership. Layout before
    ownership is one conflict; the reverse is two.
  3. Gemma 4 model support (CPU + GPU/TornadoVM, BF16 and Q8_0) #120 any time before phase 8a — it is the motivating case for the provider SPI, and it is cheaper
    to migrate one more family than to design 8a without a live example.
  4. Hybrid CUDA library tasks for single-token decode: parity, not a win — measured findings + llama.logitsLib switch #131 last and independent — it is additive, default-off, and measured as parity.

Freeze declaration: inference/state/**, tornadovm/plan/** and tornadovm/layers/type/** are
"in refactor" once phase 3 opens; feature work in those trees rebases rather than merges.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-14 — Multi-device execution absent from the target architecture

Position: Accept — seams only, no implementation. Severity Low → Medium.

I am upgrading this because the framing "record the seam now, it is cheap" undersells what I found:
TornadoVM already has the multi-device API. withDevice(TornadoDevice), per-task
withDevice(String taskName, TornadoDevice), withConcurrentDevices() /
withoutConcurrentDevices(), and TornadoDeviceMap enumerating devices across backends. Per-task
placement in particular means a program's tasks can already be spread across devices without any
upstream work.

So this is not a hypothetical future capability constraining a design — it is an available capability my
target architecture ignores by describing DeviceSelector as selecting a device. That is the wrong
default shape for the SPI: an invocation should be able to target a device set, and the KV manager
should be able to hold blocks per device.

What changes. target-architecture.md records a shard-plan seam (how a program's weights and work
map to devices) and states that invocation targets a device set. Marked design-only — no phase, no
implementation. DeviceSelector keeps a single-device convenience form.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-15 — Public API does not expose quantization of a loaded model

Position: Accept. Low, uncontroversial, and he is right that it is the first thing every consumer
would otherwise obtain by reaching into the format layer — the exact leak ADR-004 prohibits.

What changes. ModelInfo exposes both weight dtype and compute dtype, as runtime DataType
(never GGMLType). Two dtypes rather than one because they already differ today:
AbstractModelLoader.effectiveGpuWeightType collapses Q4_K/Q5_K/Q6_K to Q8_0, and
getModelQuantization maps GGUF file types 14–18 to "Q8_0", so a "Q6_K model" executes as Q8_0. A
single field would be a lie for exactly the models where the user most needs the truth.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-16 — No metrics seam, device metrics cannot reach their consumer

Position: Accept. High. The best issue in the set, and the one that found a real hole in my
reasoning rather than a gap in coverage.

His structural point is correct and I missed it: metrics have the opposite dependency direction to
everything else in the design — produced at the bottom (backend, device), consumed at the top (API,
engine, operator). My Rule 8 forbids ..backend....api.., so a metrics facility placed in the API
layer is unreachable from where the data originates. Two outcomes, both bad: the backend acquires the
forbidden upward dependency, or device timings are never available.

Grounding — and the evidence that this is the default failure. TornadoVM already produces all of it:
withProfiler(ProfilerMode) plus TornadoExecutionResult.getProfilerResult() yields
getDeviceKernelTime(), getDeviceWriteTime(), getDeviceReadTime(), getDataTransfersTime(),
getTotalBytesCopyIn() / getTotalBytesCopyOut(), getTotalDeviceMemoryUsage(), getCompileTime(),
getTornadoCompilerTime(), getKernelDispatchTime(), plus profiler/ChromeEventTracer for timelines.
GPULlama references none of it — zero hits for ProfilerMode, getProfilerResult, TornadoProfiler
across all main sources. So the capability has been sitting there unused, which is precisely his argument
that without an explicit seam this data never surfaces.

What changes. A metrics sink interface in the runtime layer, below the backend SPI's consumers:
backend writes, API/engine read. Sink implementations (in-memory, bench recorder, exporter) live above;
the interface does not. auxiliary/RunMetrics (a static holder that prints) becomes one implementation.
Add a dependency rule permitting backend → runtime.metrics explicitly, so it is a designed exception
rather than an allowlist entry.

One caveat to design in: withProfiler(...) is not free. The sink must be off by default on the
decode path, with profiling a policy the caller opts into — otherwise we pay for telemetry per token.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-17 — Observability scheduled last, but earlier phases need it now

Position: Accept. Medium. Consistent with accepting ARCH-06 and ARCH-11: every phase claiming "no
behaviour change" needs evidence, and phase 10 is too late to produce it for phases 3–7.

What changes. Split my phase 10. Early (alongside the phase 1 guardrails): the ARCH-16 seam plus load,
prefill, decode and tokens/s counters. Phase 10 keeps memory planning, error-message work, exporters and
the Javadoc/experimental-marker removal. Cheap to do early because the seam is an interface and the data
already exists.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-18 — No logging policy for an embeddable library

Position: Accept with modification. Medium.

Verified: 65 System.out / System.err occurrences across 20 main-source files, including the
generation loop. He is right that this is the same class of problem as my own P2 (Model owns the
application loop) and that a library printing to stdout is unusable in a server — it corrupts structured
logs and cannot be silenced or routed.

Modification: no external logging facade dependency. He suggests "a facade (or a no-op-by-default
interface it owns)" — take the second option and commit to it. pom.xml declares no logging dependency
today, and the project ships a shaded jar with native-image considerations; adding SLF4J to a library
whose selling point is a self-contained JVM inference stack imports a dependency-hell surface for little
gain. A tiny project-owned sink, no-op by default, with an optional SLF4J bridge in an integration
module, gets the same result. It also composes with the ARCH-16 metrics sink — same shape, same
direction, plausibly the same lifecycle.

What changes. Logging policy stated in public-api.md; new Rule 16 — no System.out/System.err
outside the CLI integration — with the current 20 files as the enumerated allowlist under the existing
shrink-only policy. The CLI keeps printing; that is its job.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

ARCH-19 — Serving-level metrics are undefined

Position: Accept. Medium. Follows from accepting ARCH-02 and ARCH-16: once an engine tier exists,
"where does time go" for a single sequence stops being the interesting question.

Grounding. #129 already reports slot utilization (82.2%), steps, gen tok/s and req/s per scheduling
mode, and its paged mode surfaces a KV block pool exhausted condition — i.e. these quantities are
already being measured ad hoc to evaluate the feature. Formalizing them is recording what the work
already needs, and his point that queue-wait accounting must be designed with the scheduler rather than
threaded through afterwards is correct.

What changes. Engine tier defines: time to first token, queue wait, batch occupancy / slot
utilization, KV block utilization, preemptions, admitted versus rejected. Emitted through the ARCH-16
sink. The per-request subset (TTFT, queue wait, tokens generated) surfaces on the result type;
GenerationResult.timings() in public-api.md extends to carry it.

@orionpapadakis

Copy link
Copy Markdown
Collaborator

Consequences for the baseline if all positions are agreed

Document edits:

Document Change
target-architecture.md Engine tier inserted; shard-plan seam recorded (design-only); metrics sink placed in runtime layer
dependency-rules.md Rule 7 reworded (lease); Rule 8 split into 8a/8b; new Rule 16 (no console I/O); explicit backend → runtime.metrics permission; engine-tier direction rules
public-api.md GenerationSession javadoc (lease, not ownership); engine submit API; ModelInfo gains weight + compute dtype; logging policy; batching-not-parallel-plans statement
terminology.md Engine, Scheduler, KV cache manager, Block pool, Block table, Lease, Slot, Prefix cache, Admission, TTFT; "engine" added to terms-to-use-carefully
migration-roadmap.md Goldens + metrics seam into phase 1; new phase 3b (engine promotion); phase 5 dtype-parameterized; phase 8 split into 8a/8b; benchmark gate bound to perf-history.jsonl with a machine field; PR land order and freeze declaration
current-architecture.md New pressure points: P12 no metrics/telemetry despite available profiler; P13 console I/O in library code
ADRs New ADR-005 (KV ownership, block pools, leases); ADR-001 concurrency question resolved to batching; ADR-003 gains the library-task sentence; ADR-002 gains sampling-as-operation
New Upstream TornadoVM proposals tracked separately: FP4/MXFP4 array type + MMA shape; concurrent independent execution plans (only if batching proves insufficient)

Sequencing note: ARCH-01, 03 and 16 change rules that phase 1 would otherwise encode, so they should be
settled before the ArchUnit module lands. ARCH-02, 04, 08 and 09 are one connected decision — the engine
tier — and are best taken together rather than issue by issue.

@mikepapadim

Copy link
Copy Markdown
Member Author

Response to the positions — agreed on all 19, with two corrections to the ledger

Taking the positions as written: 12 Accept, 6 Accept-with-modification, 1 narrowed, 0 Reject. I accept
every modification, including all four sub-claim rejections, and both severity upgrades (ARCH-08, ARCH-09
to High; ARCH-14 to Medium). Conceding the sub-claims explicitly so they are settled and not re-argued:

Issue My claim Correct position
ARCH-05 collapse is "M operations × D schemes" all the way down Wrong. TornadoVM compiles per concrete native array type and Java has no generics over primitives, so the dispatch classes collapse and the kernel set does not. The invariant test should say "k dispatch classes + one kernel set per dtype"
ARCH-06 goldens can be bit-identical Wrong as a blanket property — bit-exactness holds only inside a pinned (device, driver, TornadoVM version, backend, build) tuple. The three-tier version is better than what I asked for
ARCH-11 gate at "within 3% of the previous run on the same machine" Not expressible: perf-history.jsonl carries no machine identity. Machine field first, then a per-tuple tolerance
ARCH-18 a facade "or an interface it owns" Take the second, as you say. No SLF4J in a self-contained inference library
ARCH-10 Medium Low is right, given #131 measured 0.6% on the n=1 projection path

The ARCH-16 framing is also better than mine: an explicitly designed dependency permission
(backend → runtime.metrics) rather than an allowlist entry is the whole point, and the caveat that
withProfiler(...) is not free — so telemetry must be opt-in per decode — is the part I had not thought
through.

Correction 1 — ARCH-07: the capability exists, so the reason to batch is different

TornadoVM does not currently expose concurrent independent execution plans on one device.

This is not right, and it matters because the document would otherwise record a non-existent upstream
dependency. tornado-unittests/.../multithreaded/TestMultiThreadedExecutionPlans constructs two
separate TornadoExecutionPlan instances in two Java threads
over the same immutable graph and runs
them on the default device. I ran it on the local 5.2.1-jdk21-dev tree against CUDA on an RTX 4090:
4 tests, 0 failures.

The conclusion — batch inside one compiled program — is still right, but for a reason that is stronger
than absence of an API and that belongs in the document:

  • Device buffers are per task graph. Each TaskGraph owns its own DataObjectState, so the same
    Java object referenced by two graphs gets two device buffers. That is exactly why TornadoVM #996
    had to make cross-graph aliasing explicit for consumeFromDevice. Two sessions built as two graphs
    therefore hold two device copies of the weights — for 3B-Q8 that is ~3.4 GB duplicated per concurrent
    session, which is the binding constraint long before scheduling is.
  • Thread-safety of multiple plans is exercised; device-level overlap is not a documented guarantee.

So ARCH-07 resolves as: batching inside one plan, because per-plan buffer duplication makes parallel
plans uneconomic for a weight-heavy model — not "pending upstream support". Nothing to raise upstream;
delete that item from the upstream-proposals list.

Correction 2 — the ledger is not reachable from the pinned version

You flagged that pom.xml pins 5.0.0 while the ledger was checked against 5.2.1-jdk21-dev. Checking
the release tags, the gap is load-bearing, not cosmetic:

Capability v5.0.0-jdk21 (pinned) v5.1.0 v5.2.0
FP8Array absent present present
BFloat16Array absent absent present
Int8Array, MMAShape, withCUDAGraph, withMemoryLimit, TornadoDeviceMap present present present

So "Int8 and FP8 MMA are available now, a Q8_0 tensor-core matmul needs no upstream work" (ARCH-05) is
true on 5.2, false on what we build against; and #120's BF16 path requires ≥ 5.2.0. Consequence for
the roadmap: a TornadoVM version bump is its own step, ahead of phase 1, and the ledger should carry a
"minimum version" column so an accepted position cannot silently depend on a capability we have not
adopted yet.

One more ledger row, because it was a no-op until today: withCompilerFlags(CUDA, ...) and
-Dtornado.cuda.compiler.flags were dropped in the CUDA JNI — the options string was accepted and
never passed to NVRTC (TornadoVM #1010 fixes it, and adds default|fast|debug|repro profiles; debug
= -lineinfo is what makes nsys attribute to generated CUDA C, which ARCH-11/16/17 will want). Anything
that assumed NVRTC flags were reachable was assuming wrong.

@mikepapadim

Copy link
Copy Markdown
Member Author

Addendum — five runtime constraints the engine tier has to be designed around

The ledger answers "does the capability exist". These are cases where the capability exists and
behaves in a way that constrains the design. All five come out of the TornadoVM runtime work merged
in the last week, all measured on the same 4090 box, and each one lands on a specific accepted position.

1. CUDA-graph capture and a block pool are compatible in exactly one shape — ADR-005

withCUDAGraph() bakes device addresses into the captured graph. Re-pointing a captured buffer between
replays fails at replay with CUresult=700 (TornadoVM issue #1006), and because
tornado.recover.bailout defaults to true, the first symptom is wrong output rather than an error.

So a KvCacheManager that hands a different device buffer to a slot per step cannot coexist with CUDA
graphs. One persistent pool array with in-kernel blockTable indexing — #129's shape — is not merely one
valid implementation, it is the only one that keeps graph capture. ADR-005 should state that as an
invariant
, otherwise a later "obvious" optimisation (allocate blocks on demand) silently breaks replay.

2. Any tok/s history recorded before last week is not a baseline — ARCH-11

TornadoVMInterpreter eagerly allocated and cleared an int[dependencies][32768] wait-event matrix on
every execution. Fixed in #1002 (merged): GPULlama3 53 → 103 tok/s, identical output.
Two consequences for the gate:

  • The tuple must include TornadoVM version, not just (machine, model, quantization, backend,
    configuration). A 1.94× step change sits inside the existing 1366 perf-history.jsonl entries.
  • Start-up is equally discontinuous: #1008 (merged) added the on-disk cubin cache, moving GPULlama3
    start-up 11.5 s → 5.2 s. load_duration therefore has to record cache warm/cold, or TTFT
    (ARCH-19) is unreproducible by construction.

3. Compiled-program identity was not well-defined until #999 — ADR-002, ARCH-11

Generated kernel source was non-deterministic across JVM runs: emitVariableDefs used
HashMap/HashSet keyed by objects that hash by identity, so declaration order changed run to run
(fixed in #999, merged, verified byte-identical across runs). ADR-002's program-vs-compiled-program
distinction and ARCH-11's "compiled program identity" test both need a stable hash of generated source —
that only holds from #999 onward. Worth one sentence in ADR-002 so the identity test is not written
against an assumption that was false three commits ago.

4. The engine tier will generate far more bytecode per step than single-stream — ARCH-06

The interpreter's bytecode buffer was a fixed 4096 bytes; a ~50-task graph overflowed it and the
failure was swallowed by recover.bailout, producing truncated bytecode and silently wrong results
(fixed in #1004, merged — now sized from task and object counts). A batched engine composing
B slots × 28 layers per step is exactly the shape that overflows.

Concrete ask for the ARCH-06 net: run the golden suite with -Dtornado.recover.bailout=False. The
default swallows precisely the class of failure that phases 3/5/6/7 risk, and it is why the KFusion
version of this bug looked like a tracking-quality regression rather than an error.

5. Observability: the data is there, and the log now aggregates it — ARCH-16/19

Beyond withProfiler(...), --printBytecodes gained a per-execution summary (TornadoVM #1009, open).
For a decode step on 3B-Q8 it prints one line per layer graph:

== END graph 'layer_27' | exec #25 | 11 alloc | 11 dealloc | 9 launch | h2d 0/11 (0 B) | d2h 0 (0 B) | 10 persist

h2d 0/11 (0 B) = eleven host-to-device bytecodes, none of which transferred anything. That is the
"are weights being re-uploaded per token" check as a single line, and =dot renders buffers shared across
task-graphs — which is how per-plan buffer duplication from Correction 1 becomes visible. Useful as a
phase-3 debugging tool; not a substitute for the ARCH-16 sink.


Amendments to the consequences table

Otherwise I agree with it as written, including the sequencing note.

Document Additional change
migration-roadmap.md Phase 0: TornadoVM version bump 5.0.0 → ≥5.2.x, ahead of phase 1 (FP8Array 5.1+, BFloat16Array 5.2+); gate tuple gains TornadoVM version; goldens run with recover.bailout=False; load_duration records cache warm/cold
ADR-005 Block pool is one persistent pooled array with in-kernel indexing — stated as an invariant of CUDA-graph compatibility (#1006), not as an implementation detail
ADR-002 Note that compiled-program identity depends on deterministic generated source (#999)
public-api.md ARCH-07 sentence becomes "device concurrency comes from batching because device buffers are per task graph, so parallel plans duplicate weights" — drop the "pending upstream support" framing
Upstream proposals Remove "concurrent independent execution plans" (exists, tested — 4/4 on CUDA). Keep FP4/MXFP4 array type + MMA shape as the one genuine upstream ask
current-architecture.md P14: the project builds against a TornadoVM version that predates the dtype and profiling capabilities its roadmap assumes

Happy to take ARCH-01/02/04/08/09 as the single engine-tier decision in one sitting, as you propose — and
to settle 01/03/16 first, since phase 1 encodes those rules.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: positions summary — Accepting all 19 as written: 12 Accept, 6 with modification, 1 narrowed, both severity upgrades.
Ledger checks out against the local 5.2.1-jdk21-dev tree (profiler getters, TornadoDeviceMap, memory queries, MMAShape = M16N8K16/M16N8K32 only, arrays stopping at FP8 — and 0 profiler references / 65 System.out across 20 files on our side, both exact).
One structural ask: give the ledger a minimum-version column, because pom.xml pins 5.0.0 and parts of it are not reachable there.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-01 — Agreed, and the lease/manager split is the right correction of my own framing.
One invariant for ADR-005: the block pool must be one persistent pooled array with in-kernel indexing. Handing a slot a different device buffer per step breaks withCUDAGraph() replay with CUresult=700 (TornadoVM issue #1006), and recover.bailout=true turns that into wrong output rather than an error.
So #129's shape is not one option among several — it is the only one that keeps graph capture.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-02 — Agreed, including that this needs no second compiler and does not touch ADR-003.
Worth designing for explicitly: B slots × 28 layers per step generates far more interpreter bytecode than single-stream, and until TornadoVM #1004 the bytecode buffer was a fixed 4096 bytes whose overflow was swallowed — truncated bytecode, silently wrong results.
Fixed upstream now, but it argues for the version floor being part of the engine phase.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-03 — Agreed on 8a/8b, and on keeping Rule 14 as-is.
Sample/ArgMax as first-class operations in the phase 5 vocabulary is exactly what I was after; the policy-vs-operation split is a cleaner formulation than my "carve out sampling".
Settle before phase 1 lands ArchUnit, as you say.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-04 — Agreed on a phase 3b that promotes #129 rather than re-implementing it.
Agreed too that 3b and phase 7 must be designed together: -Dbatch.decode.* is the same process-global configuration that ExecutionPolicy replaces, and splitting them means migrating those switches twice.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-05 — Conceding the sub-claim: kernels are compiled per concrete native array type and Java has no generics over primitives, so dispatch classes collapse and the per-dtype kernel set does not. Invariant test should read "k dispatch classes + one kernel set per dtype".
Correction to the grounding: "Int8 and FP8 MMA available now, no upstream work" is true on 5.2 and false on our pinned 5.0.0FP8Array arrives in 5.1.0, BFloat16Array in 5.2.0.
FP4/MXFP4 as the one genuine upstream ask, off the critical path: agreed.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-06 — Conceding the sub-claim; bit-exactness only holds inside a pinned (device, driver, TornadoVM version, backend, build) tuple, and your three tiers are better than what I asked for.
One addition to tier 1: run it with -Dtornado.recover.bailout=False. The default swallows exactly the failure class phases 3/5/6/7 risk — in KFusion the same bug presented as a quality regression, not an error.
Agreed that goldens are only regenerated by an explicit reviewed commit.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-07 — Correcting the mechanism claim: concurrent independent plans on one device are exposed and tested. tornado-unittests/.../multithreaded/TestMultiThreadedExecutionPlans builds two TornadoExecutionPlans in two threads on the default device; I ran it on 5.2.1-jdk21-dev against CUDA on a 4090 — 4 tests, 0 failures.
Your conclusion still holds, for a stronger reason: device buffers are per TaskGraph, so two graphs holding the same weights get two device copies — ~3.4 GB duplicated per session on 3B-Q8 (this is why TornadoVM #996 had to make cross-graph aliasing explicit).
So drop "pending upstream support": batching is our design choice on memory grounds, not a workaround for a missing API.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-08 — Agreed on the upgrade to High and on prefix identity covering model, dtype and position offset.
Add to the eviction design: blocks under a live lease must be pinned, because a captured CUDA graph holds their addresses (#1006). Eviction that re-points a leased block fails at replay, not at eviction.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-09 — Agreed, including that the gap is our SPI concept rather than a runtime capability, and that the seam describes pools and lifetimes rather than per-block device allocations.
One interaction to state: withMemoryLimit(String) bounds a plan, so admission must reserve against the same budget the plan is limited to — otherwise the scheduler admits a request the plan then refuses.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-10 — Accepting the narrowing to Low; #131's 0.6% on the n=1 projection path is a tie, and I over-weighted it.
One sentence in ADR-003 plus a note in phase 5's vocabulary, no scheduled work: agreed.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-11 — Conceding the 3% gate; no machine identity in the schema, so it would have been noise.
Two more fields beyond machine/GPU: TornadoVM version and cubin-cache warm/cold. #1002 moved GPULlama3 53 → 103 tok/s and #1008 moved start-up 11.5 → 5.2 s, both merged last week — so step changes of that size already sit inside the 1366 existing entries.
Agreed that getDeviceKernelTime() is the less noisy signal and is free once ARCH-16's seam exists.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-12 — Agreed; splitting into 8a (source detection and loading, after phase 4) and 8b (program/plan provider, after phase 6) is more accurate than my "move phase 8 earlier".
You are right that only half my premise held: the ForwardPlanFactory side genuinely does depend on the program layer.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-13 — Agreed on the land order and the freeze declaration; #129 first and #138's layout before phase 3's ownership are both better reasoned than "sequence them somehow".
One insertion: the TornadoVM version bump has to precede #120, since its BF16 path needs BFloat16Array, which does not exist before 5.2.0.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-14 — Agreed on Medium and on design-only.
Confirmed on the local tree: withDevice has both the single-device and the per-task (String taskName, TornadoDevice) overload, so per-task placement is available today and your "invocation targets a device set" is the right default SPI shape.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-15 — Agreed, and exposing both weight and compute dtype is a real improvement on my version.
effectiveGpuWeightType collapsing Q4_K/Q5_K/Q6_K to Q8_0 is precisely the case where one field would mislead the user who most needs the truth.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-16 — Agreed, and your framing improves on mine: an explicitly designed backend → runtime.metrics permission rather than an allowlist entry is the whole point of the issue.
Verified every getter you list exists on TornadoProfilerResult (kernel/write/read time, bytes in/out, device memory usage, compile time) and that we reference none of them.
The caveat is the part I had missed: withProfiler(...) is not free, so the sink must be off by default on the decode path.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-17 — Agreed on splitting phase 10: seam plus load/prefill/decode/tok-s counters early, exporters and memory planning late.
Cheap early precisely because the interface is the work and the data already exists.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-18 — Agreed, and take the option you chose: a project-owned no-op sink, no SLF4J, optional bridge in an integration module.
Rule 16 with the current 20 files as a shrink-only allowlist is the right enforcement shape, and it composes with ARCH-16's sink as you note.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: ARCH-19 — Agreed; formalising quantities #129 already measures ad hoc is the cheap version of this.
One dependency: TTFT is only reproducible if the record states cubin-cache warm or cold — #1008 moves start-up by 6.3 s, which dwarfs any scheduling effect you would be trying to observe.

@mikepapadim

Copy link
Copy Markdown
Member Author

Re: consequences — Agree with the table as written, and with settling 01/03/16 before ArchUnit and taking 02/04/08/09 as one engine-tier decision.
Four amendments: Phase 0 = TornadoVM bump 5.0.0 → ≥5.2.x ahead of phase 1 (FP8Array 5.1+, BFloat16Array 5.2+); ADR-005 states the pooled-array invariant (#1006); ADR-002 notes that compiled-program identity needs deterministic generated source (#999); gate tuple gains TornadoVM version and cache warm/cold.
Remove "concurrent independent execution plans" from the upstream list — it exists and is tested; FP4/MXFP4 stays as the single genuine upstream ask.

orionpapadakis added a commit that referenced this pull request Jul 31, 2026
Applies the outcome of the architecture review on PR #140, where both
maintainers recorded a position on all 19 ARCH issues. All 19 accepted,
six with modifications; none rejected. Baseline v1.0.

Rules:
  - Rule 7 reworded: KV storage is owned by an engine-scoped cache
    manager and leased to sessions. The previous wording made a shared
    block pool a violation and shared prefix blocks unrepresentable.
  - Rule 8 split into 8a (generation policy stays out of the backend)
    and 8b (sampling is an operation and may execute on device). The
    old form made the shipped on-device sampler a violation.
  - Rule 16 added: no console I/O outside the CLI integration, with
    today's 20 files as a shrink-only allowlist.
  - Rule 17 added: metrics sink in the runtime layer, a designed
    backend -> runtime.metrics permission rather than an allowlist entry.
  - Rule 18 added: engine tier direction.

New documents:
  - tornadovm-capabilities.md — capability ledger with minimum-version
    column and six runtime constraints (C1-C6) that bind the design.
  - ADR-005 — KV cache ownership, block pools and leases, including the
    CUDA-graph invariant: one persistent pooled array with in-kernel
    indexing is the only shape that keeps graph capture.
  - ADR-006 — the engine tier: scheduling, admission, batching.

ADR-001..004 accepted and amended; ADR-001's concurrency question
resolved to batching.

Roadmap rewritten as an implementation plan: Phase 0 (TornadoVM version
floor >= 5.2.x) then M1..M13, each with tasks, acceptance criteria and
dependencies. PR land order and a freeze declaration are recorded.

Two claims were corrected by evidence during review and the corrected
reasoning is now load-bearing:
  - concurrent independent execution plans on one device are supported
    and tested; batching is chosen because device buffers are per task
    graph, so per-session plans duplicate the weights;
  - the pinned TornadoVM 5.0.0 predates FP8Array (5.1.0), BFloat16Array
    (5.2.0) and several runtime fixes, hence Phase 0.

Documentation only. No production code, tests, Maven config, CI or
existing APIs were changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@orionpapadakis
orionpapadakis merged commit 7069247 into refactor/framework-abstractions Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants