-
Notifications
You must be signed in to change notification settings - Fork 34
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
Wasm Edge #1864
Wasm Edge #1864
Conversation
…arate-nodes-and-values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪
@@ -101,6 +96,7 @@ namespace { | |||
template <auto mf> | |||
wasm::Literal callHostApiFunc(kagome::host_api::HostApi *host_api, | |||
const wasm::LiteralList &arguments) { | |||
std::cout << "Call Host method " << __PRETTY_FUNCTION__ << "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SL_DEBUG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still actual
@@ -9,10 +9,11 @@ | |||
#include <tuple> | |||
#include <utility> | |||
|
|||
#include <fmt/std.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"fmt/std.h" formatter<std::error_code> = "{category}:{enum}"
(no message) conflicts with "qtils/error.hpp" formatter<std::error_code> = "{category}:{enum}:{message}"
.
Should we replace "qtils/error.hpp" formatter
with #include <fmt/std.h>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also conflicts with <libp2p/outcome/outcome.hpp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qtils/error.hpp looks more descriptive.
* along with root hash storing logic from the trie db component | ||
*/ | ||
class TrieStorageBackend : public BufferStorage { | ||
class TrieNodeStorageBackend : public BufferStorage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(maybe changes related to #1770 from wasmedge pr)
Use BufferStorage
directly, or use wrapper instead of inheritance.
#1770 (comment)
class TrieNodeStorageBackend : public BufferStorage { | |
using TrieNodeStorageBackend = BufferStorage; | |
struct TrieValueStorageBackend = BufferStorage; |
class TrieNodeStorageBackend : public BufferStorage { | |
struct TrieNodeStorageBackend { std::shared_ptr<BufferStorage> v; }; | |
struct TrieValueStorageBackend { std::shared_ptr<BufferStorage> v; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BufferStorage
is good simple interface.
To reuse existing BufferStorage
implementations for new interface you need to create proxy wrapper class inheriting new interface (new interface calls corresponding BufferStorage
) or just reinterpret_cast
.
TrieNodeStorageBackend
/TrieValueStorageBackend
don't add any functions to BufferStorage
, so they must not inherit from it and just wrap pointer.
class TrieStorageBackendImpl : public TrieNodeStorageBackend, | ||
public TrieValueStorageBackend { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this strange diamond inheritance require virtual
inheritance?
(Remove this class, use BufferStorage
directly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are ambigous/conflicting interfaces.
Without virtual
inheritance this class will contain BufferStorage
instance twice (for each non-virtual inheriting subclass).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BufferStorage
is abstract base class and wont be instantiated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BufferStorage
is abstract base class and wont be instantiated.
You can't create BufferStorage
variable because it's vtable is empty,
but subclass always contains/stores superclass (struct SubClass { SuperClass superclass; }
).
@@ -20,6 +20,7 @@ namespace kagome::storage { | |||
kBlockBody, | |||
kJustification, | |||
kTrieNode, | |||
kTrieValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May use same kTrieNode
space with prefix (both!)
node_db = MapPrefix{"n", db.getSpace(kTrieNode)}
value_db = MapPrefix{"v", db.getSpace(kTrieNode)}
@@ -1,5 +1,5 @@ | |||
HunterGate( | |||
URL https://github.com/qdrvm/hunter/archive/refs/tags/v0.23.257-qdrvm11.tar.gz | |||
SHA1 b2a69ae501bdc99006fb86e55930640004468556 | |||
URL "https://github.com/qdrvm/hunter/archive/heads/feature/wasm-edge.zip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hunter with wasm-edge should be released before merge
@@ -156,6 +157,16 @@ | |||
#include "runtime/runtime_api/impl/session_keys_api.hpp" | |||
#include "runtime/runtime_api/impl/tagged_transaction_queue.hpp" | |||
#include "runtime/runtime_api/impl/transaction_payment_api.hpp" | |||
|
|||
#if KAGOME_WASM_COMPILER_WASM_EDGE == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or #ifdef
void register_host_api(WasmEdge_ModuleInstanceContext *instance) { | ||
BOOST_ASSERT(instance); | ||
// clang-format off | ||
REGISTER_HOST_METHOD(void, ext_allocator_free_version_1, WasmPointer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you extract list of ext_ function to file common for WAVM and WasmEdge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want it to look like this?
/// host_api_macro.hpp
// no "#pragma once"
HOST_API_MACRO(void, ext_allocator_free_version_1, WasmPointer)
HOST_API_MACRO(int32_t, ext_allocator_malloc_version_1, int32_t)
...
/// wavm.hpp
#define HOST_API_MACRO(...) wavm.add(...)
#include "host_api_macro.hpp"
/// wasmedge.hpp
#define HOST_API_MACRO(...) wasmedge.add(...)
#include "host_api_macro.hpp"
or try to make tuple-like template?
template <auto...> struct TemplateTuple {};
using HostApiTuple = TemplateTuple<
&HostApi::ext_allocator_free_version_1,
&HostApi::ext_allocator_malloc_version_1,
...
>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll look into it. Maybe in this PR, maybe make an issue.
class TrieStorageBackendImpl : public TrieNodeStorageBackend, | ||
public TrieValueStorageBackend { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BufferStorage
is abstract base class and wont be instantiated.
common::BufferView loadN(WasmPointer addr, WasmSize n) const override { | ||
BOOST_ASSERT(n > 0); | ||
auto ptr = WasmEdge_MemoryInstanceGetPointer(mem_instance_, addr, n); | ||
BOOST_ASSERT(ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw
(or you will get NullPointerException)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory methods may be called by the host outside of the runtime call, where we catch exceptions, so given that most our code is exception-free I'm not sure it's a good idea to throw here. I think we're gonna need to tackle this problem separately and produce a uniform solution for all WASM executors in kagome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadN
(and all callers) can return outcome
.
host api can return outcome
or throw with .value()
, and host api wrapper function will check outcome
or catch
exceptions
Referenced issues
Description of the Change
Benefits
Possible Drawbacks
Usage Examples or Tests
Alternate Designs