Skip to content

Conversation

@theduke
Copy link
Contributor

@theduke theduke commented Nov 18, 2025

  • feat: Progress reporting and abort for module compilation

    Add a new Module::new_with_progress function, which will call a progress
    callback with progress information.

    The callback may also return an error , which will abort compilation.

  • feat(wasix): Module load/compile progress reporting

    Implement progress reporting for module loading.

    • Extends ModuleCache with a load_with_progress() method, which receives
      a progress callback

    • Clean up and unify the module loading methods on the Runtime trait.
      Replace old methods with a new, general fn resolve_module(), which
      replaces all the old methods.
      This new method also takes an optional progress callback.

  • feat(cli): run command: Show compilation progress

    Show compilation progress using the newly added progress tracking.

NOTE: needs a test or two.
Also can't currently test the CLI progress reporting because of the Cloudflare
outage, will do so later.

Add a new Module::new_with_progress function, which will call a progress
callback with progress information.

The callback may also return an error , which will abort compilation.
Implement progress reporting for module loading.

* Extends ModuleCache with a load_with_progress() method, which receives
  a progress callback

* Clean up and unify the module loading methods on the Runtime trait.
  Replace old methods with a new, general fn resolve_module(), which
  replaces all the old methods.
  This new method also takes an optional progress callback.
Show compilation progress using the newly added progress tracking.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive progress reporting capabilities for WebAssembly module loading and compilation. It introduces a new callback-based mechanism that allows consumers to track compilation progress, receive detailed phase information, and abort compilation if needed.

Key changes:

  • New Module::new_with_progress() API for compilation progress tracking with abort support
  • Unified module loading through Runtime::resolve_module() replacing multiple deprecated methods
  • Progress reporting infrastructure with CompilationProgress and ModuleLoadProgress types

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 17 comments.

Show a summary per file
File Description
lib/types/src/progress.rs Defines core progress types (CompilationProgress, CompilationProgressCallback, UserAbort)
lib/types/src/error.rs Adds UserAbort variant to CompileError for user-initiated aborts
lib/wasix/src/runtime/module_cache/progress.rs Defines module load progress types for cache operations and downloads
lib/wasix/src/runtime/module_cache/types.rs Extends ModuleCache trait with load_with_progress() default implementation
lib/wasix/src/runtime/mod.rs Introduces ModuleInput enum and resolve_module() method, deprecates old loading methods
lib/api/src/entities/module/mod.rs Adds public new_with_progress() method to Module API
lib/api/src/entities/module/inner.rs Dispatches progress callbacks to backend-specific implementations
lib/api/src/backend/sys/entities/module.rs Implements progress reporting for sys backend with compiler feature support
lib/api/src/backend/{wasmi,wamr,v8,js,jsc}/entities/module.rs Stub implementations for other backends (progress ignored)
lib/compiler/src/engine/inner.rs Adds compile_with_progress() method to Engine
lib/compiler/src/engine/artifact.rs Threads progress callback through artifact creation
lib/compiler/src/compiler.rs Extends Compiler trait with progress callback parameter
lib/compiler-llvm/src/compiler.rs Implements progress tracking with atomic counters for parallel compilation
lib/compiler-cranelift/src/compiler.rs Implements progress tracking for both parallel and sequential compilation
lib/wasix/src/runners/wcgi/runner.rs Migrates to new resolve_module_sync() API
lib/wasix/src/bin_factory/exec.rs Migrates to new resolve_module() API
lib/cli/src/commands/run/runtime.rs Implements progress bar UI for compilation using new progress reporting
lib/wasix/src/bin_factory/binary_package.rs Adds atom_ref() helper method and fixes trailing whitespace

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +40 to +51
#[derive(Clone, Debug)]
pub struct ProgressError {
message: String,
}

impl ProgressError {
pub fn new(message: impl Into<String>) -> Self {
Self {
message: message.into(),
}
}
}
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Missing documentation: The ProgressError struct and its methods lack documentation. Consider adding doc comments explaining its purpose and how it differs from other error types.

Copilot uses AI. Check for mistakes.
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Im in favor of having something like this functionality wise, but really against how it exposes things.

The method should be not inside of the module, but the engine. More specifically the Sys engine.

@theduke
Copy link
Contributor Author

theduke commented Nov 18, 2025

Why though?

We should have a generic progress reporting system that isn't tied to any particular backend.

Copy link
Contributor

@marxin marxin left a comment

Choose a reason for hiding this comment

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

Generally speaking, I really like the idea and I am also fine with the suggested callback mechanism.

