wasmtime / PR #10083 add component-model-async/lift.wast ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10083 add component-model-async/lift.wast ...

Wasmtime GitHub notifications bot (Jan 22 2025 at 22:42):

dicej requested alexcrichton for a review on PR #10083.

Wasmtime GitHub notifications bot (Jan 22 2025 at 22:42):

dicej opened PR #10083 from dicej:async-lift-test to bytecodealliance:main:

This is another piece of #9582 which I'm splitting out to make review easier.

This test includes two components: one which exports a function using the async-with-callback ABI, and another which uses the async-without-callback ABI. It doesn't actually instantiate or run either component yet.

The rest of the changes fill in some TODOs to make the test pass.

Wasmtime GitHub notifications bot (Jan 22 2025 at 22:42):

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

Wasmtime GitHub notifications bot (Jan 22 2025 at 22:42):

dicej requested wasmtime-fuzz-reviewers for a review on PR #10083.

Wasmtime GitHub notifications bot (Jan 22 2025 at 22:47):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 22 2025 at 22:47):

dicej created PR review comment:

This file was shamelessly copied from resources.rs and modified for use with stream/future/error-context state. Those handles are different enough from resource handles that I didn't want to try to address everything with a single data structure. I'm open to other approaches, though.

Wasmtime GitHub notifications bot (Jan 22 2025 at 23:08):

dicej updated PR #10083.

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

dicej updated PR #10083.

Wasmtime GitHub notifications bot (Jan 22 2025 at 23:23):

dicej updated PR #10083.

Wasmtime GitHub notifications bot (Jan 23 2025 at 01:05):

github-actions[bot] commented on PR #10083:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jan 23 2025 at 02:07):

github-actions[bot] commented on PR #10083:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton submitted PR review:

I'm definitely pretty far out of my comfort zone reviewing this in the sense that there's enough pieces that I don't see how they're connected just in this PR that I'm not confident I have the ability to review without going over to the spec. At a high level the test added here seems like it probably doesn't require the lift/lower impls, but then again I suspect that removing those impls would remove all the meat of the PR. Would it be possible to add some tests exercising the impls added here in this PR?

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

To confirm, this'll eventually be a concurrent.rs in a future PR? So just a placeholder for now?

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

Could this perhaps be impl<T> From<FutureReader<T>> for Val?

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

If you'd prefer to simplify this I think it's reasonable to just fold this into the component-model feature above too

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

For this and the below change I had to reread and re-figure-out what I was originally intending with this. I think it would be preferable to not add pub(crate) here as that expands the number of locations that need to be audited when working with this field.

I think it should be ok though to add accessors like fn instance(&self) -> &ComponentInstance and fn instance_mut(&mut self) -> &mut ComponentInstance to these types (lift/lower contexts). That would remove the need for unsafe at access sites and might clean up some preexisting access sites as well

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

How temporary is the representation of these structures? E.g. is the u32 what it'll be in the long run?

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

The ::<()> bit here and below gives me a bit of pause. Is that temporary or final-into-the-future?

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

Personally I know I'm in a state of "I'm not a component-model async expert" so coming across bits of nontrivial code like this I'm left to go find where in the spec this behavior is implementing and trying to cross-reference that for example. Basically I find myself unable to reason about this locally and understand, just from what's here, what's going on.

Could you add some comments for any sort of nontrivial transformations like this? For example with resources I tried to leave comments about all the various state transitions in lifts/lowers.

I suspect the majority of future readers of this code are also unlikely to be component-model async experts and comments can help quite a bit in understanding what's going on. The more I read in various places in this PR this is also applicable in general to any nontrivial bits here as opposed to just this one function. For example get_mut_by_index_from above is easy enough to understand as it's pretty mechanical internally but in isolation I'm not really sure what it's doing in a broader sense just from reading it. It may not be too helpful to understand at-a-glance what it's doing but at the same time having a comment I think would still be helpful.

(aka this is sort of a general request for more comments on anything nontrivial)

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

Mind adding some comments as to what this rep is? My naive understanding of component model async is probably showing here but I'm familiar with "rep" of a resource where it's user-supplied but I didn't think that there was anything user-supplied here but rather this is an index to something (but I'm not sure)

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

I'll definitely admit that I'm very lost when it comes to error-context here. I see a local refcount table and a global refcount table but I don't actually see the global refcount table being used. I'm also not really sure what rep is in the context of an error-context so I'm pretty lost here.

While comments would help here is this something that may be best deferred to a future PR? For example ideally there'd be a test exercising this lifting and lowering and testing various errors here and things like that, but I suspect such a test isn't quite ready yet?

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

s/error/error-context/

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

I'm not sure how much this will persist int he future but you can drop the <Data = U> here in the sense that it's not necessary as part of this definition. In the future if you need bounds on U it should be possible to do S::Data: Bound I think too

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

This might be one part about error-context I don't understand b/c I think I'm missing where this is used...

Wasmtime GitHub notifications bot (Jan 23 2025 at 23:57):

alexcrichton created PR review comment:

