Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spin/precompilation): add support for precompiling wasm components #16

Closed
wants to merge 3 commits into from

Conversation

radu-matei
Copy link
Member

@radu-matei radu-matei commented Feb 21, 2024

This commit adds support for precompiling wasm components.

@radu-matei
Copy link
Member Author

This still needs fermyon/spin#2284.

@radu-matei radu-matei changed the title feat(spin/assets): support loading static assets feat(spin/precompilation): add support for precompiling wasm components Feb 21, 2024
@radu-matei
Copy link
Member Author

radu-matei commented Feb 22, 2024

This needs Spin with the following patch and the this runwasi patch (containerd/runwasi@main...radu-matei:runwasi:fix/precompilation-cache):

diff --git a/crates/trigger/src/cli.rs b/crates/trigger/src/cli.rs
index 008d87b3..df3311a8 100644
--- a/crates/trigger/src/cli.rs
+++ b/crates/trigger/src/cli.rs
@@ -170,7 +170,7 @@ where
             LLmOptions { use_gpu: true },
         );
 
-        let loader = TriggerLoader::new(working_dir, self.allow_transient_write);
+        let loader = TriggerLoader::new(working_dir, self.allow_transient_write, false);
         let executor = self.build_executor(loader, locked_url, init_data).await?;
 
         let run_fut = executor.run(self.run_config);
diff --git a/crates/trigger/src/loader.rs b/crates/trigger/src/loader.rs
index 0c25d72a..778b021e 100644
--- a/crates/trigger/src/loader.rs
+++ b/crates/trigger/src/loader.rs
@@ -16,13 +16,15 @@ use spin_common::{ui::quoted_path, url::parse_file_url};
 pub struct TriggerLoader {
     working_dir: PathBuf,
     allow_transient_write: bool,
+    aot: bool,
 }
 
 impl TriggerLoader {
-    pub fn new(working_dir: impl Into<PathBuf>, allow_transient_write: bool) -> Self {
+    pub fn new(working_dir: impl Into<PathBuf>, allow_transient_write: bool, aot: bool) -> Self {
         Self {
             working_dir: working_dir.into(),
             allow_transient_write,
+            aot,
         }
     }
 }
@@ -55,9 +57,17 @@ impl Loader for TriggerLoader {
                 path.display()
             )
         })?;
-        let component = spin_componentize::componentize_if_necessary(&bytes)?;
-        spin_core::Component::new(engine, component.as_ref())
-            .with_context(|| format!("loading module {}", quoted_path(&path)))
+        if self.aot {
+            unsafe {
+                println!("attempting to load cwasm");
+                spin_core::Component::deserialize(engine, &bytes)
+                    .with_context(|| format!("loading module {}", quoted_path(&path)))
+            }
+        } else {
+            let component = spin_componentize::componentize_if_necessary(&bytes)?;
+            spin_core::Component::new(engine, component.as_ref())
+                .with_context(|| format!("loading module {}", quoted_path(&path)))
+        }
     }
 
     async fn load_module(
diff --git a/tests/testing-framework/src/test_environment.rs b/tests/testing-framework/src/test_environment.rs
index c934ae79..da21fb2e 100644
--- a/tests/testing-framework/src/test_environment.rs
+++ b/tests/testing-framework/src/test_environment.rs
@@ -218,7 +218,8 @@ impl TestEnvironmentConfig<InProcessSpin> {
                         let json = locked_app.to_json()?;
                         std::fs::write(env.path().join("locked.json"), json)?;
 
-                        let loader = TriggerLoader::new(env.path().join(".working_dir"), false);
+                        let loader =
+                            TriggerLoader::new(env.path().join(".working_dir"), false, false);
                         let mut builder = TriggerExecutorBuilder::<HttpTrigger>::new(loader);
                         // TODO(rylev): see if we can reuse the builder from spin_trigger instead of duplicating it here
                         builder.hooks(spin_trigger::network::Network);

@jsturtevant
Copy link
Contributor

and the this runwasi patch (containerd/[email protected]:runwasi:fix/precompilation-cache):

I've pushed some similar changes that include the label on the layer plus a couple other fixes to containerd/runwasi#492

@kate-goldenring kate-goldenring marked this pull request as ready for review February 23, 2024 20:44
@kate-goldenring kate-goldenring marked this pull request as draft February 23, 2024 20:45
@kate-goldenring
Copy link
Collaborator

Adding a check to precompile to detect whether it is precompiled already and skipping if it is resolves the issue: containerd/runwasi#492 (comment)

We should still add a fix to runwasi but is a patch option

@kate-goldenring
Copy link
Collaborator

kate-goldenring commented Feb 26, 2024

@radu-matei @vdice I added the precompilation check 0bc1f47, so shim should be working well now. The check doesn't add significant latency -- it checks for the file type flag on the bytes and takes only 30.739µs to execute.

@kate-goldenring kate-goldenring force-pushed the feat/precompilation branch 2 times, most recently from 0bc1f47 to 2639c1c Compare February 28, 2024 20:01
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kate-goldenring
Copy link
Collaborator

All dependencies are now pointing to tagged releases or references off main. This should be good to go.

I tested it with:

  • Spin app with multiple components
  • 200 MB JS app
  • App updated with additional components

In all cases, subsequent ctr runs are faster and use previously compiled components rather than recompiling.

@radu-matei
Copy link
Member Author

Thank you so much for your work here, @kate-goldenring!

My original PR, so can't formally approve, but LGTM.

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

Outside of a nit, I say send it.


fn supported_layers_types() -> &'static [&'static str] {
&[
"application/vnd.wasm.content.layer.v1+wasm",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #32, consider consts please.

@kate-goldenring
Copy link
Collaborator

@radu-matei I cannot push to this anymore because this branch does not exist as your fork was recreated

@radu-matei radu-matei closed this Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants