Skip to content

Add Wasmtime runtime. #73

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 44 commits into from
Nov 10, 2020
Merged

Add Wasmtime runtime. #73

merged 44 commits into from
Nov 10, 2020

Conversation

mathetake
Copy link
Contributor

@mathetake mathetake commented Oct 22, 2020 •
edited
Loading

Signed-off-by: mathetake [email protected]

@mathetake mathetake force-pushed the wasmtime branch 2 times, most recently from 22afd7b to 0505324 Compare October 22, 2020 15:30
@mathetake
Copy link
Contributor Author

verified that it has started working fine and passing all the e2e tests in https://github.com/tetratelabs/proxy-wasm-go-sdk

PiotrSikora
PiotrSikora reviewed Oct 23, 2020
View reviewed changes
mathetake
mathetake commented Oct 23, 2020
View reviewed changes
mathetake
mathetake commented Oct 23, 2020
View reviewed changes
@mathetake
Copy link
Contributor Author

hmm..... I haven't been able to identify the cause......

@mathetake
Copy link
Contributor Author

mathetake commented Oct 23, 2020 •
edited
Loading

as for the performance, they seem to have the compile time optimization configurations: https://github.com/bytecodealliance/wasmtime/blob/main/crates/c-api/include/wasmtime.h#L84-L105
we should use them for benchmarks later

@mathetake mathetake marked this pull request as ready for review October 24, 2020 01:33
@mathetake
Copy link
Contributor Author

@PiotrSikora I followed Lizan's advice and decided to use smart pointers 🙂 But I still have struggles to identify the root cause of memory leakage when used in Envoy. Besides that, I believe this is ready for review. Please take a look!

@lizan I need your advice on C++, bazel in general 🙂

mathetake
mathetake commented Oct 24, 2020
View reviewed changes
PiotrSikora
PiotrSikora reviewed Oct 25, 2020
View reviewed changes
@mathetake mathetake force-pushed the wasmtime branch 2 times, most recently from e2f60d1 to 37a3c80 Compare October 26, 2020 07:37
@mathetake mathetake requested a review from PiotrSikora October 26, 2020 07:51
@mathetake
Copy link
Contributor Author

mathetake commented Oct 26, 2020 •
edited
Loading

@PiotrSikora I managed to generate BUILD files for wasmtime in 37a3c80 , which is based on https://github.com/mathetake/bazel-wasmtime-c-api repo where I used cargo-raze and a few of manual tweeks. PTAL! 😃

PiotrSikora
PiotrSikora reviewed Oct 26, 2020
View reviewed changes
@mathetake mathetake requested a review from PiotrSikora October 27, 2020 05:06
@PiotrSikora
Copy link
Member

Could you split this into 2 PRs? One that just adds Wasmtime build machinery, and another with an actual implementation.

PiotrSikora
PiotrSikora requested changes Nov 3, 2020
View reviewed changes
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.

Generally, it looks great, thanks!

A left a few nits, and it looks that you're missing getStrippedSource().

@mathetake
add wasmtime runtime
eaee9b8 
Signed-off-by: mathetake <[email protected]>
@mathetake
enhance signature message
e7c9980 
Signed-off-by: mathetake <[email protected]>
@mathetake
enhance clone test
34a1cc5 
Signed-off-by: mathetake <[email protected]>
@mathetake
add trap test
5965603 
Signed-off-by: mathetake <[email protected]>
@mathetake
tiny fix
8b4277b 
Signed-off-by: mathetake <[email protected]>
@mathetake
add windows flag
f6498a6 
Signed-off-by: mathetake <[email protected]>
@mathetake
fix memory leak: partially
a5e340b 
Signed-off-by: mathetake <[email protected]>
@PiotrSikora
Copy link
Member

Why disabling cache?

@mathetake
Copy link
Contributor Author

