Stream: git-wasmtime

Topic: wasmtime / PR #7688 feat: implement `Resource<T>` -> `Res...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 15:03):

rvolosatovs edited PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 16:49):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 16:51):

rvolosatovs edited PR #7688:

Closes https://github.com/bytecodealliance/wasmtime/issues/7676
Follow-up on #7680

~This feature requires changes in the resource table to allow specifying the ResourceType associated with the resource stored (since assumption that it's always equal to ResourceType::host::<T>() is no longer valid)~

Add support for Resource<T> -> ResourceAny conversion to allow invoking component functions without bindings generated at compile-time.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 16:55):

rvolosatovs commented on PR #7688:

Small update: I found a way to obtain the TypeResourceTableIndex from the instance, which simplifies getting the destructor and flags, however I keep running into

    type mismatch for own, possibly due to mixing distinct composite types

I will reach-out offline

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 20:25):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 20:28):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 20:29):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 15 2023 at 21:20):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 13:59):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:01):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:05):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:06):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:24):

rvolosatovs has marked PR #7688 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:24):

rvolosatovs requested fitzgen for a review on PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:24):

rvolosatovs requested wasmtime-core-reviewers for a review on PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2023 at 14:27):

rvolosatovs commented on PR #7688:

@alexcrichton , as discussed, I introduced a PathIndex mapping usizes to Vec<usize> import paths.
I did not attempt to work on optimizing this, but rather focused on getting something simple over the line.
I added the PathIndex to the NameMap value, since in order to use these indexes a reverse mapping from Vec<usize> import path to PathIndex is required

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2023 at 18:18):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2023 at 18:18):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2023 at 18:18):

alexcrichton created PR review comment:

I was thinking that the PathIndex here would only be returned for fn resource since it's not really applicable to any of the other types of imports. That'd also enable pushing PathIndex into the Definition::Resource variant instead of having it for all variants.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2023 at 18:18):

alexcrichton created PR review comment:

I'm a little confused about the purpose of this field, it seems like this is written/pushed to and the length is read, but otherewise the contents of this array aren't ever read. I was expecting just an integer for this field which increments on each defined resource within the linker, which tracks with how this is used I think.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2023 at 18:18):

alexcrichton created PR review comment:

I think this doc comment will need an update

view this post on Zulip Wasmtime GitHub notifications bot (Dec 20 2023 at 12:24):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 20 2023 at 12:24):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 20 2023 at 12:27):

rvolosatovs requested alexcrichton for a review on PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 21 2023 at 13:34):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 21 2023 at 13:36):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 01:03):

alexcrichton submitted PR review:

Thanks again for this! A few comments which I'm happy to defer to later, but for the implementation of LinkerInstance::resource I think it'd be good to continue to rely on the insert helper. I wouldn't worry too much about the interaction of bumping the resource counter if an error is returned.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 01:03):

alexcrichton created PR review comment:

Could this still use self.insert to avoid duplicating the implementation?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 01:03):

alexcrichton created PR review comment:

For a future PR, I think it'd be good to expand this documentation here to indicate which method this comes from and how it's expected to be used (and/or just link to the docs on the conversion added here)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 01:03):

alexcrichton submitted PR review:

Thanks again for this! A few comments which I'm happy to defer to later, but for the implementation of LinkerInstance::resource I think it'd be good to continue to rely on the insert helper. I wouldn't worry too much about the interaction of bumping the resource counter if an error is returned.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 01:03):

alexcrichton created PR review comment:

For a future PR, I think this should be able to representable as Arc<PrimaryMap<ResourceImportIndex, Option<RuntimeImportIndex>>> which I think would help with some of the type handling. No functional difference but just a slightly different organization.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:44):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:44):

rvolosatovs requested wasmtime-compiler-reviewers for a review on PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:46):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:46):

rvolosatovs created PR review comment:

This was a small enough change, that I just did it in this PR, but it did require a small addition to PrimaryMap impl to avoid iteration for constructing it

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:47):

rvolosatovs submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:47):

rvolosatovs created PR review comment:

Small enough that I did it here

view this post on Zulip Wasmtime GitHub notifications bot (Jan 03 2024 at 20:25):

sehz commented on PR #7688:

Any progress? This is blocker for us

view this post on Zulip Wasmtime GitHub notifications bot (Jan 03 2024 at 20:52):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 03 2024 at 20:54):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 03 2024 at 20:57):

rvolosatovs commented on PR #7688:

Any progress? This is blocker for us

Hey @sehz,

There was a doc CI failure:

error: unresolved link to Linker::resource
--> crates/wasmtime/src/component/resources.rs:519:60
|
519 | /// idx is the [ResourceImportIndex] returned by [Linker::resource].
| ^^^^^^^^^^^^^^^^ no item named Linker in scope

warning: cranelift-jit (lib doc) generated 1 warning (1 duplicate)
Documenting wasmtime-cranelift-shared v17.0.0 (/home/runner/work/wasmtime/wasmtime/crates/cranelift-shared)
warning: wasmtime (lib doc) generated 1 warning (1 duplicate)
error: could not document wasmtime

Which, I believe, is now fixed. I've rebased and added prtest:full(not sure if a scoped option exists) to run all CI checks on this PR, once that succeeds it should be ready for merge.

Please note that I'm generally out of office until Monday, the 8th of January, but I may be able to address small things like this doc CI failure in the meantime

view this post on Zulip Wasmtime GitHub notifications bot (Jan 03 2024 at 21:42):

rvolosatovs updated PR #7688.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 03 2024 at 23:46):

alexcrichton merged PR #7688.

Last updated: Nov 22 2024 at 16:03 UTC