Extract common bytecode operations into WasmBase. #161
Conversation
|
@mathetake could you take a look if this change (moving all bytecode processing from WasmVMs into WasmBase) makes sense? I think it's directionally correct, but it could use another set of eyes before I start rewriting tests. |
|
just had a look and yeah this change makes sense to me. Thanks! |
Signed-off-by: Piotr Sikora <[email protected]>
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
@mathetake PTAL, I left some comments to point out the API changes that might be worth discussing.
Envoy PR: envoyproxy/envoy#16430
|
|
||
| bool initialize(const std::string &code, bool allow_precompiled = false); | ||
| bool load(const std::string &code, bool allow_precompiled = false); | ||
| bool initialize(); |
There was a problem hiding this comment.
WasmBase::initialize(module, ...) is split into WasmBase::load(module, ...) and WasmBase::initialize(), and only the latter is called in clones (even in case of non-cloneable runtimes, since the base WasmBase is supposed to do all necessary pre-processing of the bytecode).
include/proxy-wasm/wasm_vm.h
Outdated
| */ | ||
| virtual bool load(const std::string &code, bool allow_precompiled) = 0; | ||
| virtual bool load(std::string_view module, bool is_precompiled, | ||
| std::unordered_map<uint32_t, std::string> function_names) = 0; |
There was a problem hiding this comment.
WasmVm::load() now takes either stripped or precompiled module, and function names index (i.e. .wasm file is preprocessed in WasmBase).
There was a problem hiding this comment.
this seems premature optimization but we could share a single function_names map and pass here by reference to it so that we don't copy it to all the thread local VMs..
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
mathetake
left a comment
mathetake
left a comment
There was a problem hiding this comment.
looks good, thanks! I left some nits.
include/proxy-wasm/wasm_vm.h
Outdated
| */ | ||
| virtual bool load(const std::string &code, bool allow_precompiled) = 0; | ||
| virtual bool load(std::string_view module, bool is_precompiled, | ||
| std::unordered_map<uint32_t, std::string> function_names) = 0; |
There was a problem hiding this comment.
this seems premature optimization but we could share a single function_names map and pass here by reference to it so that we don't copy it to all the thread local VMs..
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
…til. Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
2 participants
Signed-off-by: Piotr Sikora [email protected]