Repository navigation
feat(memory): add registry-issued provider bindings - #1279
justinchuby wants to merge 1 commit into
Conversation
Introduce narrow provider/context, authority, mechanism, binding, and allocation identities with pinned resources, stable allocator-switch behavior, deterministic invalidation, and explicit bound capability adapters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## justinchuby-1186-memory-capabilities-phase2 #1279 +/- ##
===============================================================================
- Coverage 79.99% 79.87% -0.12%
===============================================================================
Files 362 363 +1
Lines 158583 159604 +1021
Branches 158583 159604 +1021
===============================================================================
+ Hits 126856 127483 +627
- Misses 27111 27426 +315
- Partials 4616 4695 +79
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
> **A100 safety update (head `13c37a7f`):** expected shared-prefix admission failures are now all-or-private: K/V commit is transactional, a failed V share rolls K back before enqueue, and only a successful rollback permits private-KV fallback. Fatal kernel faults remain process-fatal; the new cross-process VMM probe proves a worker fault does not affect an actively executing peer, while the current server is still in-process and does not yet consume physical shared-prefix metadata. See the [final verified conclusion](#1579 (comment)). Collapses phases 1–7 of the #1186 memory architecture rework onto current `main` as a single merge. The stack forked 270 commits ago (176 of them touching `crates/`), so rebasing it layer by layer would mean solving the same 17 conflicts fourteen times, with no reviewer ever looking at the intermediate states. Closes the stack: #1252 #1263 #1279 #1283 #1301 #1341 #1349 #1426 #1440 #1448 #1454 #1462 #1465 #1468 #1533. **A review guide is in the first comment.** It is the part worth reading — this diff is 103 files, but only three decisions in it are ones a compiler cannot check. ## What was already reviewed, and what wasn't Every phase was reviewed and approved by a session that was not its author, under the rejection-lockout rule (a rejected author never writes the next revision). | Phase | PR | Verdict | |---|---|---| | 1–5 | #1252 → #1426 | approved | | 6 | #1440, #1448, #1454 | **rejected** ×3 | | 6 | #1462 | approved | | 7 | #1465 | **rejected** | | 7 | #1468 | approved | | 7 test fixes | #1533 | approved, A100-verified | The four rejected rounds were not replaced — the later rounds are stacked **on top of** them. So the tree here is the approved state, but the history contains the rejected commits. **#1440 / #1448 / #1454 / #1465 should not be reviewed individually**; they close automatically. **Not reviewed anywhere:** the conflict resolutions themselves. That is what this PR is for. ## Verification Run here, on this merged tree: - `cargo check --workspace --all-targets` — clean - `cargo check -p onnx-genai-engine --features cuda,native-backend --all-targets` — clean. Worth calling out: the default feature set does **not** compile the `cfg(cuda)` code, which is where the riskiest edits are. Checking only the default set would have missed a real break (see the guide). - `cargo test` over 7 crates — **1105 passed, 2 failed, 88 ignored** - `cargo clippy --workspace --all-targets` - `cargo fmt` **The 2 failures and the 1 clippy error are pre-existing and proven so, not merge damage:** - `platform_capacity::{disk_capacity_is_measured_for_the_working_directory, an_explicit_byte_limit_is_honored_without_a_device_query}` — `platform_capacity.rs` is byte-identical to `main` (`git diff origin/main HEAD -- ` that file is empty). The cause is a macOS-only FFI layout bug: `fsblkcnt_t` is 4 bytes on macOS (confirmed: `sizeof(struct statvfs)=64`, `sizeof(fsblkcnt_t)=4`) while the Rust struct declares `f_blocks`/`f_bfree`/`f_bavail` as `u64`. CI is Linux, where it is 8 bytes. Left alone: it is a real bug but not this PR's. - `optimizer.rs` clippy `approx_constant` — present on both parents; the merged file is byte-identical to `main`. - Three files had `rustfmt` drift already present on `main` (`matmul_nbits.rs`, `normalization.rs`, `optimizer.rs`). Reverted rather than swept in, to keep the diff readable. **Final A100 revalidation (head `13c37a7f`):** full CUDA-memory GPU suite passed; CUDA EP default-parallel lib suite passed (**488 passed / 17 ignored**); real fp16 GQA covered three requests × two interleaved decode steps with two shared peers and one transactional private fallback, byte-identical to independent GPU and CPU references. A device-started/event-gated worker remained healthy through a peer process `CUDA_ERROR_ILLEGAL_ADDRESS`, owner exit, replacement worker, and process restart. Governor tests, targeted Miri, CI Clippy, fmt, and CUDA honesty also passed. ## Still open after this merges - `memory-plugin-provider-wiring` — the back half of Phase 6 criterion 8 (provider/context pinning), which fell in the gap between phases. **#1186 must not be closed until it lands.** This merge makes it more tractable, not less: see decision 3 in the guide. - `memory-deferred-invariant-asserts` — two surviving mutants found by #1462's reviewer, adjudicated NON-BLOCKING and not defects. - #1533's CUDA honesty guard warns on a legitimately portable anchor. Should be moved out of the CUDA test binary or allow-listed rather than left warning. --------- Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> Copilot-Session: 46c5d75b-8146-489c-b82f-08ee29c27ce4 Copilot-Session: 39ff6824-d35f-4d3b-8f5f-043a7119a100 Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
|
Closing: superseded by #1579, which collapsed phases 1–7 into a single merge — landed as This PR was approved by a session that was not its author, and its work is in Containment verified, not assumed: Every one of the 15 stack heads (#1252 → #1533) is a literal ancestor of the merged head, and I separately confirmed the merged head's content reached Why this didn't close itself: #1579's body says #1186 stays open: #1579 explicitly gates it on |
Summary
BindingRegistrythat issues provider-context, authority, mechanism, binding, and allocation identities instead of trusting allocator-reported pointers,TypeId, or tokensArc<dyn DeviceAllocator>plus opaque provider-context and authority resources in every binding/allocation/view/capability handlewith_memory(Arc<dyn DeviceAllocator>)paths as incremental adapters; no memory policy or physical-release behavior changesIdentity source and lifetime graph
The registry is the sole source of
ProviderContextIdentity,AuthorityIdentity,MechanismIdentity,BindingId/BindingGeneration, andAllocationGeneration. EachMemoryBindingpoints to one registered mechanism entry, which owns the allocator and the provider-context/authority lifetime pins. Bound allocation, view, virtual-backing, and shared-mapping metadata clone that same binding.AllocationGenerationis process-local, opaque, never pointer-derived, and checked together with the full binding identity and allocation metadata. Reusing the same VA therefore cannot validate stale metadata from an earlier allocation.Switch and invalidation semantics
Changing the selected mechanism affects only future
bind(device)calls. Existing bindings remain pinned to their original mechanism, and explicit whole-allocation release still goes through that originalDeviceAllocator. Retiring a valid mechanism rejects new work while allowing existing allocations to release.Device loss removes selection and terminally invalidates affected binding operations, including explicit release. That path performs no allocator callback, physical free, lease release, or delegated-quota refund. After externally observed provider-context/process termination and callback quiescence,
confirm_context_terminatedretires allocation identity metadata; authority/accounting reconciliation remains outside this registry.Cross-binding, cross-mechanism, cross-authority, and cross-device metadata is rejected before capability/device callbacks. A split-inner transparent bundle must use the explicit unsafe
register_trusted_compositetrust boundary; the API does not claim Rust proves hostile compositions coherent.Lock order and teardown
The registry has two non-nested lock classes:
Lookups clone an entry and release the registry lock before lifecycle inspection. Allocator, capability, validated-view, and deallocation callbacks run with neither lock held. Invalidation never waits; termination confirmation returns
ContextNotQuiescentso waiting/polling stays outside registry and governance locks. Provider-context and authority pins can be removed only after their mechanism registrations are quiescent and removed, while outstanding bound handles retain their ownArcpins.Migration and Phase 3 boundary
The new binding layer lives in
onnx-runtime-memory-apiand is re-exported from the governor compatibility surface. Existing CPU, CUDA, VMM/shared-prefix, ORT governed allocator, canonicalDeviceAllocatorrelease, capability discovery, and erased third-partyArc<dyn DeviceAllocator>behavior remain unchanged.This PR adds no allocation
Dropfree, deferred-free queue, event/fence scheduling, synchronization, physical-release completeness claim, partial-unmap recovery, quarantine, residual-handle state machine, pointer-only retry API, orProcessMemoryManagerpolicy/transaction centralization.Validation
cargo clippy -p onnx-runtime-memory-api -p onnx-runtime-memory-governor --all-targets -- -D warningspassedRUSTDOCFLAGS='-D warnings' cargo doc -p onnx-runtime-memory-api --no-depspassedgit diff --checkpassed and an independent high-confidence code review found no significant issuesExact Phase 2 baseline comparison at
d120140a:cargo fmt --all -- --checkreports only the same pre-existingonnx-genai-server/src/routes/completions.rsformatting diffs at lines 2891 and 2914onnx-genai-bench/tests/fused_batch_prefill.rsfeature/import errorsBUDGET_SHARE_DENOMINATORand unresolvedKvLayoutlinks; the changed memory API rustdoc is cleanPart of #1186 — Phase 3 only.