Skip to content

WAMR support clone #317

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

Merged
merged 1 commit into from
Dec 16, 2022
Merged

WAMR support clone #317

merged 1 commit into from
Dec 16, 2022

Conversation

lum1n0us
Copy link
Contributor

@lum1n0us lum1n0us commented Nov 9, 2022

  • Let WAMR support CompiledBytecode level of clone
  • enable WAMR JIT
  • upgrade to a WMAR newer commit

@PiotrSikora
Copy link
Member

@mpwarres PTAL

@lum1n0us lum1n0us force-pushed the support_clone branch 3 times, most recently from b0f730c to 209caa9 Compare November 13, 2022 07:19
@lum1n0us
Copy link
Contributor Author

@PiotrSikora @mathetake Just a reminder, please take a look. I am looking forward to your comments.

BTW, is there any update about the feature of supporting AOT files as a kind of pre-compiled .Wasm files?

@PiotrSikora
Copy link
Member

@PiotrSikora @mathetake Just a reminder, please take a look. I am looking forward to your comments.

@mpwarres took over this codebase, so I'll let him review this PR.

BTW, is there any update about the feature of supporting AOT files as a kind of pre-compiled .Wasm files?

Precompiled modules have been supported since day one. They are implemented for V8 and WAVM engines, but you need to add support for WAMR if you want, which should be pretty easy. Note that both of them currently load precompiled code from a Custom Section of .wasm module, so you'd need to wire this up yourself if you want to load precompiled modules from a separate files, but I don't see a reason why it shouldn't be supported.

@PiotrSikora PiotrSikora requested a review from mpwarres November 18, 2022 16:47
@mpwarres
Copy link
Contributor

Yup, looking at it today.

@lum1n0us
Copy link
Contributor Author

bytecodealliance/wasm-micro-runtime#1291 ... ... adding option in Proxy-Wasm C++ Host and Envoy to simply consume precompiled Wasm module from a separate file instead, we need to do that for Wasmtime anyway.

so you'd need to wire this up yourself if you want to load precompiled modules from a separate files, but I don't see a reason why it shouldn't be supported.

@PiotrSikora Thanks for answering. Just to be sure we are on the same page that

  • Proxy Wasm C++ Host would like to have a feature that Wasm Runtimes simply consume precompiled Wasm module from a separate file
  • And there is no ongoing development on this feature as we know for now

@PiotrSikora
Copy link
Member

@lum1n0us correct.

@lum1n0us
Copy link
Contributor Author

@PiotrSikora Great! Then I think we might working on it and provide a patch to enable the feature.

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.

LGTM, but could you also send a draft PR to Envoy updating WAMR and Proxy-Wasm C++ Host to your branch, so that it'd pass the tests there?

@lum1n0us
Copy link
Contributor Author

Here it is envoyproxy/envoy#24200.

@PiotrSikora
Copy link
Member

Here it is envoyproxy/envoy#24200.

It looks that it fails when running WAMR tests, e.g.:

[2022-11-25 06:34:36.458][20][error][wasm] [external/envoy/source/extensions/common/wasm/wasm_vm.cc:38] Function: proxy_on_context_create failed: Exception: thread signal env not inited
[2022-11-25 06:34:36.458][20][error][wasm] [external/envoy/source/extensions/common/wasm/wasm_vm.cc:38] Function: proxy_on_vm_start failed: Exception: thread signal env not inited
[2022-11-25 06:34:36.458][20][error][wasm] [external/envoy/source/extensions/common/wasm/wasm.cc:109] Wasm VM failed Failed to start thread-local Wasm

Could you take a look?

@lum1n0us lum1n0us force-pushed the support_clone branch 4 times, most recently from ae75695 to df276e2 Compare December 7, 2022 10:31
@lum1n0us
Copy link
Contributor Author

lum1n0us commented Dec 7, 2022

All fixed. envoyproxy/envoy#24200 😮‍💨

Copy link
Contributor

@mpwarres mpwarres left a comment

Choose a reason for hiding this comment

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

A couple minor questions, just for my own understanding.

@PiotrSikora already looked at and signed off on these changes. I'll wait a day in case he wants to take a final look, but can otherwise merge them if he's done.

- Let WAMR support `CompiledBytecode` level of clone
- upgrade to a WMAR newer commit(2022-11-09)

Signed-off-by: [email protected] <[email protected]>
@lum1n0us
Copy link
Contributor Author

@mathetake I know you are very busy, so I appreciate you can review the PR. Thanks very much.

@mathetake mathetake merged commit 72ce32f into proxy-wasm:master Dec 16, 2022
@lum1n0us lum1n0us deleted the support_clone branch December 16, 2022 02:06
johnlanni pushed a commit to higress-group/proxy-wasm-cpp-host that referenced this pull request Mar 25, 2025
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