Repository navigation
refactor(engine): give the workflow runtime's state named owners (W=1 state split) - #2044
Merged
Merged
Conversation
`WorkflowRuntime` held the compiled plan, the ORT and native backend handles, the caches derived from them, every counter and the interpreter's session cells as one flat field list. Which of `docs/architecture/SESSION_CONCURRENCY.md` §3's three categories a field belonged to was therefore a fact about the comment above it, and a second worker could be given state it must not share without anything failing to compile. This splits the list into the owners §3 already names, and nothing else: - `WorkflowPlan` (§3.1) holds the frozen package data — workflow, compiled graph, derived output sets, memory plan, backend, adapter service, preprocessing, speculative contract — behind an `Arc`, with a static assertion that it is `Send + Sync`. A mutable cell added to it fails the build rather than quietly making the plan unshareable. - `WorkerBackend` (§3.2) holds the component sessions, execution islands and native component set. - `WorkerRuntimeState` (§3.2 storage under §3.3 access) holds the bindings, allocators, device values, embedding tables, session cells, session leases, iteration variants, adapter cache and counters. - `WorkflowPass` (§3.3) is created when a pass opens and dropped when it ends, carrying the pass id and the telemetry that used to be two unrelated locals. Both worker types are structurally `!Send`/`!Sync` through a `PhantomData` marker — not a new `unsafe impl`, not a lock — and static assertions hold them there. `Engine`'s existing `unsafe impl Send` is untouched. The id namespaces are made explicit at the same time, because a future worker that mints from the wrong one hands two conversations the same id: - `PassId` (was the bare `workflow_execution_generation` counter) and `GraphCaptureId` mint from worker-local allocators, since both are only ever compared against state the same worker owns. Component capture ids came from `bindings.len()`, which is a counter only while nothing is ever removed from that map; islands seeded a hand-rolled `island_id * 1000` stride, which is kept for log readability. Exhaustion is now a typed error instead of a wrap into ORT's `-1` uncaptured sentinel. - Session ids are the only ones that leave the engine, so they mint from `engine::ids::SharedSessionIds`, an atomic allocator. Ids still start at 1 and still increment by 1, so nothing externally observed changes. #2019's lifetime contracts are preserved deliberately. `WorkflowRuntime`'s `Drop` still clears island bindings and then values before allocators — a `Value` is a bare `OrtValue` handle with no back-reference to its allocator, so nothing in the type system orders that release — and the clears now live in `WorkerRuntimeState::release_ort_state`, called from the same `Drop`. `worker` is declared before `backend` and `_ort_environment` stays last, so the field order agrees with the explicit teardown instead of contradicting it. This is a `W = 1` structural change only. There is still one worker and one thread, no pre-enqueue lease, no server routing, and batching semantics are untouched. Tests: static assertions for the plan being shareable and the runtime being neither `Send` nor `Sync`; the plan really crossing a thread boundary behind an `Arc`; pass ids being fresh, worker-local and starting at "none"; capture ids being unique, non-negative and disjoint per island; session ids being collision-free across eight threads; a conversation still continuing and still being released through the worker's own cells; the inner session lease still refusing a second holder by name; and the drop path still releasing cached values before the allocators that own their memory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2044 +/- ##
==========================================
+ Coverage 80.25% 80.49% +0.23%
==========================================
Files 409 425 +16
Lines 189791 209135 +19344
Branches 189791 209135 +19344
==========================================
+ Hits 152319 168334 +16015
- Misses 32048 35141 +3093
- Partials 5424 5660 +236
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The first bounded step of
docs/architecture/SESSION_CONCURRENCY.md§13Phase 1's state split:
WorkflowRuntime's flat field list becomes the owners§3 already names.
W = 1structural prerequisite only — this PR makesnothing concurrent and claims nothing about concurrency.
Before, the runtime held the compiled plan, the ORT and native backend
handles, the caches derived from them, every counter and the interpreter's
session cells in one list, so which §3 category a field belonged to was a
fact about the comment above it. A future worker could be handed state it
must not share without anything failing to compile.
The split
WorkflowPlanbehindArcWorkerBackendWorkerRuntimeStateWorkflowPassWorkflowPlancarries a static assertion that it isSend + Sync— amutable cell added to it fails the build rather than quietly making the plan
unshareable. Both worker types are structurally
!Send/!Syncthrough aPhantomDatamarker: no newunsafe impl, no blanket lock, and staticassertions keep it that way.
Engine's existingunsafe impl Sendisuntouched.
Id namespaces
Made explicit, because a future worker minting from the wrong one hands two
conversations the same id.
PassId(was the bareworkflow_execution_generationcounter) and
GraphCaptureId. Both are only ever compared against statethe same worker owns. Component capture ids came from
bindings.len(),which is a counter only for as long as nothing is removed from that map;
islands seeded a hand-rolled
island_id * 1000stride, kept because itmakes a captured graph's id say which island captured it. Exhaustion is now
a typed error rather than a wrap into ORT's
-1uncaptured sentinel.mint from
engine::ids::SharedSessionIds, an atomic allocator. Ids stillstart at 1 and still increment by 1 — nothing externally observed
changes.
#2019's contracts are preserved
WorkflowRuntime::dropstill clears island bindings, then values, thenallocators: a
Valueis a bareOrtValuehandle with no back-reference toits allocator, so nothing in the type system orders that release. The clears
moved into
WorkerRuntimeState::release_ort_state, called from the sameDrop.workeris now declared beforebackend, and_ort_environmentstays last, so the field order agrees with the explicit teardown instead of
contradicting it.
Explicitly not in this PR
No
N > 1. No pre-enqueue lease. No server routing.EngineOwner'sunsafe impl Sendis not removed. Batching semantics unchanged. No publicAPI change — the split is private to the crate and every accessor keeps its
signature.
Tests
New, in
pipeline/runtime_state.rs,pipeline/mod.rsandengine/ids.rs:Send + SyncbehindArc;WorkflowRuntimeisneither
SendnorSync; each worker type and id allocator likewise.Arcand reads thesame on the other side.
mints in its own namespace without advancing its parent's.
across islands, and exhaustion is refused rather than wrapped.
concurrent minters.
worker's own cells, and the inner session lease still refuses a second
holder by name.
their memory.
Verification
cargo clippy --workspacecannot run in this worktree:git worktree adddoes not populate the
crates/onnx-runtime-cpuinfo/vendor/cpuinfosubmodule, which is an environment fault rather than a result. The crates
this change touches are covered above.
Docs
SESSION_CONCURRENCY.md§13 Phase 1 gains a note that the state splitproper has landed for the interpreter, and that the phase is not done:
the
Box<Session>holders and thethread_affinityextension to the nativehandles remain. Citations into
pipeline/mod.rsthat the move invalidatedare repointed at
pipeline/runtime_state.rs.