wasmtime's cache spawns a new thread for a cache worker, and I thought it would affect the host's performance, although the same may be true for V8.
https://github.com/bytecodealliance/wasmtime/blob/main/crates/cache/src/worker.rs#L77-L79
https://github.com/bytecodealliance/wasmtime/blob/main/crates/cache/src/worker.rs#L253-L273

and what's more, it seems to use file system rather than in-memory cache: https://github.com/bytecodealliance/wasmtime/blob/main/crates/cache/src/worker.rs#L404

That said, I'm not sure how impactful it is, so I disable it for now (as an experimental runtime)

@PiotrSikora anythought?

@PiotrSikora
Copy link
Member

Ah, OK. Sounds good!

btw: v0.21.0 is out.

@mathetake
update wasmtime to 0.21.0
eab25dd 
Signed-off-by: mathetake <[email protected]>
@mathetake
Copy link
Contributor Author

btw: v0.21.0 is out.

Thanks! Updated

@mathetake
Copy link
Contributor Author

envoyproxy/envoy@a871b14 also updated for envoy

PiotrSikora
PiotrSikora reviewed Nov 9, 2020
View reviewed changes
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.

A few more nits before merge.

PiotrSikora
PiotrSikora reviewed Nov 10, 2020
View reviewed changes
@mathetake
use wasm-c-api
92ad8a3 
Signed-off-by: mathetake <[email protected]>
@mathetake
add wasm-c-api.BUILD
ba07309 
Signed-off-by: mathetake <[email protected]>
PiotrSikora
PiotrSikora approved these changes Nov 10, 2020
View reviewed changes
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.

Thanks!

@PiotrSikora PiotrSikora changed the title add new runtime: wasmtime Add Wasmtime runtime. Nov 10, 2020
@PiotrSikora
Copy link
Member

Ah, one more thing. You need to include sha256 in generated BUILD files for included crates, otherwise Bazel complains about sources not being pinned. You need to run cargo generate-lockfile before running cargo-raze tool. Also, please check-in resulting Cargo.lock file.

PiotrSikora
PiotrSikora reviewed Nov 10, 2020
View reviewed changes
@PiotrSikora PiotrSikora merged commit 4741d2f into proxy-wasm:master Nov 10, 2020
@PiotrSikora
Copy link
Member

Thanks!

@mathetake mathetake deleted the wasmtime branch November 10, 2020 09:02
lizan pushed a commit to envoyproxy/envoy that referenced this pull request Nov 12, 2020
@mathetake @PiotrSikora
wasm: add wasmtime runtime (#13932)
3b4d165 
Commit Message: wasm: Add Wasmtime as a new runtime
Additional Description: add a new wasm runtime named Wasmtime
Risk Level: low
Testing: run in `bazel.compile_time_options` build target as WAVM.
Docs Changes: Docs will be provided in subsequent PRs.
Release Notes: enable Wasmtime as a Wasm filter runtime optionally.
ref: proxy-wasm/proxy-wasm-cpp-host#73

Signed-off-by: mathetake <[email protected]>
Co-authored-by: Piotr Sikora <[email protected]>
rexengineering pushed a commit to rexengineering/istio-envoy that referenced this pull request Oct 15, 2021
@mathetake @PiotrSikora
wasm: add wasmtime runtime (#13932)
6cbde94 
Commit Message: wasm: Add Wasmtime as a new runtime
Additional Description: add a new wasm runtime named Wasmtime
Risk Level: low
Testing: run in `bazel.compile_time_options` build target as WAVM.
Docs Changes: Docs will be provided in subsequent PRs.
Release Notes: enable Wasmtime as a Wasm filter runtime optionally.
ref: proxy-wasm/proxy-wasm-cpp-host#73

Signed-off-by: mathetake <[email protected]>
Co-authored-by: Piotr Sikora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PiotrSikora PiotrSikora PiotrSikora approved these changes

@lizan lizan Awaiting requested review from lizan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mathetake @PiotrSikora @lizan