feat: compile modules in parallel #80
Conversation
There was a problem hiding this comment.
Pull Request Overview
The purpose of this PR is to refactor the module linking and store usage to compile wasm modules in parallel, replacing the old GuestState with a new Apis type. Key changes include updating the API linking functions, modifying store creation and module iteration logic for parallel compilation, and updating admin/help command handling to use the new types.
- Updating API link signatures across several files (sys.rs, sync_request.rs, log.rs, kv_store.rs)
- Changing the Wasm module store type from GuestState to Apis and compiling modules in parallel using rayon
- Adjusting admin and help command handling to work with the new module-store structure
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wasm/apis/sys.rs | Updated API linking signature to use Apis instead of GuestState |
| src/wasm/apis/sync_request.rs | Updated API linking signature to use Apis |
| src/wasm/apis/mod.rs | Removed GuestState and updated linking to use Apis |
| src/wasm/apis/log.rs | Updated API linking signature to use Apis |
| src/wasm/apis/kv_store.rs | Updated API linking signature to use Apis |
| src/wasm.rs | Adjusted store creation and parallel module compilation logic |
| src/lib.rs | Modified admin and help command handling to work with the new module/store structure |
| Cargo.toml | Added rayon dependency for parallel processing |
| // Create APIs directly for the module | ||
| let apis = match Apis::new(name.clone(), db.clone()) { | ||
| Ok(apis) => apis, | ||
| Err(_) => return None, |
There was a problem hiding this comment.
In the parallel module compilation block, errors from Apis::new are silently swallowed (returning None) which can make debugging difficult. Consider logging the error details before returning None.
| Err(_) => return None, | |
| Err(e) => { | |
| tracing::error!("Failed to create APIs for module '{}': {}", name, e); | |
| return None; | |
| }, |
| if apis::Apis::link(&mut linker).is_err() { | ||
| return None; |
There was a problem hiding this comment.
The linking failure is handled by immediately returning None without logging the error. It is advisable to log error information to aid in troubleshooting module linking issues.
| if apis::Apis::link(&mut linker).is_err() { | |
| return None; | |
| match apis::Apis::link(&mut linker) { | |
| Ok(_) => {} | |
| Err(e) => { | |
| tracing::error!("Failed to link APIs for module: {name}. Error: {e}"); | |
| return None; | |
| } |
|
superseded by #81 |
For the record: most of the code in the "feat: compile modules in parallel" commit has been "vibe" "coded". My experience of it was painful: it was super super slow, the agent generated incorrect code several times, that didn't compile, and it got stuck not managing to fix the compilation failures for a short cycle.
I'll probably rejigger the changes so they make more sense, but overall this is nice to compile the modules in parallel, even though it's not really a problem for release builds of the rust modules (it's more a problem for the debug builds where lots of lib code isn't stripped out).