dicej opened PR #6637 from dicej:functions_in_instances
to bytecodealliance:main
:
Many months ago, I implemented
func_new
, but only supporting top-level function imports. If you tried to link a host function under an imported interface, it would mistakenly treat it as a top-level function and either error out if it couldn't find a corresponding type definition in the passed&Component
; or, if it found a top-level function that happened to have the same name, it would use that type (which would coincidentally work if the type happens to match, but lead to a runtime error later on otherwise).This fixes the issue by looking up the correct component instance when necessary and getting the type from there.
Note I've made no effort to optimize for performance here. Happy to revisit that if there's a need.
<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
dicej requested jameysharp for a review on PR #6637.
dicej requested wasmtime-core-reviewers for a review on PR #6637.
jameysharp submitted PR review:
I've gone ahead and reviewed this because I want to better understand the component-model implementation, but I want to leave final approval to @alexcrichton.
I think this does what you said it should do :+1:.
That said, I wonder if you can assign sequential integers to instances, somewhat like
Strings::intern
does for names. Then aLinker
would have a singleVec<NameMap>
indexed by this sequential instance ID;Definition::Instance
would store this index instead of the full map; and aLinkerInstance
would store only its instance index instead of a full path.If that's feasible, I think it's simpler than the reference-counted singly-linked-list. As a nice bonus, it's also asymptotically faster since
func_new
can find the right map in constant time instead of linear time.
jameysharp submitted PR review:
I've gone ahead and reviewed this because I want to better understand the component-model implementation, but I want to leave final approval to @alexcrichton.
I think this does what you said it should do :+1:.
That said, I wonder if you can assign sequential integers to instances, somewhat like
Strings::intern
does for names. Then aLinker
would have a singleVec<NameMap>
indexed by this sequential instance ID;Definition::Instance
would store this index instead of the full map; and aLinkerInstance
would store only its instance index instead of a full path.If that's feasible, I think it's simpler than the reference-counted singly-linked-list. As a nice bonus, it's also asymptotically faster since
func_new
can find the right map in constant time instead of linear time.
jameysharp created PR review comment:
Should these uses of
name
all be the string version of the name, rather than the interned usize?
dicej created PR review comment:
Yup, good catch.
dicej updated PR #6637.
jameysharp requested alexcrichton for a review on PR #6637.
alexcrichton submitted PR review:
Seems reasonable to me :+1:
I'd be slightly averse though to a custom
Rc
-based linked list, so I'd lean more towards somethingVec
like such as this (feel free to take it or leave it though)<details>
diff --git a/crates/wasmtime/src/component/linker.rs b/crates/wasmtime/src/component/linker.rs index 6665eccf7..db8daa89d 100644 --- a/crates/wasmtime/src/component/linker.rs +++ b/crates/wasmtime/src/component/linker.rs @@ -10,7 +10,6 @@ use std::future::Future; use std::marker; use std::ops::Deref; use std::pin::Pin; -use std::rc::Rc; use std::sync::Arc; use wasmtime_environ::component::TypeDef; use wasmtime_environ::PrimaryMap; @@ -25,6 +24,7 @@ pub struct Linker<T> { engine: Engine, strings: Strings, map: NameMap, + path: Vec<usize>, allow_shadowing: bool, _marker: marker::PhantomData<fn() -> T>, } @@ -35,25 +35,15 @@ pub struct Strings { strings: Vec<Arc<str>>, } -struct PathCell { - name: usize, - next: Path, -} - -#[derive(Clone)] -enum Path { - Nil, - Cell(Rc<PathCell>), -} - /// Structure representing an "instance" being defined within a linker. /// /// Instances do not need to be actual [`Instance`]s and instead are defined by /// a "bag of named items", so each [`LinkerInstance`] can further define items /// internally. pub struct LinkerInstance<'a, T> { - engine: Engine, - path: Path, + engine: &'a Engine, + path: &'a mut Vec<usize>, + path_len: usize, strings: &'a mut Strings, map: &'a mut NameMap, allow_shadowing: bool, @@ -78,6 +68,7 @@ impl<T> Linker<T> { strings: Strings::default(), map: NameMap::default(), allow_shadowing: false, + path: Vec::new(), _marker: marker::PhantomData, } } @@ -100,8 +91,9 @@ impl<T> Linker<T> { /// the root namespace. pub fn root(&mut self) -> LinkerInstance<'_, T> { LinkerInstance { - engine: self.engine.clone(), - path: Path::Nil, + engine: &self.engine, + path: &mut self.path, + path_len: 0, strings: &mut self.strings, map: &mut self.map, allow_shadowing: self.allow_shadowing, @@ -246,8 +238,9 @@ impl<T> Linker<T> { impl<T> LinkerInstance<'_, T> { fn as_mut(&mut self) -> LinkerInstance<'_, T> { LinkerInstance { - engine: self.engine.clone(), - path: self.path.clone(), + engine: self.engine, + path: self.path, + path_len: self.path_len, strings: self.strings, map: self.map, allow_shadowing: self.allow_shadowing, @@ -327,13 +320,6 @@ impl<T> LinkerInstance<'_, T> { name: &str, func: F, ) -> Result<()> { - let mut names = Vec::new(); - let mut path = &self.path; - while let Path::Cell(cell) = path { - names.push(cell.name); - path = &cell.next; - } - let mut map = &component .env_component() .import_types @@ -341,7 +327,7 @@ impl<T> LinkerInstance<'_, T> { .map(|(k, v)| (k.clone(), *v)) .collect::<IndexMap<_, _>>(); - while let Some(name) = names.pop() { + for name in self.path.iter().copied().take(self.path_len) { let name = self.strings.strings[name].deref(); if let Some(ty) = map.get(name) { if let TypeDef::ComponentInstance(index) = ty { @@ -409,10 +395,8 @@ impl<T> LinkerInstance<'_, T> { Definition::Instance(map) => map, _ => unreachable!(), }; - self.path = Path::Cell(Rc::new(PathCell { - name, - next: self.path, - })); + self.path.truncate(self.path_len); + self.path.push(name); Ok(self) }
</details>
dicej updated PR #6637.
alexcrichton merged PR #6637.
Last updated: Nov 22 2024 at 16:03 UTC