Introduce ThreadContext instead of execution depth #726

chfast · 2021-02-04T17:46:39Z

Replace depth param in execute() functions with ThreadContext&.
The ThreadContext encapsulates the depth control from now on and is
extensible. In future it can support metering and better stack space
allocation.

Call depth control semantic is preserved, except the depth is now passed by reference (aka int& depth) so additional context guard is needed in places where depth is being bumped.

In C the ThreadContext is opaque what is good for API, but it is impossible to dump the depth inside a C host function.

This is baseline for PoC thread context extensions proposed in #529 and #572. Rebasing these is impossible, but the rest of the changes from there should be re-applied later.

codecov · 2021-02-04T17:50:00Z

Codecov Report

Merging #726 (62cfb87) into master (5028558) will decrease coverage by 0.01%.
The diff coverage is 97.51%.

@@            Coverage Diff             @@
##           master     #726      +/-   ##
==========================================
- Coverage   99.34%   99.32%   -0.02%     
==========================================
  Files          73       72       -1     
  Lines       11142    10696     -446     
==========================================
- Hits        11069    10624     -445     
+ Misses         73       72       -1

Flag	Coverage Δ
rust	`?`
spectests	`90.88% <100.00%> (+0.03%)`	⬆️
unittests	`99.32% <97.51%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
lib/fizzy/instantiate.hpp	`100.00% <ø> (ø)`
tools/wasi/wasi.cpp	`57.30% <20.00%> (ø)`
lib/fizzy/capi.cpp	`98.93% <100.00%> (+0.01%)`	⬆️
lib/fizzy/execute.cpp	`100.00% <100.00%> (ø)`
lib/fizzy/execute.hpp	`100.00% <100.00%> (ø)`
lib/fizzy/instantiate.cpp	`100.00% <100.00%> (ø)`
test/unittests/api_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/capi_test.cpp	`99.87% <100.00%> (-0.01%)`	⬇️
test/unittests/execute_call_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_test.cpp	`100.00% <100.00%> (ø)`
... and 4 more

axic · 2021-02-04T20:39:44Z

lib/fizzy/instantiate.hpp

+class ThreadContext
+{
+public:
+    int depth = 0;


So there's no way to set a different starting depth?

You can ctx.depth = 1, as in tests.

Should we expose it? In any case if we it should be just a separate PR for the C API (and then Rust). It is fine to hide it in this PR.

In C the ThreadContext is opaque what is good for API, but it is impossible to dump the depth inside a C host function.

Well we would need C++ helpers for it first, right? :)

axic · 2021-02-04T20:40:55Z

lib/fizzy/instantiate.hpp

@@ -21,8 +21,31 @@ namespace fizzy
 struct ExecutionResult;
 struct Instance;

+class ThreadContext


Why do we go with the term thread? Is this future proofing for multithreaded wasm? Is some kind of locking planned, given the memory (to begin with) is still part of the instance and not the thread.

It has nothing to do with multithreading. I is just execution thread context (in principle, you can create multiple thread context and start multiple call stacks in parallel). I understand the name may be confusing although WABT uses the same name for this concept. We can consider alternative names.

I think it would be a good name, but given wasm multithreading this may be confusing. Would this be applicable during that? Would something like ExecutionContext be less confusing, or equally confusing as it is partial too?

lib/fizzy/instantiate.hpp

bindings/rust/src/lib.rs

axic · 2021-02-05T21:00:26Z

include/fizzy/fizzy.h

@@ -20,6 +20,8 @@ typedef struct FizzyModule FizzyModule;
 /// The opaque data type representing an instance (instantiated module).
 typedef struct FizzyInstance FizzyInstance;

+typedef struct FizzyThreadContext FizzyThreadContext;


Should have a comment, see FizzyInstance above.

axic · 2021-02-05T21:06:02Z

lib/fizzy/instantiate.hpp

+public:
+    int depth = 0;
+
+    void acquire() noexcept { ++depth; }


Do these need to be public? Are these supposed to be used directly or only via the Guard?

axic · 2021-02-05T21:06:35Z

lib/fizzy/instantiate.hpp

@@ -21,8 +21,31 @@ namespace fizzy
 struct ExecutionResult;
 struct Instance;

+class ThreadContext


Should have some level of documentaiton.

axic · 2021-02-05T21:07:00Z

lib/fizzy/execute.hpp

+    Instance& instance, FuncIdx func_idx, const Value* args, ThreadContext& ctx);
+
+/// Execute a function from an instance starting at default depth of 0.
+inline ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args)


Is this version used heavily in the tests?

axic · 2021-02-05T21:09:02Z

test/unittests/capi_test.cpp

@@ -318,7 +318,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));
+
+    // FIXME: thread context missing.


This should be fixed before merging I guess?

This is not fixable if ThreadContext to stay opaque in C.

axic · 2021-02-05T21:13:30Z

Looks great in general!

axic · 2021-02-05T21:46:09Z

I think there should be a benchmark run after the comments are addressed and before this is merged, to see what is the effect, if any.

Replace depth param in execute() functions with ThreadContext&. The ThreadContext encapsulates the depth control from now on and is extensible. In future it can support metering and better stack space allocation. Call depth control semantic is preserved, except the depth is now passed by reference (aka `int& depth`) so additional context guard is needed in places where depth is being bumped.

gumb0 · 2021-02-09T11:44:30Z

lib/fizzy/execute.hpp

+public:
+    int depth = 0;
+
+    Guard bump_call_depth() noexcept


Hm, this returns private class, I wouldn't think it's possible...

Maybe call it increment_call_depth, bump sounds a bit informal it seems.

Just an idea: it could be execute member function of ThreadContext bumping depth and redirecting to normal execute

ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args) noexcept;

then Guard can be completely hidden from user.

But maybe confusingly too many execute variants then.

gumb0 · 2021-02-09T13:44:27Z

test/unittests/execute_call_test.cpp

-    auto sub = [&instance1, func_idx](Instance&, const Value* args, int depth) -> ExecutionResult {
-        return fizzy::execute(*instance1, *func_idx, args, depth);
+    auto sub = [&instance1, func_idx](Instance&, const Value* args, ThreadContext& ctx) {
+        // ThreadContext is bumped here.


I was a bit confused by this comment. I think you mean at this point it was already bumped by the caller?

Probably should be "is not bumped". And it should be according to the current semantic.

chfast · 2021-02-10T13:38:07Z

I will rename it to ExecutionContext.

gumb0 · 2021-02-10T13:44:20Z

I will rename it to ExecutionContext.

What I don't like about ExecutionContext is that void* context in C API also can be thought of as "execution context". But I don't have a better name.

chfast · 2021-03-23T13:09:01Z

Replaced by #766.

chfast force-pushed the thread_context branch 3 times, most recently from 4aa51c4 to 7d287f0 Compare February 4, 2021 20:06

chfast requested review from gumb0 and axic and removed request for gumb0 February 4, 2021 20:15

axic reviewed Feb 4, 2021

View reviewed changes

chfast requested a review from gumb0 February 5, 2021 09:05

gumb0 reviewed Feb 5, 2021

View reviewed changes

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

axic reviewed Feb 5, 2021

View reviewed changes

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved

axic reviewed Feb 5, 2021

View reviewed changes

chfast force-pushed the thread_context branch from 7d287f0 to b07f1f7 Compare February 8, 2021 08:39

chfast added 8 commits February 8, 2021 09:43

Drop default value for depth parameter in execute()

7f25e52

capi: Drop depth param from fizzy_execute()

2f5c0f8

rust: Drop depth param from execute() functions

58bea12

test: Drop execute helpers with non-default depth

04ac27b

Add ThreadContext

dee285e

Alternative implementation of depth bump in ThreadContext

e0a6dfd

Move ThreadContext to execute.hpp

62cfb87

chfast force-pushed the thread_context branch from b07f1f7 to 62cfb87 Compare February 8, 2021 08:43

gumb0 reviewed Feb 9, 2021

View reviewed changes

chfast closed this Mar 23, 2021

chfast deleted the thread_context branch March 23, 2021 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ThreadContext instead of execution depth #726

Introduce ThreadContext instead of execution depth #726

chfast commented Feb 4, 2021 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading

axic Feb 4, 2021

chfast Feb 5, 2021

axic Feb 5, 2021

chfast Feb 8, 2021

axic Feb 8, 2021

axic Feb 4, 2021

chfast Feb 5, 2021

axic Feb 5, 2021

axic Feb 5, 2021

axic Feb 5, 2021

axic Feb 5, 2021

axic Feb 5, 2021

chfast Feb 8, 2021

axic Feb 5, 2021

chfast Feb 8, 2021

axic commented Feb 5, 2021

axic commented Feb 5, 2021

gumb0 Feb 9, 2021

gumb0 Feb 9, 2021

gumb0 Feb 9, 2021

gumb0 Feb 9, 2021

chfast Feb 9, 2021

chfast commented Feb 10, 2021

gumb0 commented Feb 10, 2021

chfast commented Mar 23, 2021

Introduce ThreadContext instead of execution depth #726

Introduce ThreadContext instead of execution depth #726

Conversation

chfast commented Feb 4, 2021 • edited Loading

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

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

axic commented Feb 5, 2021

axic commented Feb 5, 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

chfast commented Feb 10, 2021

gumb0 commented Feb 10, 2021

chfast commented Mar 23, 2021

chfast commented Feb 4, 2021 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading