Stream: git-wasmtime

Topic: wasmtime / PR #10047 async/stream/future plumbing for was...

Wasmtime GitHub notifications bot (Jan 18 2025 at 00:51):

dicej opened PR #10047 from dicej:wasmtime-environ-async-plumbing to bytecodealliance:main:

I've split this out of #9582 to make review easier.

This patch includes the plumbing needed to route
async/stream/future/error-context data from wit-parser, through the various layers of wasmtime-environ, and on to wasmtime-cranelift and wasmtime. The wasmtime::runtime, wasmtime_environ::fact, and wasmtime_cranelift::compiler::component modules only contain todo!() stubs to begin with; I'll flesh those out in later PRs.

<!--
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 (Jan 18 2025 at 00:51):

dicej requested alexcrichton for a review on PR #10047.

Wasmtime GitHub notifications bot (Jan 18 2025 at 00:55):

dicej commented on PR #10047:

@alexcrichton note all the new fields I've added to VMComponentContext/VMComponentOffsets. I dimly recall that you wanted me to switch most or all of them over to libcalls, and that that would involve adding a bunch of new methods to the VMStore trait, but I can't remember the details. Perhaps you could walk me through that next week.

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

Question on this (and the tables for futures/streams). https://github.com/WebAssembly/component-model/pull/427 is in my inbox of "eventually I need to deal with this", specifically the change to place all resources for a component in a single table rather than in multiple tables. Eventually I assume the same treatment will need to happen here with futures/streams/errors/etc. Do you think it would be possible/reasonable to start moving in that direction with futures/streams/errors?

I realize it's probably easiest to do the same thing as resources at this time because that's how everything is structured. If you think it's reasonable to move all of these types into a single table though it might be good to get a head-start on that. I think it's reasonable to land either strategy though, this current separate-tables strategy shouldn't make a future migration all that much harder to implement that component model change.

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

Does rustfmt insist on this comment being at the end here or is it possible to leave it on a newline like before?

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

Should the return value here be renamed to drop the "Table" in its name to indicate that it's representing a local error context, not just the table of them? Either that or do you think it'd be good to have a newtype-wrapper around the other? (or do you feel as-is is the best balance?)

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

Temporary debugging bits

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

(also if you'd like feel free to file feature requests on wasm-tools for anything you'd have found useful during development)

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

This looks like it might be duplicating code in adapter_options below, so perhaps an extraction but a forgotten update-the-original-site-to-call-this-helper?

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

Questions on this (as you predicted I might have!)

• These are accessed from Cranelift-compiled code right? If yes on to further questions, if no, though, then I think these should move to ComponentInstance instead of VMComponentContext.
• If accessed, I might recommend moving these into a table, ideally the preexisting VMComponentBuiltins table. I also vaguely remember you mentioning that's difficult/not possible, so I might need to be re-educated...

One concern of a lot of fields like this is that profiling in the past has shown that it can be a slight bottleneck in component instantiation. For example storing a pointer-to-a-table (one pointer write) is much more efficient than storing dozens of pointers (e.g. core builtins + component builtins gets into the dozens-of-function-pointers). Not a huge concern, but it's nice to keep things more compact if we can.

Wasmtime GitHub notifications bot (Jan 21 2025 at 16:02):

alexcrichton created PR review comment:

Or, alternatively, this is what the factc example was supposed to help with so feel free to file issues/requests for that if you'd find it helpful

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:00):

dicej updated PR #10047.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:03):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:03):

dicej created PR review comment:

Yeah, looks like a case of me-being-in-a-hurry-to-get-stuff-working-and-then-forgetting-to-clean-up. I'll remove the duplication.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:11):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:11):

dicej created PR review comment:

Yeah, I was thinking of that too. The complicating factor here is that we have:

• Resources, which can be owned or borrowed
• Futures and streams, which can only be owned. Also, only the read ends can been passed around, but the same rep is used for both ends, and when the same component has both ends they just use the same end to read and write.
• Subtasks, which can only be owned and cannot be passed around.
• Error-contexts, which are immutable and reference counted

I'm not sure it makes sense logistically to put all of those in the same table given they each have very different rules. A single data structure that handles all of them at once could get pretty ugly.

One possibility might be to factor out a "rep allocator" whose only job is to allocate and deallocate u32 reps, and use those reps as indexes into separate maps containing the states for each kind of handle.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:13):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:13):

