refactor: ExecuteFunction without std::function #716

gumb0 · 2021-02-02T16:33:22Z

Simpler alternative to #643.

codecov · 2021-02-02T17:03:40Z

Codecov Report

Merging #716 (0ad804f) into master (07f87db) will increase coverage by 0.01%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master     #716      +/-   ##
==========================================
+ Coverage   99.24%   99.25%   +0.01%     
==========================================
  Files          76       76              
  Lines       11482    11571      +89     
==========================================
+ Hits        11395    11485      +90     
+ Misses         87       86       -1

Flag	Coverage Δ
rust	`99.86% <ø> (ø)`
spectests	`90.53% <90.00%> (-0.03%)`	⬇️
unittests	`99.21% <97.29%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tools/wasi/wasi.cpp	`59.57% <16.66%> (ø)`
lib/fizzy/capi.cpp	`99.27% <100.00%> (+0.35%)`	⬆️
lib/fizzy/instantiate.cpp	`100.00% <100.00%> (ø)`
lib/fizzy/instantiate.hpp	`100.00% <100.00%> (ø)`
test/unittests/api_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_call_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_control_test.cpp	`99.46% <100.00%> (ø)`
test/unittests/execute_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/instantiate_test.cpp	`100.00% <100.00%> (ø)`
test/utils/fizzy_engine.cpp	`100.00% <100.00%> (ø)`

lib/fizzy/execute.cpp

test/unittests/execute_call_test.cpp

chfast · 2021-02-10T15:53:50Z

I pushed a proposed implementation simplification.
There should be also an option to eliminate shared_ptr, but I will leave it for some other time - this requires wrapping C with C++ not the other way around.

gumb0 · 2021-02-10T18:09:59Z

I pushed a proposed implementation simplification.
There should be also an option to eliminate shared_ptr, but I will leave it for some other time - this requires wrapping C with C++ not the other way around.

Have you seen #643 (comment)?
shared_ptr is got rid of there, suggestion are welcome on how to simplify it.

chfast · 2021-02-10T19:01:11Z

Ok, but is it still worth to add memory allocation for wasm function case? Even if temporarily?

gumb0 · 2021-02-10T19:28:17Z

Ok, but is it still worth to add memory allocation for wasm function case? Even if temporarily?

Yeah, agree it might be a good optimization here.

test/unittests/execute_call_test.cpp

lib/fizzy/instantiate.cpp

axic · 2021-03-10T15:01:49Z

test/unittests/api_test.cpp

    const auto bar_type = FuncType{{}, {ValType::i32}};

    auto instance_reexported_function =
-        instantiate(parse(wasm_reexported_function), {{bar, bar_type.inputs, bar_type.outputs}});


The inputs/outputs seems to be like a stray change, but looking at other cases below we do seem to have a constructor for it.

Yes, it's not required / can be split out.

Fine with me, it is only this case so having a single PR for it would be overkill.

axic · 2021-03-10T15:02:23Z

test/unittests/execute_call_test.cpp

@@ -361,14 +363,14 @@ TEST(execute_call, imported_function_call_void)

    const auto module = parse(wasm);

-    bool called = false;
-    const auto host_foo = [&called](Instance&, const Value*, int) {
+    static bool called = false;


Hmm, why not just capture this in the function below?

Lambda's capture can't be used, because only lambdas without capture can be converted into function pointer.

Another option is to pass it via host_context, but I decided against it, see https://github.com/wasmx/fizzy/pull/716/files/5baa695073c325728d1e60ee321ef94bf77e7248#r580939050

Also a possible evolution of this that would allow lambdas with capture is in #740

axic · 2021-03-10T15:06:43Z

test/unittests/execute_call_test.cpp

@@ -652,13 +663,13 @@ TEST(execute_call, call_initial_depth)
    const auto wasm = from_hex("0061736d01000000010401600000020b01036d6f6403666f6f0000");

    auto module = parse(wasm);
-    auto host_foo = [](Instance& /*instance*/, const Value*, int depth) -> ExecutionResult {


Oh actually why do we drop the explicit return type?

It can be implicitly deduced.

Sure, we just used it for clarity. But either way it is fine, the thing is typechecked in instantiate.

test/unittests/execute_call_test.cpp

axic · 2021-03-10T15:45:01Z

test/unittests/execute_call_test.cpp

+        "0061736d0100000001070160027f7f017f020a01026d31037375620000030201000a0a0108002000200110000"
+        "b");
+
+    const auto func_idx = fizzy::find_exported_function(*module1, "sub");


Took me a while looking up headers what is going on. So confusing we have find_exported_function with instance and module, returning different things.

Yes, in C API this one is fizzy_find_exported_function_index, probably should be renamed in C++, too.

axic · 2021-03-10T15:52:13Z

test/unittests/api_test.cpp

@@ -217,6 +217,40 @@ TEST(api, resolve_imported_function_duplicate)
    EXPECT_THAT(execute(*instance, 1, {0_u32}), Result(42));
 }

+TEST(api, resolve_imported_function_duplicate_with_context)


The point of this test to check that even though the any is duplicated in each ExternalFunction, they are somehow still reference to the same?

It's just to check that any context is copied as we expect it to and to illustrate what you would do to share a context.

It's reference to the same only, if it's a pointer inside any.

Maybe another case would be informative, when any has non-shared copied values.

axic · 2021-03-10T15:57:15Z

lib/fizzy/instantiate.hpp

@@ -46,9 +46,6 @@ class ExecuteFunction
    std::any m_host_context;

 public:
-    /// Default function constructor.
-    ExecuteFunction() noexcept = default;


Was the constructor of the line fizzy::ImportedFunction imported_func; using this?

axic · 2021-03-10T15:57:39Z

lib/fizzy/capi.cpp

    const auto& c_type = c_imported_func.external_function.type;
-    imported_func.inputs.resize(c_type.inputs_size);
+
+    std::vector<fizzy::ValType> inputs(c_type.inputs_size);


While using this constructor here and reserve in the case below?

Here it's possible to default-construct ValTypes.
Not much difference, but I find it a bit simpler to use vector constructor + transform, than reserve + transform + back_inserter.

test/unittests/execute_call_test.cpp

axic · 2021-03-10T16:00:29Z

test/unittests/execute_call_test.cpp

+    };
+    const auto& host_func_type = module->typesec[0];
+
+    int counter = 0;


Why do you int type here and uint32_t type in the other test? Just because in the other case the value is returned to wasm?

Yes, I think that was the motivation for uint32_t in the test below.

axic · 2021-03-10T16:01:07Z

test/unittests/execute_call_test.cpp

+    EXPECT_EQ(counter, 2);
+}
+
+TEST(execute_call, imported_functions_with_shared_cotext)


Isn't this kind of a duplicate of resolve_imported_function_duplicate_with_context)?

They are somewhat similar, but in resolve it's a single function that is imported twice and context is copied internally.
Here it's different functions and the user copies the context.

gumb0 force-pushed the execute-function-noexcept branch 3 times, most recently from 0ba7217 to ff0b51a Compare February 2, 2021 16:54

gumb0 force-pushed the execute-function-noexcept branch 3 times, most recently from 13ae5ff to f05b6c4 Compare February 3, 2021 14:00

gumb0 commented Feb 3, 2021

View reviewed changes

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved

gumb0 force-pushed the execute-function-noexcept branch 7 times, most recently from e62f7d0 to 86b574b Compare February 8, 2021 11:14

gumb0 commented Feb 9, 2021

View reviewed changes

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

gumb0 force-pushed the execute-function-noexcept branch 2 times, most recently from 8d42298 to cadff06 Compare February 10, 2021 14:57

gumb0 force-pushed the execute-function-noexcept branch 5 times, most recently from ff45d85 to 7ae3859 Compare February 11, 2021 14:31

chfast reviewed Feb 11, 2021

View reviewed changes

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

gumb0 force-pushed the execute-function-noexcept branch 2 times, most recently from 859532a to efc9b87 Compare February 11, 2021 17:21

axic reviewed Mar 10, 2021

View reviewed changes

lib/fizzy/instantiate.cpp Outdated Show resolved Hide resolved

axic reviewed Mar 10, 2021

View reviewed changes

test/unittests/execute_call_test.cpp Show resolved Hide resolved

axic reviewed Mar 10, 2021

View reviewed changes

test/unittests/execute_call_test.cpp Show resolved Hide resolved

axic reviewed Mar 10, 2021

View reviewed changes

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

axic reviewed Mar 10, 2021

View reviewed changes

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

axic reviewed Mar 10, 2021

View reviewed changes

gumb0 force-pushed the execute-function-noexcept branch from 1996489 to 31d5017 Compare March 10, 2021 18:55

axic approved these changes Mar 10, 2021

View reviewed changes

gumb0 force-pushed the execute-function-noexcept branch from 31d5017 to 16e12bc Compare March 10, 2021 19:41

gumb0 added 5 commits March 11, 2021 12:19

refactor: ExecuteFunction with std::any context instead of std::function

34ea90d

test: Add a call test of imported wasm function via instance + func_idx

23846ef

test: Imported host functions sharing a context

5dff6d6

refactor: remove default constructor of ExecuteFunction

5f06883

test: Calling host functions with a context

0ad804f

gumb0 force-pushed the execute-function-noexcept branch from 16e12bc to 0ad804f Compare March 11, 2021 11:21

gumb0 merged commit 3b73c24 into master Mar 11, 2021

gumb0 deleted the execute-function-noexcept branch March 11, 2021 12:13

gumb0 mentioned this pull request Mar 11, 2021

refactor: ExecuteFunction instead of std::function #643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: ExecuteFunction without std::function #716

refactor: ExecuteFunction without std::function #716

gumb0 commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

chfast commented Feb 10, 2021

gumb0 commented Feb 10, 2021

chfast commented Feb 10, 2021

gumb0 commented Feb 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021 •

edited

Loading

axic Mar 10, 2021

gumb0 Mar 10, 2021

refactor: ExecuteFunction without std::function #716

refactor: ExecuteFunction without std::function #716

Conversation

gumb0 commented Feb 2, 2021 • edited Loading

codecov bot commented Feb 2, 2021 • edited Loading

Codecov Report

chfast commented Feb 10, 2021

gumb0 commented Feb 10, 2021

chfast commented Feb 10, 2021

gumb0 commented Feb 10, 2021

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

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

gumb0 Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

gumb0 Mar 10, 2021 •

edited

Loading