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.

Last updated: Jan 24 2025 at 00:11 UTC