Stream: git-wasmtime

Topic: wasmtime / PR #12152 allow recursive Wasm invocation from...

Wasmtime GitHub notifications bot (Dec 10 2025 at 17:58):

dicej requested alexcrichton for a review on PR #12152.

Wasmtime GitHub notifications bot (Dec 10 2025 at 17:58):

dicej opened PR #12152 from dicej:fix-12098 to bytecodealliance:main:

The core changes here are:

• remove an unnecessary assertion from concurrent::prepare_call
• track instance states (e.g. backpressure, etc.) on a per (Instance, RuntimeComponentInstanceIndex) basis
  - both parts of that key are needed now that concurrent state is tracked on a per-store basis rather than a per-instance basis since RuntimeComponentInstanceIndexes are not globally unique

While discussing the above with Alex, we realized the use of a HashMap to track per-instance states was both pessimal and unnecessary. Consequently, I've folded that state into ComponentInstance::instance_handle_tables, renaming it to instance_states. That involved a fair amount of code churn, but doesn't change behavior except as described in the second bullet point above.

Thanks to Alex for the test case!

Fixes #12098

<!--
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 (Dec 10 2025 at 17:58):

dicej requested wasmtime-core-reviewers for a review on PR #12152.

Wasmtime GitHub notifications bot (Dec 11 2025 at 18:18):

alexcrichton submitted PR review:

This all looks reasonable to me but I think it would be worth trying to clean up the use of tuples here and/or make it a bit more formal. For example given how frequently this tuple is created, passed around, and accessed I think it would make more sense to put it in a struct with named fields. It might be worth hanging methods off that as well?

Overall the concurrent.rs file feels a bit disorganized as over time methods have moved around to various different &self receivers based on the needs at the time, but I feel like it's pretty difficult to understand the various layers of abstractions there. To that extent I feel like this is moving the needle more towards the complicated side of things due to making the definition of "what's an instance" a bit murkier as there's sort of two definitions now instead of one.

I don't really have a concrete suggestion though unfortunate. "Just" replacing the tuple with a struct won't really do anything or move the needle on things, it just inches towards a local optima a bit more. I feel like this file is becoming more and more in need of a restructuring, refactoring, or something like that to make it more clear what methods go where, how context is passed around, and making various abstractions more first class.

Do you have any ideas about how to improve the structure internally and/or disagree about how I feel about the organization though?

Wasmtime GitHub notifications bot (Dec 11 2025 at 18:18):

alexcrichton created PR review comment:

For this almost all other *Index-based accessors panic internally on out-of-bounds, so I think it's reasonable to return &mut here instead of `Option<&mut T>

Wasmtime GitHub notifications bot (Dec 11 2025 at 19:12):

dicej commented on PR #12152:

For example given how frequently this tuple is created, passed around, and accessed I think it would make more sense to put it in a struct with named fields. It might be worth hanging methods off that as well?

Will do.

Do you have any ideas about how to improve the structure internally and/or disagree about how I feel about the organization though?

Yeah, I think some of the awkwardness stems from:

• Each function needing access to some combination of StoreContextMut/StoreOpaque/VMStore, Instance, RuntimeComponentInstance, etc., and it's rarely obvious which one of those should be self, i.e. which impl to put the method on. Personally, I'd be fine with just making them all freestanding functions and ditching any semblance of object-orientation (except for trait impls, of course), but that's the FP part of my brain talking, and I don't think it would be idiomatic with respect to the rest of the codebase.
• I've been pretty careful to ensure that each function takes only the parameters it needs, with the most specific types possible. Later, when a given function ends up needing more or less than it used to, that often means moving the function elsewhere because self is now either too narrow or too broad.
• Having gone through several iterations of the above, everything feels pretty haphazard

I don't have any brilliant ideas to address that, but one thing we could do is establish some guidelines about where to put a given method, e.g.:

• Every function that takes both a &mut StoreOpaque and an Instance should be a method of StoreOpaque (and _not_ of Instance)
• Every function that takes a &mut ConcurrentState should be a method of ConcurrentState
• etc.

I'm just making up the above, and don't care too much what the guidelines are, but having any guidelines at all could help.

Wasmtime GitHub notifications bot (Dec 11 2025 at 19:15):

dicej commented on PR #12152:

Also, we could consider splitting concurrent.rs into several, smaller modules, each of which would be easier to navigate.

Wasmtime GitHub notifications bot (Dec 11 2025 at 19:26):

alexcrichton submitted PR review:

Oh, and to clarify, I don't think a large-scale refactor should block this.

You've helped me articulate something though:

I've been pretty careful to ensure that each function takes only the parameters it needs

I'm realizing now that, with the benefit of hindsight, we should undo some of this. For example thing: Instance may want to entirely go away in favor of thing: InstanceAndRuntimeComponentInstanceIndex. Except maybe as stored in structures in-memory, there it should retain the minimal size necessary. But things like that where with the benefit of hindsight we've got a better sense now of what pieces of state/options need to be plumbed around to various locations so we can consolidate.

Overall I think it's worth having a convention for passing state around, even when not all of the state is used in every function. If it's just function arguments that's not really costly and the only time we need to think a bit harder is when it's in runtime data structures.

Well, put another way, the way I'd phrase it is that I think we should use our experience of developing this module over the past year or so to refactor/clean it up. I don't think we need to go so far as documenting conventions, but more consistently using one impl vs another, passing state, smaller files, etc, I think will all help a lot.

Wasmtime GitHub notifications bot (Dec 11 2025 at 22:39):

dicej updated PR #12152.

Wasmtime GitHub notifications bot (Dec 11 2025 at 23:27):

dicej merged PR #12152.

Wasmtime GitHub notifications bot (Dec 12 2025 at 22:55):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Dec 12 2025 at 22:55):

dicej created PR review comment:

Ugh, I forgot to address this before merging. I'll address it in a follow-up.

Last updated: Dec 13 2025 at 19:03 UTC