Introduce ExecutionContext instead of depth param #766
Conversation
Codecov Report
@@ Coverage Diff @@
## master #766 +/- ##
==========================================
- Coverage 99.26% 99.25% -0.01%
==========================================
Files 77 77
Lines 11718 11740 +22
==========================================
+ Hits 11632 11653 +21
- Misses 86 87 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
chfast
force-pushed
the
execution_context
branch
from
cb73a26
to
c97d43e
Compare
The ExecutionContext type represents the storage for information shared by calls in the same execution "thread".
chfast
force-pushed
the
execution_context
branch
2 times, most recently
from
6a7e7a7
to
5bb55bb
Compare
lib/fizzy/capi.cpp
Outdated
| auto* func = static_cast<fizzy::ExternalFunction*>(context); | ||
| return wrap((func->function)(*unwrap(instance), unwrap(args), depth)); | ||
| static constexpr FizzyExternalFn c_function = | ||
| [](void* host_context, FizzyInstance* instance, const FizzyValue* args, |
There was a problem hiding this comment.
Nit: it seems some places use _ctx, while this uses _context. I think a while back we argued to use the long form, but if we go for the short one, please make it consistent.
There was a problem hiding this comment.
Shadowing issues here.
Do you want to change everything to context?
There was a problem hiding this comment.
It was _context before, I'm happy with _ctx too.
test/unittests/capi_test.cpp
Outdated
| @@ -330,7 +330,9 @@ TEST(capi, find_exported_function) | |||
| EXPECT_EQ(function.type.output, FizzyValueTypeI32); | |||
| EXPECT_NE(function.context, nullptr); | |||
| ASSERT_NE(function.function, nullptr); | |||
| EXPECT_THAT(function.function(function.context, instance, nullptr, 0), CResult(42_u32)); | |||
| fizzy::ExecutionContext ctx; | |||
| auto* const c_ctx = reinterpret_cast<FizzyExecutionContext*>(&ctx); | |||
There was a problem hiding this comment.
Hmm, is this the intended C API? :)
There was a problem hiding this comment.
Calling host function wrapper is not part of C API.
There was a problem hiding this comment.
Well function.function is accessible in C, it is the C struct, isn't it ?
What is the goal of this test?
There was a problem hiding this comment.
Yeah good point, it was possible to call FizzyExternalFunction before.
Maybe wrapped external function in FizzyExternalFunction wrap(fizzy::ExternalFunction external_func) should call unwrap(ctx) only if context is non-null, otherwise create default one.
There was a problem hiding this comment.
There should not be two way of executing a wasm function. You should pick if you want to execute returned pointer to external function. Or having execute(index).
Anyway, the solution in #771.
There was a problem hiding this comment.
I'm fine with either but should be documented.
There was a problem hiding this comment.
Can you also leave a comment here in the test too?
There was a problem hiding this comment.
There should not be two way of executing a wasm function. You should pick if you want to execute returned pointer to external function. Or having
execute(index).
Not sure why it's a big problem, I think it might be convenient and logical: you're calling fizzy_find_exported_function and getting a "function" that you can call.
Anyway, the solution in #771.
So is #771 in your view introducing a problem, that we should avoid?
There was a problem hiding this comment.
There should not be two way of executing a wasm function. You should pick if you want to execute returned pointer to external function. Or having
execute(index).Not sure why it's a big problem, I think it might be convenient and logical: you're calling
fizzy_find_exported_functionand getting a "function" that you can call.
I agree here, but then there should not be fizzy_execute(). I.e. you can only execute exported functions.
In practice, I prefer the default ExecuteContext creation to be done in single place than in multiple places.
Anyway, the solution in #771.
So is #771 in your view introducing a problem, that we should avoid?
No, it looks good. But there are some details there we need to discuss later.
There was a problem hiding this comment.
So this PR breaks one of the ways functions are called, #771 will restore it back, but you think in the future on of the ways should be removed?
| std::any host_ctx; | ||
| ExecutionContext execution_ctx; | ||
| EXPECT_THAT(host_fn_1(host_ctx, instance, nullptr, execution_ctx), Traps()); | ||
| EXPECT_THAT(host_fn_2(host_ctx, instance, nullptr, execution_ctx), Traps()); |
There was a problem hiding this comment.
Not sure why, but this test perhaps intentionally used two separate host contextes?
There was a problem hiding this comment.
Both of them were not initialized so they couldn't be used anyway.
|
Haswell 4.0 GHz, Clang LTO |
Instead of the call depth value.
Change int depth param to ExecutionContext& in execute()
chfast
force-pushed
the
execution_context
branch
from
5bb55bb
to
60ee89e
Compare
|
Why do we see a speed bump? |
chfast
force-pushed
the
execution_context
branch
from
adeafaf
to
185c9c0
Compare
include/fizzy/fizzy.h
Outdated
| /// @param instance Pointer to module instance. | ||
| /// @param args Pointer to the argument array. Can be NULL iff function has no inputs. | ||
| /// @param depth Call stack depth. | ||
| /// @param ctx Opaque pointer to execution context. Execution context cannot be created |
There was a problem hiding this comment.
Needs a note that it is allowed to be NULL.
chfast
force-pushed
the
execution_context
branch
from
48e08f5
to
6e8a0a5
Compare
| @@ -137,9 +137,16 @@ inline FizzyExternalFunction wrap(fizzy::ExternalFunction external_func) | |||
| { | |||
| static constexpr FizzyExternalFn c_function = | |||
| [](void* host_ctx, FizzyInstance* instance, const FizzyValue* args, | |||
| FizzyExecutionContext* ctx) -> FizzyExecutionResult { | |||
| FizzyExecutionContext* c_ctx) -> FizzyExecutionResult { | |||
| // If execution context not provided, allocate new one. | |||
There was a problem hiding this comment.
is not provided`
or could just say "Allocate new execution context if none provided."
No description provided.