Refund VM linear-memory budget#1712
Conversation
Wrap the wasmi Store's user-data type in a dedicated VmStoreData struct (currently just holding the Host handle) instead of using Host directly, and move the ResourceLimiter impl from Host onto VmStoreData. Thread the Host -> VmStoreData type change through every Store/Linker/Caller/VmCaller/ StoreContextMut/VmUserState use site. Pure refactor with no behavior change: this gives each VM store its own data slot, which a subsequent change uses to track per-VM linear memory.
jayz22
changed the title
f3826e9 to
bd88b89
Compare
The memory budget has always measured cumulative memory -- the sum of all allocations -- and does not credit back transient memory that is later freed. For most invocations cumulative memory stays close to peak live memory, so this is a fine proxy. But each VM instantiation charges the contract's WASM linear memory (~1 MiB with default toolchain settings), which is freed at VM teardown; under deep or wide cross-contract calling, many VMs are created and destroyed and cumulative memory diverges sharply from peak, significantly overcharging for memory never held at once. Track the mem_bytes charged for each VM store's linear memory in its VmStoreData (initial allocation + runtime grows), and refund it to the budget's memory dimension when the store -- and thus its linear memory -- is freed at Drop. Scope is VM linear memory only; CPU is unaffected. The refund credits only the real budget total, never the shadow budget. Adds Budget::refund_mem and BudgetDimension::refund, and updates the affected metering assertions.
Mechanical regeneration (UPDATE_OBSERVATIONS=1) reflecting the reduced net memory now that transient VM linear memory is refunded at teardown. No hand edits.
bd88b89 to
c82d5d5
Compare
| .host | ||
| .as_budget() | ||
| .is_in_shadow_mode() | ||
| .map_err(|_| errors::MemoryError::OutOfBoundsGrowth)? |
There was a problem hiding this comment.
Are there any other error codes besides OutOfBoundsGrowth? This will hide an invariant error behind a budget error, which is not desirable.
Also, this check looks redundant to me TBH, I don't think this should care which budget mode is being used.
| @@ -1396,7 +1396,7 @@ mod test { | |||
| expect![[r#" | |||
| InvocationResources { | |||
| instructions: 320615, | |||
| mem_bytes: 1135822, | |||
| mem_bytes: 87230, | |||
There was a problem hiding this comment.
I don't think we need to block the PR on this, but this change is quite misleading (we have hidden the invocation completely).
I think we should report max mem_bytes for any invocation. That might be slightly confusing with sub-invocations, but that's still better than completely removing most of the memory observations because VM frame gets popped when the invocation ends.
| @@ -1874,7 +1874,7 @@ mod test { | |||
| ), | |||
| data: String( | |||
| ScString( | |||
| StringM(invocation resource limits are exceeded: instructions: 1788420 > 10, memory bytes: 1163561 > 20, contract events size bytes: 800 > 50, contract data key 'ContractData(LedgerKeyContractData { contract: Contract(ContractId(Hash(2e0ff7a55065f3a896723a964c3d9862a4722bfc77229fe4875f390ef2a0027e))), key: LedgerKeyContractInstance, durability: Persistent })' size: 48 > 25, contract data entry with key 'ContractData(LedgerKeyContractData { contract: Contract(ContractId(Hash(2e0ff7a55065f3a896723a964c3d9862a4722bfc77229fe4875f390ef2a0027e))), key: LedgerKeyContractInstance, durability: Persistent })' size: 104 > 35, contract code entry with key 'ContractCode(LedgerKeyContractCode { hash: Hash(1a2ee5a89dd2162a20e716b3e2de70a2d662480a9565350378661871bcf8e398) })' size: 1840 > 45), | |||
| StringM(invocation resource limits are exceeded: instructions: 1788420 > 10, memory bytes: 114969 > 20, contract events size bytes: 800 > 50, contract data key 'ContractData(LedgerKeyContractData { contract: Contract(ContractId(Hash(2e0ff7a55065f3a896723a964c3d9862a4722bfc77229fe4875f390ef2a0027e))), key: LedgerKeyContractInstance, durability: Persistent })' size: 48 > 25, contract data entry with key 'ContractData(LedgerKeyContractData { contract: Contract(ContractId(Hash(2e0ff7a55065f3a896723a964c3d9862a4722bfc77229fe4875f390ef2a0027e))), key: LedgerKeyContractInstance, durability: Persistent })' size: 104 > 35, contract code entry with key 'ContractCode(LedgerKeyContractCode { hash: Hash(1a2ee5a89dd2162a20e716b3e2de70a2d662480a9565350378661871bcf8e398) })' size: 1840 > 45), | |||
There was a problem hiding this comment.
This is a more concerning side-effect of the issue I've mentioned above: we use the invocation metering for the resource limit enforcement, so not using the max memory results in wrong checks. I think the prod memory reporting path is also broken because we don't track the max memory anywhere. Maybe that's worth addressing in this PR after all.
| /// refunded. The only refund site is [Vm::execute_with_mem_refund], the | ||
| /// canonical way to use a VM (see the rationale there); a VM used outside | ||
| /// that scope (tests and benches) keeps its memory charged. | ||
| pub struct VmStoreData { |
There was a problem hiding this comment.
I'm not sure what to think of this change. It seems to create a weird coupling between the host and budget charges, but host itself also manages the budget. Compared to the sketch that I did with Claude, the budget itself is affected much less (though you're missing the max memory tracking that we still need), but this is much more invasive change scope-wise.
So I came to think that it would make more sense to keep the refund stack in the host/budget in some way. I'm not sure if my sketch has the best version of this, maybe we could try tying the refunds to a Frame (that's also going to be more scalable if in the future we decide to also drop, say, host objects allocated by a given frame). If that's inconvenient, I think maintaining a separate stack is fine as well - we do that for a few different per-frame entities (like auth managers).
Resolves #1707
What
VmStoreDatatype that tracks linear-memory allocation per VM.Vm::execute_with_mem_refundthat runs a VM and refunds its memory when the scope exits, applied to every production path that instantiates a VM - contract invocation, the protocol-version probe, and wasm upload validation. As a result the memory cost of all of these is reduced.Why
The memory budget has always measured cumulative memory and never credits back transient memory that is later freed. For most invocations that is a fine proxy. But each VM instantiation charges ~1 MiB of linear memory (the toolchain-default shadow stack) that is freed at teardown yet never refunded, so deep or wide cross-contract calling diverges from peak and overcharges contracts for that memory - making contract interactions needlessly expensive and prone to hitting the memory limit.
The tracker lives in
VmStoreDatawhich is the object wasmi owns (originally just aHosthandle for the metering callback), per-VM. The refund happens when the VM is dropped, guarded automatically by the scoped function (like aDropimpl, but fallible).