Stream: git-wasmtime

Topic: wasmtime / PR #11490 Add PoolingAllocatorMetrics

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:13):

lann opened PR #11490 from lann:pooling-metrics to bytecodealliance:main:

This exposes some basic runtime metrics derived from the internal state of a PoolingInstanceAllocator.

Two new atomics were added to PoolingInstanceAllocator: live_memories and live_tables. While these counts could be derived from existing state it would require acquiring mutexes on some inner state.

Fixes https://github.com/bytecodealliance/wasmtime/issues/11449
<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:13):

lann requested pchickey for a review on PR #11490.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:13):

lann requested wasmtime-core-reviewers for a review on PR #11490.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:44):

lann updated PR #11490.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:45):

lann has marked PR #11490 as ready for review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:50):

lann submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:50):

lann created PR review comment:

I copied this approach from increment_component_instance_count but I could easily be convinced that it would be fine to just increment after successful allocation.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:52):

lann created PR review comment:

I was on the fence about whether to just do Ordering::Relaxed here, but if someone ends up using metrics in actual business logic (like server backpressure) it would be a pretty nasty surprise for these to underflow...

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:52):

lann submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 16:52):

lann edited PR review comment.

Wasmtime GitHub notifications bot (Aug 21 2025 at 18:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 18:53):

alexcrichton created PR review comment:

Similar to above, I'd say that using Relaxed here is fine since these aren't actually synchronizing with anything other than themselves.

Wasmtime GitHub notifications bot (Aug 21 2025 at 18:53):

alexcrichton created PR review comment:

You'll want to switch this to using a Drop implementation because otherwise this won't be correctly accounted for if the future is cancelled/dropped.

Wasmtime GitHub notifications bot (Aug 21 2025 at 18:53):

alexcrichton created PR review comment:

Given the need to have a Drop to handle the cancelled-the-future case I'd be ok just putting this in the Ok path since that would avoid the need for Drop entirely

Wasmtime GitHub notifications bot (Aug 21 2025 at 18:53):

alexcrichton created PR review comment:

I'd actually opt towards using Relaxed here since there's explicitly no data dependency with anything else in the system (e.g. this isn't looking for memory effects of another operation or publishing its own memory effects). Could you detail more about the underflow point though? How would atomic ordering affect that?

Wasmtime GitHub notifications bot (Aug 21 2025 at 18:53):

alexcrichton created PR review comment:

Perhaps debug_assert! the previous value is > 0?

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:06):

lann submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:06):

lann created PR review comment:

Ah good point; I didn't think about this hard enough when I rebased on #11470. I'll just switch to incrementing on Ok anyway.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:11):

lann submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:11):

lann created PR review comment:

Sounds good. I don't really have a good grasp of relaxed atomic semantics so I just pessimistically assume that a concurrent thread would be able to go back in time and kill its own grandparent or whatever.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:15):

lann updated PR #11490.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:16):

alexcrichton created PR review comment:

Heh nah makes sense. I'm by no means an expert but I'm confident enough in my understanding that if atomics are used for things like metric purposes it's fine to use relaxed. Relaxed ordering, AFAIK, basically means that you get no guarantees about other memory locations in the program when you observe a value. For the memory location in question, though, everything is still just as atomic as you'd expect (e.g. fetch-and-add still does that, it doesn't like switch to non-atomics). This happens because relaxed atomics can be reordered around each other by the compiler, so you can't for example say "I saw N memories in use, therefore I must see N+1 tables in use next", that's not valid.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:18):

lann updated PR #11490.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:22):

lann submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:22):

lann created PR review comment:

Yeah makes sense. I guess I'm not entirely clear on what makes increment_component_instance_count different here that it would need AcqRel?

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:23):

lann edited PR review comment.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:27):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:27):

alexcrichton created PR review comment:

Oh it's not, the main difference is I saw this here and no one saw it there most likely. There's no need for that to use AcqRel either and IMO it should also be using Relaxed

Wasmtime GitHub notifications bot (Aug 21 2025 at 19:27):

alexcrichton has enabled auto merge for PR #11490.

Wasmtime GitHub notifications bot (Aug 21 2025 at 21:45):

alexcrichton commented on PR #11490:

While the vet error is spurious, the miri failure looks legitimate.

Wasmtime GitHub notifications bot (Aug 22 2025 at 15:41):

lann commented on PR #11490:

This does indeed fail under miri locally: https://github.com/bytecodealliance/wasmtime/blob/fc3f429d72fabd4fb669f17c977c11752e078313/crates/wasmtime/src/runtime/vm/instance/allocator/pooling/metrics.rs#L76-L77

Presumably I can work around this by overriding some default config, but is it a bug that the defaults don't work with miri?

Wasmtime GitHub notifications bot (Aug 22 2025 at 15:44):

lann commented on PR #11490:

Oh well I guess that's pretty straightforward: https://github.com/bytecodealliance/wasmtime/blob/fc3f429d72fabd4fb669f17c977c11752e078313/crates/environ/src/tunables.rs#L149-L150

Any preference on how to fix this?

• Add a cfg!(miri) for default max_memory_size
• Just set max_memory_size in the test

Wasmtime GitHub notifications bot (Aug 22 2025 at 15:47):

alexcrichton commented on PR #11490:

The main test suite has a helper function for a "small pool config" which could be modeled here as well, but this test isn't too interesting for miri so it's also fine to slap a #[cfg_attr(miri, ignore)] on the test

Wasmtime GitHub notifications bot (Aug 22 2025 at 15:51):

lann updated PR #11490.

Wasmtime GitHub notifications bot (Aug 22 2025 at 15:57):

alexcrichton has enabled auto merge for PR #11490.

Wasmtime GitHub notifications bot (Aug 22 2025 at 16:25):

alexcrichton merged PR #11490.

Last updated: Dec 06 2025 at 07:03 UTC