Add a test case for using host functions #320

axic · 2020-05-19T17:24:14Z

Depends on #318.

test/unittests/wasm_engine_test.cpp

axic · 2020-05-19T17:25:31Z

test/unittests/wasm_engine_test.cpp

+    {
+        auto engine = engine_create_fn();
+        ASSERT_TRUE(engine->parse(wasm));
+        ASSERT_TRUE(engine->instantiate(wasm));


Ideally this test case would add the imported function here, but right now I would refrain from that because that introduces a call overhead in our abstraction. Maybe I'm wrong here, @gumb0 @chfast?

I'm not sure this will help in the case were we have only single host function.

For this PR it would not make any difference, however if the interface would be flexible, we could add more functions in fizzy-bench or have external tools use it, such as ewasm-bench, with a multitude of functions.

axic · 2020-05-19T18:42:41Z

test/utils/wabt_engine.cpp

@@ -35,6 +36,17 @@ std::unique_ptr<WasmEngine> create_wabt_engine()
 bool WabtEngine::parse(bytes_view input) const
 {
    wabt::interp::Environment env;
+
+    wabt::interp::HostModule* hostModule = env.AppendHostModule("env");
+    assert(hostModule != nullptr);


Should return false.

I think this is fine as it is. Would fail on memory allocation only.

axic · 2020-05-19T18:44:19Z

Will also add a micro-benchmark which calls env::crc32() N-times and adds the results together (v += crc32(v).

chfast

I see two problem:

Big maintenance burden. Is it number of host function times number of engines?
This predates "call" rework/optimization so it will be another part of code to update.

test/benchmarks/micro/host_adler32.wast

axic · 2020-05-20T16:49:25Z

Big maintenance burden. Is it number of host function times number of engines?

Only plan to have this single host function for unit testing and benchmarking.

chfast

Only plan to have this single host function for unit testing and benchmarking.

Then it is fine.

test/utils/fizzy_engine.cpp

test/benchmarks/micro/host_adler32.inputs

test/benchmarks/micro/host_adler32.wast

test/utils/adler32.hpp

test/utils/fizzy_engine.cpp

test/utils/wasm3_engine.cpp

gumb0 · 2020-05-22T17:05:31Z

test/utils/wasm3_engine.cpp

+const void* env_adler32(IM3Runtime /*runtime*/, uint64_t* stack, void* mem)
+{
+    uint64_t* ret = stack;
+    uint32_t offset = *(uint32_t*)stack++;


Suggested change

uint32_t offset = *(uint32_t*)stack++;

const auto offset = *static_cast<uint32_t*>(stack++);

Will do, it was copy paste from wasm3 :)

gumb0 · 2020-05-22T17:09:37Z

test/utils/wabt_engine.cpp

+    assert(hostModule != nullptr);
+
+    hostModule->AppendFuncExport("adler32", {{wabt::Type::I32, wabt::Type::I32}, {wabt::Type::I32}},
+        [=](const wabt::interp::HostFunc*, const wabt::interp::FuncSignature*,


Quite big for a lambda, can you make the code inside a separate private method?

Also why not use the same function in parse?

Parse doesn't need it, it only wants the signature.

I had a different version using a unique_ptr but something wasn't working. The problem is GetMemory will only return correctly after instantiation, so it can't be captured prior. Also wabt has no context argument, so we'll need a lambda in any case, but could extract out into a helper.

I would prefer however not to do any of that here, but someone else could optimize these in subsequent PRs.

test/utils/fizzy_engine.cpp

chfast · 2020-05-22T17:12:21Z

test/utils/wasm3_engine.cpp

+const void* env_adler32(IM3Runtime /*runtime*/, uint64_t* stack, void* mem)
+{
+    uint64_t* ret = stack;
+    uint32_t offset = *(uint32_t*)stack++;


Is this ((uint32_t*)stack)++ or (uint32_t*)(stack++)?

I believe the intention is *(uint32_t*)(stack++). It is a copy paste from wasm3.

gumb0 · 2020-05-22T17:44:45Z

test/utils/fizzy_engine.cpp

+fizzy::execution_result env_adler32(fizzy::Instance& instance, std::vector<uint64_t> args, int)
+{
+    assert(instance.memory != nullptr);
+    // Since memory is a bytes_view, substr produces another bytes_view and no copy is performed.


comment is kinda outdated now :)

Will remove before merging.

gumb0 · 2020-05-22T17:48:43Z

test/utils/wabt_engine.cpp

+            assert(m_env.GetMemoryCount() > 0);
+            auto memory = m_env.GetMemory(0);
+            assert(memory != nullptr);
+            auto memory_data = memory->data;


what's the type of memory_data?

it's vector<char>

Some wabt memory type. Apparently std::vector<char> data.

chfast

With single outdated comment to be fixed.

chfast · 2020-05-25T14:34:15Z

test/unittests/wasm_engine_test.cpp

+    {
+        auto engine = engine_create_fn();
+        ASSERT_TRUE(engine->parse(wasm));
+        ASSERT_TRUE(engine->instantiate(wasm));


I'm not sure this will help in the case were we have only single host function.

chfast · 2020-05-25T14:34:43Z

test/utils/fizzy_engine.cpp

+fizzy::execution_result env_adler32(fizzy::Instance& instance, std::vector<uint64_t> args, int)
+{
+    assert(instance.memory != nullptr);
+    // Since memory is a bytes_view, substr produces another bytes_view and no copy is performed.


…head

axic commented May 19, 2020

View reviewed changes

test/unittests/wasm_engine_test.cpp Outdated Show resolved Hide resolved

axic commented May 19, 2020

View reviewed changes

axic force-pushed the test-host branch 3 times, most recently from 3cce710 to 7746d7f Compare May 20, 2020 16:35

chfast reviewed May 20, 2020

View reviewed changes

axic commented May 20, 2020

View reviewed changes

test/benchmarks/micro/host_adler32.wast Outdated Show resolved Hide resolved

axic force-pushed the test-host branch 3 times, most recently from a0632b6 to d5b916a Compare May 21, 2020 00:02

chfast reviewed May 21, 2020

View reviewed changes

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

axic commented May 22, 2020

View reviewed changes

test/benchmarks/micro/host_adler32.inputs Outdated Show resolved Hide resolved

axic force-pushed the test-host branch 5 times, most recently from 7637f21 to 182e4ed Compare May 22, 2020 14:51

axic commented May 22, 2020

View reviewed changes

test/benchmarks/micro/host_adler32.wast Outdated Show resolved Hide resolved

axic force-pushed the test-host branch 2 times, most recently from 945103e to a9f790a Compare May 22, 2020 15:11

axic requested review from gumb0 and chfast May 22, 2020 15:15

axic force-pushed the test-host branch 5 times, most recently from 832d58d to 13af1cb Compare May 22, 2020 15:37

axic force-pushed the test-host branch from 13af1cb to d00beeb Compare May 22, 2020 15:38

gumb0 reviewed May 22, 2020

View reviewed changes

test/utils/adler32.hpp Outdated Show resolved Hide resolved

axic requested a review from gumb0 May 22, 2020 16:52

chfast reviewed May 22, 2020

View reviewed changes

test/utils/adler32.hpp Outdated Show resolved Hide resolved

test/utils/fizzy_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 22, 2020

View reviewed changes

test/utils/wasm3_engine.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 22, 2020

View reviewed changes

chfast reviewed May 22, 2020

View reviewed changes

axic force-pushed the test-host branch 2 times, most recently from e90cf0e to a0f952b Compare May 22, 2020 17:38

gumb0 reviewed May 22, 2020

View reviewed changes

chfast approved these changes May 25, 2020

View reviewed changes

axic added 6 commits May 25, 2020 15:46

test: add adler32 helper in utils

a6ed6e8

test: implement env::adler32 in fizzy_engine

30e6a82

test: implement env::adler32 in wabt_engine

aab3dfa

test: implement env::adler32 in wasm3_engine

e2a5b34

test: add wasm_engine_test case for host functions

c1fe910

benchmarks: add host_adler32 benchmark to measure host interface over…

5cbf41d

…head

axic force-pushed the test-host branch from a0f952b to 5cbf41d Compare May 25, 2020 14:51

axic merged commit d0b883f into master May 25, 2020

axic deleted the test-host branch May 25, 2020 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test case for using host functions #320

Add a test case for using host functions #320

axic commented May 19, 2020 •

edited

Loading

axic May 19, 2020

chfast May 25, 2020

axic May 25, 2020

axic May 19, 2020

axic May 22, 2020

axic commented May 19, 2020

chfast left a comment

axic commented May 20, 2020

chfast left a comment

gumb0 May 22, 2020

axic May 22, 2020

gumb0 May 22, 2020

axic May 22, 2020 •

edited

Loading

chfast May 22, 2020

axic May 22, 2020

gumb0 May 22, 2020

axic May 25, 2020

chfast May 25, 2020

gumb0 May 22, 2020

gumb0 May 22, 2020

axic May 22, 2020

chfast left a comment

chfast May 25, 2020

chfast May 25, 2020

	uint32_t offset = (uint32_t)stack++;
	const auto offset = static_cast<uint32_t>(stack++);

Add a test case for using host functions #320

Add a test case for using host functions #320

Conversation

axic commented May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented May 19, 2020

chfast left a comment

Choose a reason for hiding this comment

axic commented May 20, 2020

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented May 19, 2020 •

edited

Loading

axic May 22, 2020 •

edited

Loading