wasm: introduce engine probe #14604
wasm: introduce engine probe #14604
Conversation
|
Force push: Fix the method we call to report memory usage |
There are other types of probes internal to the wasm subsystem so rename the transform probe so that we can disambiguate from other probes, since the transform probe is "external". Signed-off-by: Tyler Rockwood <[email protected]>
Signed-off-by: Tyler Rockwood <[email protected]>
|
Force push: Fix conflicts with |
rockwotj
requested review from
oleiman,
travisdowns,
StephanDollberg and
ballard26
|
sigh, failures are all known failures around decommissioning |
|
/ci-repeat |
|
new failures detected in https://buildkite.com/redpanda/redpanda/builds/40290#018b8eec-4815-4b1e-b8b6-4730e0e8ddad: "rptest.tests.rpk_registry_test.RpkRegistryTest.test_produce_consume_proto" |
|
new failures detected in https://buildkite.com/redpanda/redpanda/builds/40290#018b8ef8-aa1f-4973-8ee9-72845642a9ec: "rptest.tests.memory_stress_test.MemoryStressTest.test_fetch_with_many_partitions.memory_share_for_fetch=0.7" |
src/v/wasm/engine_probe.cc
Outdated
| } | ||
|
|
||
| engine_probe engine_probe_cache::make_probe(const ss::sstring& name) { | ||
| { |
There was a problem hiding this comment.
Bit said that there is no better pattern for this conditional insert but wouldn't just using operator[] be the easiest here?
Can just check for a default constructed nullptr shared_ptr?
There was a problem hiding this comment.
I miss some of the gtl map helpers from my time at Google, but yes default construction works fine - I've made the switch
There was a problem hiding this comment.
Sorry I meant you can then also replace the emplace call? Something like:
auto& probe = _probe_cache[name];
if (!probe) {
probe = ss::make_lw_shared<internal::engine_probe_impl>(
this, name);
}
return engine_probe(probe);
or am I missing something?
There was a problem hiding this comment.
We store these as raw pointers, and the destructor of the probe removes the map entry (a sneaky pattern), because we want this to be a cache, but to cleanup the probes when all the holders to the impl are deleted.
I'm open to better approaches here!
There was a problem hiding this comment.
Ah sorry missed that, I think you could still avoid the extra emplace call by rearranging the code a bit.
This sounds like the classical weak_ptr pattern but lw_shared probably doesn't support that.
There was a problem hiding this comment.
It does support weak_ptr but I still need to remove the map entries if you have ideas on how to do that.
ref to weak_ptr: https://github.com/redpanda-data/seastar/blob/245e0ccfa6d58d7e0dca2b4034ce1bc43e39bdc5/include/seastar/core/weak_ptr.hh#L97-L113
|
Force push: Address review comment: #14604 (comment) |
| engine_probe engine_probe_cache::make_probe(const ss::sstring& name) { | ||
| auto probe = _probe_cache[name]; | ||
| if (probe) { | ||
| return engine_probe(probe->shared_from_this()); |
There was a problem hiding this comment.
Calling shared from this is odd here, no? Just copying should be fine?
There was a problem hiding this comment.
We store them as raw pointers, see my other comment.
|
Force push: Remove |
The engine probe will be responsible for tracking all the resources that a Wasm engine is actively using, such as memory and CPU. This commit only adds memory capabilities. Signed-off-by: Tyler Rockwood <[email protected]>
To ensure that when we are reporting our absolute values across many components we don't mess up the underlying value. Signed-off-by: Tyler Rockwood <[email protected]>
Signed-off-by: Tyler Rockwood <[email protected]>
We're going to periodically report memory usage when the engine is invoked. This means on start and each time a transform is performed. We also need to report our usage to 0 when the engine is stopped. Signed-off-by: Tyler Rockwood <[email protected]>
This is an internal detail to the wasm subsystem and the wasm subsystem will be responsible for reporting VM resource usage. Signed-off-by: Tyler Rockwood <[email protected]>
|
Force push: Add pragma once to header |
|
new failures detected in https://buildkite.com/redpanda/redpanda/builds/40340#018b91af-82f3-45c4-b5af-9da70fc8d536: "rptest.tests.memory_stress_test.MemoryStressTest.test_fetch_with_many_partitions.memory_share_for_fetch=0.5" |
|
CI Failures: #14623 and bad test infra |
Introduce a probe for reporting the resource usage of WebAssembly VMs.
The intention of decoupling this from
transform_probeis that evenif we have many different "types" of VMs (authn, etc) we still want
to report the baseline resource usage.
Currently this PR only introduces tracking for memory usage.
We report the maximum (allowed) memory usage and the used memory,
which is updated when the engine is done with startup or after
a transform is invoked.
A followup PR will introduce CPU resource usage tracking.
Backports Required
Release Notes