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: Implement parts of the extism runtime in WebAssembly #384

Merged
merged 31 commits into from
Jul 27, 2023
Merged

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Jul 7, 2023

This PR adds the kernel directory which contains a port of the Extism memory allocator compiled to WebAssembly and removes runtime/src/memory.rs completely.

Being able to re-use memory functions as a WASM module allows us to begin to experiment with porting Extism to new runtimes!

This is in a draft state while I'm verifying some of these changes.

@zshipko zshipko marked this pull request as ready for review July 18, 2023 20:28
@bhelx
Copy link
Contributor

bhelx commented Jul 18, 2023

Could you add a README to the kernel project with a loose description of why it exists? Any graphs or art you have might help too. Either that or a top level module comment.

@zshipko
Copy link
Contributor Author

zshipko commented Jul 18, 2023

Could you add a README to the kernel project with a loose description of why it exists? Any graphs or art you have might help too. Either that or a top level module comment.

Good call! Just added a README: https://github.com/extism/extism/blob/fd783357547c5065284efaaa2755bb517ace7bfa/kernel/README.md and some more documentation to the implementation too: https://github.com/extism/extism/blob/fd783357547c5065284efaaa2755bb517ace7bfa/kernel/src/lib.rs

@@ -378,22 +401,22 @@ impl Plugin {
}

pub(crate) fn initialize_runtime(&mut self) -> Result<(), Error> {
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be done in kernel-based SDKs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now yeah, that just makes it possible to side-step _start in modules that require initialization.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, good to know -- I think we need to include it in TODOs in the go-sdk cc/ @mhmd-azeez, unless its already being handled!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can use ModuleConfig.WithStartFunctions to do the same thing in wazero

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a TODO in the PR readme for it so that we don't forget it

match res {
Ok(()) => (),
Err(e) => {
plugin.store.set_epoch_deadline(1);
Copy link
Member

Choose a reason for hiding this comment

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

figuring all of this is actually expected to remain here... I suppose this is at minimum related to the Rust SDK then, and possibly does need to be implemented in kernel-based SDKs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, timeouts and memory limits are features that will have to be implemented by each kernel-based SDK. This PR doesn't really change anything, but it does make it possible to start building up new SDKs that don't rely on this code anymore. Like in Go, cancellation suddenly becomes way easier because of Contexts, but in Rust it's kinds of a pain right now. If we decided to have an async runtime it would probably change the design a little bit - those are all things we should definitely explore!

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

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

LGTM, just curious about the timeout/deadline/cancellation things referred to in other comments.

Really excited about this!

I think you should self-approve this one when ready ;)

@zshipko
Copy link
Contributor Author

zshipko commented Jul 27, 2023

Looks like I can't approve my own PR, but I'm merging this now!

@zshipko zshipko merged commit 3da5262 into main Jul 27, 2023
32 checks passed
@zshipko zshipko deleted the wasm-runtime branch July 27, 2023 18:31
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.

None yet

4 participants