Improve accuracy and change semantics of metric wasm_memory_bytes#5131
Draft
gefjon wants to merge 1 commit into
Draft
Improve accuracy and change semantics of metric wasm_memory_bytes#5131gefjon wants to merge 1 commit into
wasm_memory_bytes#5131gefjon wants to merge 1 commit into
Conversation
Prior to this commit, the metric `wasm_memory_bytes` had several problems: 1. Despite its name, it was used for both Wasmtime and V8 modules. For V8 modules, it was the same value as `v8_used_heap_size_bytes`. 2. It stored only the value for a single instance at any given time, so it under-reported a database's memory usage. 3. The same row (set of label values) was written concurrently by all instances of a particular database, with each one clobbering the previously written value. In this commit, we change the metric so that: 1. It is recorded only for Wasmtime instances, not V8 instances. For V8 instances, instead directly check `v8_used_heap_size_bytes`, or one of the other V8 heap metrics. This change involved moving the recording of this metric from `module_host_actor.rs` to `wasmtime/wasm_instance_env.rs` 2. Similar to the V8 heap metrics, all the instances cooperatively share the metric entry, updating it by incrementing and decrementing rather than `set`ting. Note that this metric is used for billing, and so we will need to update our billing code (elsewhere) to account for the change. In particular, our billing code should now charge for the sum of `wasm_memory_bytes` and `v8_used_heap_size_bytes`. We also should expect with this change for each database's recorded usage to increase, as we are now accurately recording the usage for all instances, not just one.
Comment on lines
+166
to
+172
| let delta = memory_usage as i64 - self.last_observed; | ||
|
|
||
| if delta > 0 { | ||
| self.wasm_memory_bytes.add(delta); | ||
| } else { | ||
| self.wasm_memory_bytes.sub(-delta); | ||
| } |
Contributor
There was a problem hiding this comment.
Need to update last_observed
Comment on lines
1104
to
1105
| // Whenever we sample heap statistics, we cache them on the isolate so that | ||
| // the per-call execution stats can avoid querying them on each invocation. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Whenever we sample heap statistics, we cache them on the isolate so that | |
| // the per-call execution stats can avoid querying them on each invocation. |
Since we're no longer caching heap stats.
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
2 participants
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.
Marked as draft until I write its private companion; this PR will probably pass internal tests but still requires private changes.
Description of Changes
Prior to this commit, the metric
wasm_memory_byteshad several problems:v8_used_heap_size_bytes.In this commit, we change the metric so that:
v8_used_heap_size_bytes, or one of the other V8 heap metrics. This change involved moving the recording of this metric frommodule_host_actor.rstowasmtime/wasm_instance_env.rssetting.Note that this metric is used for billing, and so we will need to update our billing code (elsewhere) to account for the change. In particular, our billing code should now charge for the sum of
wasm_memory_bytesandv8_used_heap_size_bytes. We also should expect with this change for each database's recorded usage to increase, as we are now accurately recording the usage for all instances, not just one.API and ABI breaking changes
Billing metric semantics changed.
Expected complexity level and risk
3: billing metric semantics changed.
Testing
I do not know how to test metrics.