dicej created PR review comment:

@vados-cosmonic wrote this part. Victor, do you have thoughts about this?

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:15):

dicej updated PR #10047.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:16):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:16):

dicej created PR review comment:

Not sure what happened there; rustfmt seems happy with the old position, so I've moved it back.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:22):

dicej edited PR review comment.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:29):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:29):

dicej created PR review comment:

These are accessed from Cranelift-compiled code right?

Yes, correct.

I might recommend moving these into a table, ideally the preexisting VMComponentBuiltins table. I also vaguely remember you mentioning that's difficult/not possible, so I might need to be re-educated...

The only significant constraint is that I need access to a StoreContextMut<T> for all (or nearly all) of them. IIRC I should still be able to use VMComponentBuiltins, but I might need to add a function for each builtin to VMStore so that I can implement it for impl<T> VMStore for StoreInner<T>. Does that sound familiar?

Wasmtime GitHub notifications bot (Jan 21 2025 at 18:29):

dicej edited PR review comment.

Wasmtime GitHub notifications bot (Jan 21 2025 at 19:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 19:58):

alexcrichton created PR review comment:

These are good points, but I'm also curious from a component-model-spec-perspective because at least for resources we're required to throw everything into a per-runtime-component-instance table as opposed to per-runtime-component-instance-and-resource-type table that we have right now. Is the same not true for futures/streams/etc where they're, at the component model level, going into separate tables?

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:00):

alexcrichton created PR review comment:

Ah right yes, and builtins don't get access to T.

Could this be deferred to a future PR perhaps? We try to keep pretty tight control over all possible exits from wasm and adding a new one here will require both extra scrutiny as well as something along the lines of Pulley support eventually. My hope is that this is small enough we could defer this change to happening in the future.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:04):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:04):

dicej created PR review comment:

Thinking about this some more, I wonder if it would be appropriate to make ComponentTypesBuilder::error_context_table_type pub and remove this alias. In my mind, we really are talking about a table, of which there is one per subcomponent. It's different than e.g. streams or futures since it's not generic, so we don't need a separate table per (component, payload) like we do for streams/futures, but we still need one table per component. So I think having "table" in the name is appropriate and we should consistently use that term.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:11):

dicej updated PR #10047.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:13):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:13):

dicej created PR review comment:

I just pushed an update that does what I described above. Feel free to reopen this conversation if there's more to discuss.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:20):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:20):

dicej created PR review comment:

Currently, all waitables (subtasks, futures, and streams) must be in the same table because task.wait, task.poll, and async callbacks all deal with waitable handles without additional context, so there can't be any ambiguity there. AFAIK there's nothing in the spec (yet) that requires error-context or resource handles to be allocated from the same set as waitables, though. @lukewagner can correct me if I'm wrong about that and/or clarify what the plan is going forward.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:23):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:23):

dicej created PR review comment:

Ok, I'll defer this part to a future PR. Meanwhile, if you have thoughts about how you'd like that future PR to look, please let me know.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:28):

dicej updated PR #10047.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:29):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 20:29):

dicej created PR review comment:

I just pushed an update to revert the VMComponentOffsets changes.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:48):

alexcrichton created PR review comment:

Ok I think let's stay the course of what you've already got working and we can always come back in the future and start merging tables. That's on me for doing resources at some point...

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 22 2025 at 17:48):

dicej updated PR #10047.

Wasmtime GitHub notifications bot (Jan 22 2025 at 17:48):

dicej has marked PR #10047 as ready for review.

Wasmtime GitHub notifications bot (Jan 22 2025 at 17:48):

dicej requested fitzgen for a review on PR #10047.

Wasmtime GitHub notifications bot (Jan 22 2025 at 17:48):

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

Wasmtime GitHub notifications bot (Jan 22 2025 at 17:48):

dicej has enabled auto merge for PR #10047.

Wasmtime GitHub notifications bot (Jan 22 2025 at 18:19):

dicej updated PR #10047.

Wasmtime GitHub notifications bot (Jan 22 2025 at 18:19):

dicej has enabled auto merge for PR #10047.

Wasmtime GitHub notifications bot (Jan 22 2025 at 18:56):

dicej merged PR #10047.

Last updated: Jan 24 2025 at 00:11 UTC