Could you expand the documentation for this struct as to what problem it's solving? For example what extra functionality it has over ResourceTable. I'm a bit surprised for example of the reps_to_indexes reverse mapping where most of the component model stuff doesn't require hash maps today and that seems like it's bordering on needing something like that.

Also this is something I probably should have done for ResourceTable but if it's possible this'd be a good place to have some #[test] unit tests for this data structure doing some simple pushes/pops/etc.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:16):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:16):

dicej created PR review comment:

concurrent.rs is already in main and was added in https://github.com/bytecodealliance/wasmtime/pull/10044 (and will be expanded in subsequent PRs). This is the stub module to be used when the component-model-async feature is disabled, so it will only go away when that feature is removed.

Alternatively, I could remove this stub module and make concurrent.rs the only implementation, but that would mean adding more #[cfg(...)] annotations elsewhere, so it's kind of a tradeoff. Happy to do whichever you think is best, though.

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

dicej submitted PR review.

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

dicej created PR review comment:

They're like resource reps in that they'll be u32s for wasm32 but become u64s when we add wasm64 support for the component model.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:29):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:29):

dicej created PR review comment:

It's "final" in that I wasn't planning to change it, but I agree it looks weird. I had already written the static API Lift/Lower impls for these types, so I figured I'd reuse those implementations here since they're payload-type-agnostic. I'll move the lifting and lowering code into separate, top-level functions which both the static API and dynamic API can use, which will avoid this.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:34):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:34):

dicej created PR review comment:

Agreed and apologies for forgetting to add comments; I have "Add doc and code comments to new, non-trivial code (especially concurrent.rs and futures_and_streams.rs)" in the task list on #9582, and that was my next priority before I switched my focus to breaking the PR up into smaller pieces. I'll start doing both now.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:34):

dicej edited PR review comment.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:41):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:41):

dicej created PR review comment:

Good catch. This is just a stub, with the signature copied from https://github.com/dicej/wasmtime/blob/d1596df0361a4a8c956ba7ef34ac8d3c5e0140e1/crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs#L578, but it looks like the real version doesn't use U either, so I'm guessing I had originally been using U, then stopped using it, but forgot to update the signature.

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

dicej submitted PR review.

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

dicej created PR review comment:

I might have the terminology mixed up myself, but I've been using "rep" to mean "the instance-agnostic index for the waitable" which used to look up the state of the stream/future/task in a table (which will be added in a later PR), akin to the ResourceTable index for resources. When lifting and lowering we convert between that index and a per-instance local index visible to the guest.

If there's a more conventional way to refer to these "global" and "local" indexes, I'm happy to adopt it.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:56):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:56):

dicej created PR review comment:

Yeah, I can replace this stuff with stubs. My approach with this PR was to run the lift.wast test, hit a todo!(), replace the todo!() with code copied from #9582, let the compiler tell me what _other_ code _that_ code needed, then either copying more code or adding stubs to make the compiler happy. Looks like I copied over too much, and that's making the review more difficult; sorry about that. I'll trim it down.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:57):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 00:57):

dicej created PR review comment:

I don't think it _is_ being used yet; again, I may have copied over more than necessary from the Big PR.

Wasmtime GitHub notifications bot (Jan 24 2025 at 15:50):

dicej updated PR #10083.

Wasmtime GitHub notifications bot (Jan 24 2025 at 15:55):

dicej commented on PR #10083:

@alexcrichton I've "rebooted" this PR from scratch, this time only pulling in the minimum code needed for the lift.wast test. It's much smaller now.

As a consequence, most of your review comments no longer apply, but I'll address them in future PRs. Sorry for the churn; I'll try to keep future PRs more focused.

Wasmtime GitHub notifications bot (Jan 24 2025 at 15:59):

dicej requested alexcrichton for a review on PR #10083.

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:26):

alexcrichton created PR review comment:

Ah I see, sorry I missed the not(...)!

I'll take a closer look in a future PR to see how it fits together :+1:

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:29):

alexcrichton created PR review comment:

No worries of course! I myself basically never write comments as I write code itself and then I have to force myself to go back afterwards and comment things, which I often forget to do. I just wanted to highlight this as something I think will be important to do as pieces incrementally land as opposed to only once at the end

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:31):

alexcrichton created PR review comment:

How about something like rep: WaitableIndex? Basically using a type here to indicate more clearly what it's pointing to. My history with resources makes me think that "rep" comes from guests themselves and needs to be preserved and handed back to the guest but I don't think that's what's going on here, so this isn't so much a "representation" internally but moreso an index somewhere else, which is where I think a custom newtype might help to distinguish

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:31):

alexcrichton submitted PR review:

Looks good to me, thanks!

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:44):

dicej updated PR #10083.

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:44):

dicej has enabled auto merge for PR #10083.

Wasmtime GitHub notifications bot (Jan 24 2025 at 16:45):

dicej updated PR #10083.

Wasmtime GitHub notifications bot (Jan 24 2025 at 17:21):

dicej merged PR #10083.

Last updated: Feb 28 2025 at 01:30 UTC