rvolosatovs edited PR #7688.
rvolosatovs updated PR #7688.
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 toResourceType::host::<T>()
is no longer valid)~Add support for
Resource<T>
->ResourceAny
conversion to allow invoking component functions without bindings generated at compile-time.
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 intotype mismatch for own, possibly due to mixing distinct composite types
I will reach-out offline
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs has marked PR #7688 as ready for review.
rvolosatovs requested fitzgen for a review on PR #7688.
rvolosatovs requested wasmtime-core-reviewers for a review on PR #7688.
rvolosatovs commented on PR #7688:
@alexcrichton , as discussed, I introduced a
PathIndex
mappingusize
s toVec<usize>
import paths.
I did not attempt to work on optimizing this, but rather focused on getting something simple over the line.
I added thePathIndex
to theNameMap
value, since in order to use these indexes a reverse mapping fromVec<usize>
import path toPathIndex
is required
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I was thinking that the
PathIndex
here would only be returned forfn resource
since it's not really applicable to any of the other types of imports. That'd also enable pushingPathIndex
into theDefinition::Resource
variant instead of having it for all variants.
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.
alexcrichton created PR review comment:
I think this doc comment will need an update
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs requested alexcrichton for a review on PR #7688.
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
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 theinsert
helper. I wouldn't worry too much about the interaction of bumping the resource counter if an error is returned.
alexcrichton created PR review comment:
Could this still use
self.insert
to avoid duplicating the implementation?
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)
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 theinsert
helper. I wouldn't worry too much about the interaction of bumping the resource counter if an error is returned.
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.
rvolosatovs updated PR #7688.
rvolosatovs requested wasmtime-compiler-reviewers for a review on PR #7688.
rvolosatovs submitted PR review.
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
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Small enough that I did it here
Any progress? This is blocker for us
rvolosatovs updated PR #7688.
rvolosatovs updated PR #7688.
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 namedLinker
in scopewarning:
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 documentwasmtime
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
rvolosatovs updated PR #7688.
alexcrichton merged PR #7688.
Last updated: Dec 23 2024 at 13:07 UTC