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]>
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
| @@ -58,7 +58,8 @@ class WasmBase : public std::enable_shared_from_this<WasmBase> { | |||
| WasmBase(const std::shared_ptr<WasmHandleBase> &other, WasmVmFactory factory); | |||
| virtual ~WasmBase(); | |||
|
|
|||
| 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
| * @return whether or not the load was successful. | ||
| */ | ||
| 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]>
include/proxy-wasm/wasm_vm.h
Outdated
| * @return whether or not the load was successful. | ||
| */ | ||
| 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]>
Signed-off-by: Piotr Sikora [email protected]