Extract common code from runtime implementations.

[mathetake]

fixes #148

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

still wip since I haven't tested on Envoy yet

[mathetake]

(char *)(code.data()) is a temporary workaround for now to pass build and we should fix. This is necessary since wasm::vec<byte_t>::make requires us to pass char vector instead of const char vector. If we want to do zero copy, we need to do cast like this or change the arg type of code to std::string&.. but I'm not sure why ::make require non-const bytes code since V8 seems to copy the code internally anyway..

[mathetake]

@PiotrSikora I need your inputs here.. Should we change the signature of load to std::string &code from const std::string &code so that we can have char * instead of const char* from code.data(), or we allow (char *)(code.data())..?

[mathetake]

The following is the error we get without modifying signature and just pass code.data()

external/com_googlesource_chromium_v8/wee8/third_party/wasm-api/wasm.hh:149:18: error: cannot initialize an array element of type 'char' with an rvalue of type 'typename std::remove_reference<const char *&>::type' (aka 'const char *')
    T data[] = { std::move(args)... };
                 ^~~~~~~~~~~~~~~
external/proxy_wasm_cpp_host/src/v8/v8.cc:293:37: note: in instantiation of function template specialization 'wasm::vec<char>::make<unsigned long, const char *>' requested here
      code_vec = wasm::vec<byte_t>::make(code.size(), code.data());

[PiotrSikora]

I think this might be the reason why we originally copied the bytecode. Perhaps make getStrippedSource always return std::string (either stripped version or a copy of the original bytecode) and then use it as input here?

[mathetake]

so we cannot avoid copying the original bytecode anyway.. thanks for suggestion.

[mathetake]

done ed0f3a5

[PiotrSikora]

The bytecode read from the configuration is passed to both: canary and base WasmVMs at startup, so I'm afraid that we have to copy it, unfortunately.

Perhaps we can rewrite this logic somehow, but that should probably be done in a separate PR.

[mathetake]

made ready for review at least now this passes Envoy tests

[PiotrSikora]

Thanks for working on this!

1. Wavm is an odd duck now. We're using its internal parsing to extract precompiled module in Wavm::load, but replaced Wavm::getCustomSection with WasmUtil. I think we should either replace the iterator in Wavm::load with WasmUtil::getCustomSection or retain getCustomSection as part of the interface, and use WasmUtil-based default, so that Wavm could override it.

2. Could you also extact code responsible for mapping ABI versions? Right now, we do the same string_view to AbiVersion conversion in each runtime.

[mathetake]

changed to use BytecodeUtil::getCustomSection in WAVM: d807f06

[mathetake]

OK verified that WAVM change also passes Envoy tests (locally😄).

[mathetake]

added ABI version extraction in BytecodeUtil and use it in getAbiVersion of runtimes except NullVm 305d48d 41a9ac6 ,

[PiotrSikora]

I think this might be the reason why we originally copied the bytecode. Perhaps make getStrippedSource always return std::string (either stripped version or a copy of the original bytecode) and then use it as input here?

[PiotrSikora]

Thanks! Could you update Envoy PR to make sure it passes with latest changes?

[mathetake]

Thanks! Once the test passes in Envoy, I will merge this and update Envoy to point to the main branch here.

[PiotrSikora]

Thanks! Once the test passes in Envoy, I will merge this and update Envoy to point to the main branch here.

We probably shouldn't update every single commit in Enovy, especially since sometimes they get merged out-of-order.

This is mostly refactoring, so I'd skip it.

[mathetake]

Yeah agreed👍

…

On Tue, Apr 13, 2021 at 17:13 Piotr Sikora ***@***.***> wrote: Thanks! Once the test passes in Envoy, I will merge this and update Envoy to point to the main branch here. We probably shouldn't update every single commit in Enovy, especially since sometimes they get merged out-of-order. This is mostly refactoring, so I'd skip it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#152 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADHDJ6JJDMTOEBOOQDDJ4ZLTIP4J7ANCNFSM42YZ22BQ> .

[PiotrSikora]

Thanks! Once the test passes in Envoy, I will merge this and update Envoy to point to the main branch here.

We probably shouldn't update every single commit in Enovy, especially since sometimes they get merged out-of-order.

This is mostly refactoring, so I'd skip it.

Well, that was terrible timing on my part... For anything that requires changes to code and/or tests, even if it's only because of refactoring, we need to update Envoy as well, otherwise we're might be blocking other PRs.

Do you mind reopening that Envoy PR?

[mathetake]

yeah will do