handle interface functions correctly in component::Linker::func_new

[dicej]

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.

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

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

• peterhuene: wasmtime:api

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

Learn more.

[jameysharp]

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 👍.

That said, I wonder if you can assign sequential integers to instances, somewhat like Strings::intern does for names. Then a Linker would have a single Vec<NameMap> indexed by this sequential instance ID; Definition::Instance would store this index instead of the full map; and a LinkerInstance 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]

Should these uses of name all be the string version of the name, rather than the interned usize?

[dicej]

Yup, good catch.

[alexcrichton]

Seems reasonable to me 👍

I'd be slightly averse though to a custom Rc-based linked list, so I'd lean more towards something Vec like such as this (feel free to take it or leave it though)

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)
     }
 

[dicej]

Seems reasonable to me +1

I'd be slightly averse though to a custom Rc-based linked list, so I'd lean more towards something Vec like such as this (feel free to take it or leave it though)

Sounds reasonable. I went for the linked list because I assumed there could be more than one sibling LinkerInstance alive at a a given moment and thus couldn't share the same Vec. But now I see that's not possible since it would create aliasing &muts.

[dicej]

I like @jameysharp's proposal, but Alex's won because it came with a patch :)

[alexcrichton]

Oh dear I apologize, I saw comments from Jamey and updates from Joel afterwards and I naively didn't actually read the comments and just assumed that the updates handled them! After reading Jamey's comments in more detail I would agree with his suggestion and I think that'd be an excellent refactoring to have. I poked at it for a moment but it was going to get somewhat nontrivial with the matching implementation where the list-of-instances needs to be passed around as context. Nothing insurmountable of course but it might be a bit of a nontrivial refactoring.

I do think though that we won't be able to go to the constant-time implementation of this though because given the current API we always need to walk the path to the current instance in the provided &Component to figure out the type.

@jameysharp how would you feel about deferring your suggestion to an issue? (and/or how do you feel about the current state of the PR as well)

[jameysharp]

I'm happy to defer to your judgement, both of you. I think this isn't performance-critical, right? So making it correct is the only important thing right now, and my impression is that this PR does that as-is.

[alexcrichton]

Ok sounds good 👍. This isn't performance critical yeah so I'll merge this for now and we can follow-up later as necessary