Add per-execution memory budgets to RVM - #792
Conversation
0105542 to
2f2238c
Compare
There was a problem hiding this comment.
Pull request overview
Adds opt-in, per-execution memory budgets to isolate RVM evaluations using thread-local allocator accounting.
Changes:
- Enforces fresh memory budgets for run-to-completion RVM execution.
- Adds typed Rust, FFI, and C# errors and configuration APIs.
- Adds documentation, tests, and benchmark coverage.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/memory_limits.rs |
Tests enforcement, precedence, reset, and threading. |
src/utils/limits/mod.rs |
Exports budget configuration and counters. |
src/utils/limits/memory.rs |
Defines memory budget configuration. |
src/rvm/vm/state.rs |
Resets state before capturing baselines. |
src/rvm/vm/rules.rs |
Treats budget exhaustion as fatal. |
src/rvm/vm/machine.rs |
Implements budget accounting and checks. |
src/rvm/vm/execution.rs |
Integrates budgets into execution entry points. |
src/rvm/vm/errors.rs |
Adds typed budget errors. |
src/lib.rs |
Exposes the Rust configuration API. |
mimalloc/src/mimalloc.rs |
Re-exports thread live-byte accounting. |
mimalloc/src/limits.rs |
Implements and tests live-byte sampling. |
mimalloc/src/lib.rs |
Exposes allocator accounting publicly. |
docs/limits/memory_budget.md |
Documents behavior and limitations. |
bindings/ffi/src/rvm.rs |
Adds FFI configuration and status mapping. |
bindings/ffi/src/limits.rs |
Defines FFI budget configuration. |
bindings/ffi/src/common.rs |
Adds the FFI exhaustion status. |
bindings/csharp/Regorus/StatusExtensions.cs |
Maps exhaustion to a typed exception. |
bindings/csharp/Regorus/Rvm.cs |
Adds budget configuration methods. |
bindings/csharp/Regorus/RegorusMemoryBudgetExceededException.cs |
Defines the typed exception. |
bindings/csharp/Regorus/NativeMethods.cs |
Adds native declarations and types. |
bindings/csharp/Regorus/MemoryBudgetConfig.cs |
Defines validated C# configuration. |
bindings/csharp/Regorus.Tests/RvmMemoryBudgetTests.cs |
Tests the C# API. |
bindings/csharp/README.md |
Documents C# usage. |
benches/rvm_benchmark.rs |
Benchmarks budget overhead. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Maksym (@maksym-mishchenko) Thanks for doing this very useful feature! Overall looks good to me. Copilot reviews found some interesting cases that are worth addressing. |
Hi Anand Krishnamoorthi (@anakrish), thanks for the thorough review. I addressed comments, I kept the two API suggestions unchanged for the reasons explained in their threads. Could you please take another look when you have time? |
| } | ||
|
|
||
| #[cfg(feature = "allocator-memory-limits")] | ||
| vm.set_memory_budget_config(config.memory_budget.then(|| MemoryBudgetConfig { |
There was a problem hiding this comment.
Low: These benchmark imports/calls use #[cfg(feature = "allocator-memory-limits")], but the core MemoryBudgetConfig export and RegoVM::set_memory_budget_config are gated by all(feature = "allocator-memory-limits", not(miri)). A bench build selected under Miri can therefore fail to compile.
Suggested change:
#[cfg(all(feature = "allocator-memory-limits", not(miri)))]
use std::num::NonZeroU64;
#[cfg(all(feature = "allocator-memory-limits", not(miri)))]
use regorus::MemoryBudgetConfig;
#[cfg(all(feature = "allocator-memory-limits", not(miri)))]
vm.set_memory_budget_config(config.memory_budget.then(|| MemoryBudgetConfig {
limit: NonZeroU64::new(MEMORY_LIMIT_BYTES).expect("non-zero memory budget"),
}));Please use the same predicate for both imports and the configuration block.
| let result = RegorusResult::ok_string(json); | ||
|
|
||
| #[cfg(all(feature = "allocator-memory-limits", not(miri)))] | ||
| if let Err(err) = guard.check_memory_budget() { |
There was a problem hiding this comment.
Medium: If this post-serialization budget check fails, the FFI returns MemoryBudgetExceeded after dropping only the provisional JSON result, but the VM has already stored the value as ExecutionState::Completed { result }. A caller can then call regorus_rvm_get_execution_state() and observe/re-serialize the oversized completed result despite the reported failure.
Suggested shape:
if let Err(err) = guard.check_memory_budget() {
regorus_result_drop(result);
guard.mark_execution_error(err.clone()); // clear retained result/state
return Err(err.into());
}Alternatively, move the final serialization check into a core-owned completion helper that transitions the VM to ExecutionState::Error and releases the retained result before returning. Please add a regression asserting the state is Error after this failure.
| .jump_to(0_u32) | ||
| .map_err(|err| self.apply_memory_budget_precedence(err)) | ||
| .and_then(|value| { | ||
| self.check_memory_budget()?; |
There was a problem hiding this comment.
Medium: When this final budget check fails, the Err arm records ExecutionState::Error but leaves the failed execution allocations (registers, rule_cache, evaluated, and pooled values) resident until the next execution or VM drop. The named/indexed entry-point paths and the FFI post-marshalling failure do not clean them up at all. Reusing a VM after repeated oversized failures can therefore accumulate roughly one failed result/state per cycle.
Suggested shape:
fn fail_execution(&mut self, err: VmError) -> VmError {
self.release_previous_execution_state();
self.execution_state = ExecutionState::Error { error: err.clone() };
err
}Use this helper in the run-to-completion Err arm, the named/indexed entry-point budget-error paths, and the FFI post-marshalling failure path. Preserve the error value while releasing the result/register/cache state, and add a regression that repeats budget failures on a reused VM and verifies memory/state are reset.
The rationale for both sounds good. Review rerun found 3 more comments worth addressing. Then it should be good to go. |
|
Maksym (@maksym-mishchenko) I see the following drawbacks in this implementation:
Why haven't you pinned thread level baseline and used every allocation (delta gated) for comparison (just like a global limit)? Why have you chosen execute() calls? Consider whether the intended primitive is a scope ( Anand Krishnamoorthi (@anakrish) any thoughts on the above ^? Apart from the design: Follow-ups on the fixes from the previous review round
New
|
Mark Birger (@kusha) However, I do see that it would be nice to also limit input, data memory consumption. Some challenges around implementation:
Some ideas
|
Maybe this would be the simplest approach. Have a thread level limit on alive memory? |
Yes. That was also my impression from the PR description and the implementation. An execution-level budget is the clearest semantic: it bounds the additional working memory used during one RVM execution and intentionally excludes objects such as input, data, and the program that may have been created before—and may outlive—that execution. However, I do agree that large data/input may go undetected by this budget, even if they are eventually subject to configured global memory limit. We should meet to discuss your scenario and see if the design can be generalized and the semantics made more clear/easy to reason about. |
|
Mark Birger (@kusha) Anand Krishnamoorthi (@anakrish). I made another pass based on this discussion. The existing I avoided a Program loading and compilation are still excluded. I also left input and context out for now. Is covering data enough for the fetch scenario, or do you think input should be included before we approve the API? The cleanup and documentation issues are fixed as well. The benchmark showed no measurable difference: 859.82 ns without the budget and 847.59 ns with it, with overlapping intervals. |
|
Maksym (@maksym-mishchenko) |
|
I found a performance issue in the new memory-budget enforcement path: With allocator limits enabled, each call can sample the budget and run the throttled global-limit check, so this adds avoidable hot-path overhead and makes checkpoint cadence effectively 2x what the execution loop suggests. Suggested fix: retain the check at one dispatch boundary (preferably the outer execution loop, unless a specific instruction-level invariant requires the inner check), then add a test-only check counter or benchmark assertion to verify one check per dispatched instruction. Please also compare the benchmark with one vs. two checks so the overhead is measurable. |
|
Documentation suggestion — cooperative enforcement and overshoot: Please make the hard-cap distinction especially explicit in |
|
Documentation suggestion — VM reuse and warm pools: Please document that budget outcomes can depend on prior executions when the same |
|
Documentation suggestion — baseline ratcheting and the Please clarify that the baseline is lowered when sampled same-thread live bytes fall below the prior baseline, and that this lost headroom is not restored. As a result, the effective budget can become stricter after unrelated or legitimate same-thread frees, and |
|
Documentation suggestion — host callbacks and cross-thread frees: Please call out that accounting is based on the current thread’s live-byte counter, not allocation ownership. Allocations/frees performed by synchronous custom or host builtins on the execution thread are therefore observed as part of the budget; objects allocated on one thread and freed on another can temporarily skew per-thread/global observations until counters are published. This would help embedding applications avoid assuming strict query-owned attribution or thread-independent accounting. |
Add optional cooperative memory budgets for run-to-completion RVM execution across Rust, FFI, and C# surfaces. Reject budgeted suspendable execution and release retained execution state on budget failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 150dc69c-19c0-485f-b880-8db6e8d25a31
e5da647 to
a1dc5d3
Compare
AB#3522638
A process-global allocator limit cannot isolate individual policy evaluations and may cause unrelated requests to fail. This adds optional per-execution memory budgets for run-to-completion RVM evaluations.
Enforcement is cooperative and checkpoint-based. The budget observes current-thread live bytes relative to a fresh execution baseline; it is not an allocation-time peak cap, so one instruction, builtin, native serialization step, or
CStringallocation may temporarily overshoot. Reported usage is a diagnostic thread-level change rather than exact query-owned memory. Synchronous callbacks on the execution thread affect accounting, cross-thread frees may temporarily skew observations, and a lower live-byte sample ratchets the baseline downward without restoring lost headroom.A reused VM captures a fresh baseline after prior execution state is released, but retained capacities and pools precede that baseline, so warm and fresh VMs may allocate differently. Program compilation and loading data, input, or context remain outside the execution budget. Native result serialization and
CStringconstruction are included; managed C# UTF-8 decoding and managed-string allocation are excluded.Budget failures return additive typed Rust, FFI, and C# errors, invalidate completed results, release retained execution state, and take precedence over the process-global allocator limit. Budgeted suspendable execution is rejected because thread-local accounting cannot safely span host-await thread migration; host-await accounting is separate follow-up work.
The opt-in controls are
RegoVM::set_memory_budget_configand C#Rvm.SetMemoryBudgetConfig; existing behavior is unchanged when no budget is configured.