Have 2 questions/comments about the CLI:

  • after the change, wasmer run --llvm python stopped producing the progress messages: ⠲ Compiling WebAssembly module for command... and so on.
  • can we provide proper progress bar for the compilation progress based on the new API (indicatif)?

@theduke theduke force-pushed the compiler-progress branch 2 times, most recently from 44b639c to 4c33f09 Compare November 18, 2025 21:58
@theduke
Copy link
Contributor Author

theduke commented Nov 18, 2025

@marxin

Changes since the review:

  • Added singlepass
  • Added trampolines to counters
  • Added abort and progress tests for llvm/cranelift/singlepass in lib/api/tests/module_compilation_progress.rs
  • Moved method to new extension trait ProgressEngineExt (as per discussion with Syrus)
  • Improved the progress bar:
image
  • Other small tweaks, docs, etc

@theduke theduke requested a review from marxin November 18, 2025 22:08
@theduke
Copy link
Contributor Author

theduke commented Nov 18, 2025

@syrusakbary

I instead moved the method to a new ProgressEngineExt::new_module_with_progress(engine, ..).

Why not on NativeEngineExt?
To have a uniform interface for all backends, and not require cumbersome #[cfg(...)] everywhere.

) -> Result<crate::Module, wasmer_types::CompileError>;
}

impl ProgressEngineExt for crate::Engine {
Copy link
Member

@syrusakbary syrusakbary Nov 18, 2025

Choose a reason for hiding this comment

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

This impl and work should not live here. Should live only on the sys implementation that already exists (NativeEngineExt):

https://github.com/wasmerio/wasmer/blob/main/lib/api/src/backend/sys/entities/engine.rs#L53

unsafe { Self::from_binary_unchecked(_engine, binary) }
}

pub(crate) fn from_binary_with_progress(
Copy link
Member

Choose a reason for hiding this comment

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

This function should only be needed on the sys crate. Remove this

unsafe { Self::from_binary_unchecked(_engine, binary) }
}

pub(crate) fn from_binary_with_progress(
Copy link
Member

Choose a reason for hiding this comment

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

This function should only be needed on the sys crate. Remove this

unsafe { Self::from_binary_unchecked(engine, binary) }
}

pub(crate) fn from_binary_with_progress(
Copy link
Member

Choose a reason for hiding this comment

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

This function should only be needed on the sys crate. Remove this

unsafe { Self::from_binary_unchecked(_engine, binary) }
}

pub(crate) fn from_binary_with_progress(
Copy link
Member

Choose a reason for hiding this comment

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

This function should only be needed on the sys crate. Remove this

unsafe { Self::from_binary_unchecked(_engine, binary) }
}

pub(crate) fn from_binary_with_progress(
Copy link
Member

Choose a reason for hiding this comment

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

This function should only be needed on the sys crate. Remove this

Comment on lines +46 to +59
#[inline]
pub fn new_with_progress(
engine: &impl AsEngineRef,
bytes: impl AsRef<[u8]>,
callback: CompilationProgressCallback,
) -> Result<Self, CompileError> {
#[cfg(feature = "wat")]
let bytes = wat::parse_bytes(bytes.as_ref()).map_err(|e| {
CompileError::Wasm(WasmError::Generic(format!(
"Error when converting wat: {e}",
)))
})?;
Self::from_binary_with_progress(engine, bytes.as_ref(), callback)
}
Copy link
Member

Choose a reason for hiding this comment

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

This should not live here

}

