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

DynVm Implementation: Dynamically loaded native modules #379

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Protryon
Copy link

This PR adds a variant of NullVm, DynVM, that can load in native modules as dynamically linked modules.

Haven't written any automated tests yet, wanted to check if any design changes were going to be needed first, since the tests are going to be a bit more complicated than NullVM.

We've been testing this internally in an Envoy integration.

@PiotrSikora
Copy link
Member

@mpwarres @kyessenov could one of you look at it? Thanks!

@PiotrSikora
Copy link
Member

This PR adds a variant of NullVm, DynVM, that can load in native modules as dynamically linked modules.

Nitpicking, but if we're still talking about the NullVM, then I'd rather use "dynamically-loaded NullVM plugins" or similar, instead of adding another name for the NullVM.

@Protryon
Copy link
Author

Protryon commented Feb 2, 2024

@mpwarres @kyessenov Any updates on this?

Copy link
Collaborator

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

I scanned through it, and it looks reasonable to me. I think you could probably add a few more macros to make it smaller. I like this approach since it's closer to full-Wasm - globals and shared libraries are directly visible in the library binary, and it allows separating the release/code management.

@PiotrSikora A lot of this is mechanical code - I think we just need to have enough test coverage for this to be accepted.

@Protryon
Copy link
Author

@kyessenov @PiotrSikora I've pushed up some tests for DynVM.

@martijneken
Copy link
Contributor

This is a very cool approach, let's get this merged. @PiotrSikora @kyessenov @mpwarres any other feedback here?

@kyessenov
Copy link
Collaborator

I'm fine with the PR. Please stamp the approval :)

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Other than the comments above, I think this still needs some tests and a PR with Envoy integration.

src/dyn/dyn_ffi.cc Outdated Show resolved Hide resolved
include/proxy-wasm/exports.h Show resolved Hide resolved
src/exports.cc Show resolved Hide resolved
include/proxy-wasm/exports.h Show resolved Hide resolved
test/utility.cc Show resolved Hide resolved
test/BUILD Show resolved Hide resolved
fix comments

format and lints

Signed-off-by: Protryon <[email protected]>
Signed-off-by: Protryon <[email protected]>
Signed-off-by: Protryon <[email protected]>
Signed-off-by: Protryon <[email protected]>
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