refactor(smartengine): add abstraction layer for WASM engine #3257
Conversation
xxchan
changed the title
|
Nice work on adding WasmEdge features. To make it easier to review, can you split these into two separate PR: (1) non WasEdge related changes so we can merge that (2) WasEdge related code. We still have to figure out how to support WasEdge features with minimum impact to Fluvio developers |
|
Sure. I'll do it later.
Sehyo Chang ***@***.***> 于2023年5月16日周二 12:30写道：
… Nice work on adding WasmEdge features. To make it easier to review, can
you split these into two separate PR: (1) non WasEdge related changes so we
can merge that (2) WasEdge related code. We still have to figure out how to
support WasEdge features with minimum impact to Fluvio developers
—
Reply to this email directly, view it on GitHub
<#3257 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJBQZNLW4AJBHMKGSMN6KOLXGNJNPANCNFSM6AAAAAAYCEJTBE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
xxchan
changed the title
|
WasmEdge code is removed. |
@sehz Just FYI, there's a rustfmt option (unstable) https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#group_imports I added and run Not sure whether you want it for the whole repo, as you use stable toolchian. |
There was a problem hiding this comment.
This becomes common/init.rs
| } | ||
|
|
||
| impl SmartModuleInstance { |
There was a problem hiding this comment.
This goes to common/mod.rs
| } | ||
| } | ||
|
|
||
| pub(crate) trait SmartModuleTransform: Send + Sync { |
There was a problem hiding this comment.
This goes to common/transforms/mod.rs
| [dependencies] | ||
| tracing ={ workspace = true } | ||
| thiserror = { workspace = true } | ||
| anyhow = { workspace = true } | ||
| serde = { workspace = true, features = ['derive'] } | ||
| # std is needed for ser/de AtomicU64 | ||
| serde = { workspace = true, features = ['derive', 'std'] } |
There was a problem hiding this comment.
previously std features is depended by wasmtime, I added it when developing WasmEdge
|
|
||
| /// Building SmartModule | ||
| #[derive(Default)] | ||
| pub struct SmartModuleChainBuilder { |
There was a problem hiding this comment.
This becomes src/engine/mod.rs
| I: WasmInstance, | ||
| F: WasmFn<Context = I::Context>, | ||
| { | ||
| let (ptr, len, version) = instance.write_input(&input, ctx)?; |
There was a problem hiding this comment.
the prev version used slice abstraction. is there a reason for changing this?
| &mut self, | ||
| input: &E, | ||
| ctx: &mut Self::Context, | ||
| ) -> Result<(i32, i32, i32)>; |
There was a problem hiding this comment.
Use slice abstraction as before
|
Stale pull request message |
This PR contains:
To run WasmEdge test, install wasmedge: https://wasmedge.org/book/en/quick_start/install.html
cargo test -p fluvio-smartengine --features wasmedge-engineI'd like to gain feedbacks about the overall design first. If the general ideas look good, we can take a deeper look and begin to resolve other points (e.g., styling, CI, ...), and maybe split into smaller PRs if necessary.
Some notes:
wasmedgeis WasmEdge-specific code, and that's ~300 lines of simple code. To discuss how to maintain WasmEdge related code, and to get the ideas of how the common code work, it might be better to keep WasmEdge related code now. This can actually make it easier to review this PR. I can remove the WasmEdge related code after the code review if that's not decided.