Stream: git-wasmtime

Topic: wasmtime / PR #6637 handle interface functions correctly ...

Wasmtime GitHub notifications bot (Jun 23 2023 at 22:26):

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

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 23 2023 at 22:26):

dicej requested jameysharp for a review on PR #6637.

Wasmtime GitHub notifications bot (Jun 23 2023 at 22:26):

dicej requested wasmtime-core-reviewers for a review on PR #6637.

Wasmtime GitHub notifications bot (Jun 23 2023 at 23:40):

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

Wasmtime GitHub notifications bot (Jun 23 2023 at 23:40):

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

Wasmtime GitHub notifications bot (Jun 23 2023 at 23:40):

jameysharp created PR review comment:

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

Wasmtime GitHub notifications bot (Jun 23 2023 at 23:57):

dicej created PR review comment:

Yup, good catch.

Wasmtime GitHub notifications bot (Jun 23 2023 at 23:58):

dicej updated PR #6637.

Wasmtime GitHub notifications bot (Jun 24 2023 at 00:03):

jameysharp requested alexcrichton for a review on PR #6637.

Wasmtime GitHub notifications bot (Jun 24 2023 at 01:00):

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 something Vec 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>

Wasmtime GitHub notifications bot (Jun 24 2023 at 02:32):

dicej updated PR #6637.

Wasmtime GitHub notifications bot (Jul 05 2023 at 23:35):

alexcrichton merged PR #6637.

Last updated: Oct 23 2024 at 20:03 UTC