#[inline]
pub fn from_binary_with_progress(
Copy link
Member

Choose a reason for hiding this comment

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

Delete me

Copy link
Contributor

@marxin marxin left a comment

Choose a reason for hiding this comment

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

Have a couple of nit comments. I've just tested the branch locally and I really like the wasmer run progress bar report - good job!

pub fn short_hash(&self) -> String {
use std::fmt::Write as _;

let bytes = match self {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using self.bytes()[..4]?

Self::Sha256(bytes) => &bytes[0..4],
};

let mut s = String::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

hex crate would be your friend here


/// Error returned when the user requests to abort an expensive computation.
#[derive(Clone, Debug)]
pub struct UserAbort {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about making it use thiserror as I see you implement Display trait anyway.

@marxin
Copy link
Contributor

marxin commented Nov 20, 2025

Noticed the progress bar is not displayed if I compile php:

cargo r --features=singlepass,llvm -p wasmer-cli run --disable-cache --llvm -- php --version

If I compare the back-trace leading to load_module with running python, then it's the following difference:

❯ diff -u /tmp/good.txt /tmp/bad.txt
--- /tmp/good.txt	2025-11-20 10:56:21.298739172 +0100
+++ /tmp/bad.txt	2025-11-20 10:56:08.681375318 +0100
@@ -1,6 +1,6 @@
-⠁ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ Compiling python[lib/wasix/src/runtime/mod.rs:203:9] ("resolve_module", on_progress.is_some()) = (
+[lib/wasix/src/runtime/mod.rs:203:9] ("resolve_module", on_progress.is_some()) = (
     "resolve_module",
-    true,
+    false,
 )

 thread 'tokio-runtime-worker' panicked at lib/wasix/src/runtime/mod.rs:204:9:
@@ -14,34 +14,30 @@
              at /rustc/29483883eed69d5fb4db01964cdf2af4d86e9cb2/library/core/src/panicking.rs:145:5
    3: wasmer_wasix::runtime::Runtime::resolve_module
              at ./lib/wasix/src/runtime/mod.rs:204:9
-   4: <wasmer_cli::commands::run::runtime::MonitoringRuntime<R> as wasmer_wasix::runtime::Runtime>::resolve_module::{{closure}}
-             at ./lib/cli/src/commands/run/runtime.rs:180:18
-   5: <core::pin::Pin<P> as core::future::future::Future>::poll
-             at /home/marxin/.rustup/toolchains/1.89-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/future.rs:124:9
-   6: wasmer_wasix::bin_factory::exec::spawn_exec::{{closure}}::{{closure}}
-             at ./lib/wasix/src/bin_factory/exec.rs:38:60
-   7: wasmer_wasix::bin_factory::exec::spawn_exec::{{closure}}
+   4: wasmer_wasix::bin_factory::exec::spawn_exec::{{closure}}::{{closure}}
+             at ./lib/wasix/src/bin_factory/exec.rs:38:26
+   5: wasmer_wasix::bin_factory::exec::spawn_exec::{{closure}}
              at ./lib/wasix/src/bin_factory/exec.rs:27:1
-   8: wasmer_wasix::runners::wasi::WasiRunner::run_command::{{closure}}
+   6: wasmer_wasix::runners::wasi::WasiRunner::run_command::{{closure}}
              at ./lib/wasix/src/runners/wasi.rs:502:26
-   9: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
+   7: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
              at /home/marxin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tracing-0.1.41/src/instrument.rs:321:15
-  10: <D as wasmer_wasix::runtime::task_manager::VirtualTaskManagerExt>::spawn_and_block_on::{{closure}}
+   8: <D as wasmer_wasix::runtime::task_manager::VirtualTaskManagerExt>::spawn_and_block_on::{{closure}}
              at ./lib/wasix/src/runtime/task_manager/mod.rs:480:28
-  11: <core::pin::Pin<P> as core::future::future::Future>::poll
+   9: <core::pin::Pin<P> as core::future::future::Future>::poll
              at /home/marxin/.rustup/toolchains/1.89-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/future.rs:124:9
-  12: <wasmer_wasix::runtime::task_manager::tokio::TokioTaskManager as wasmer_wasix::runtime::task_manager::VirtualTaskManager>::task_shared::{{closure}}
+  10: <wasmer_wasix::runtime::task_manager::tokio::TokioTaskManager as wasmer_wasix::runtime::task_manager::VirtualTaskManager>::task_shared::{{closure}}
              at ./lib/wasix/src/runtime/task_manager/tokio.rs:136:17
-  13: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
+  11: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
              at /home/marxin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.48.0/src/runtime/task/core.rs:365:24
-  14: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
+  12: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
              at /home/marxin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.48.0/src/loom/std/unsafe_cell.rs:16:9
-  15: tokio::runtime::task::core::Core<T,S>::poll
+  13: tokio::runtime::task::core::Core<T,S>::poll
              at /home/marxin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.48.0/src/runtime/task/core.rs:354:30
-  16: tokio::runtime::task::harness::poll_future::{{closure}}
+  14: tokio::runtime::task::harness::poll_future::{{closure}}
              at /home/marxin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.48.0/src/runtime/task/harness.rs:535:30
-  17: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
+  15: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
              at /home/marxin/.rustup/toolchains/1.89-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:272:9
-  18: std::panicking::catch_unwind::do_call
+  16: std::panicking::catch_unwind::do_call
              at /home/marxin/.rustup/toolchains/1.89-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:589:40

@marxin
Copy link
Contributor

marxin commented Nov 20, 2025

CC: @Arshia001

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Just to be clear: This PR can't move forward unless it gets further review and approval for the API changes from my side

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.

4 participants