Skip to content
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.

Sign up for GitHub

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

Refactor table and simplify calls #616

Merged
merged 2 commits into from
Nov 3, 2020
Merged

Refactor table and simplify calls #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c02ed94
refactor: represent table elements as instance pointer + function index
gumb0 Oct 21, 2020
77fb4db
refactor: Instead of optional for table elements, check instance is set
gumb0 Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 8 additions & 15 deletions lib/fizzy/execute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,15 +477,14 @@ void branch(const Code& code, OperandStack& stack, const uint8_t*& pc, const uin
stack.drop(stack_drop);
}

template <class F>
inline bool invoke_function(
const FuncType& func_type, const F& func, Instance& instance, OperandStack& stack, int depth)
inline bool invoke_function(const FuncType& func_type, uint32_t func_idx, Instance& instance,
OperandStack& stack, int depth)
{
const auto num_args = func_type.inputs.size();
assert(stack.size() >= num_args);
const auto call_args = stack.rend() - num_args;

const auto ret = func(instance, call_args, depth + 1);
const auto ret = execute(instance, func_idx, call_args, depth + 1);
// Bubble up traps
if (ret.trapped)
return false;
Expand All @@ -503,14 +502,6 @@ inline bool invoke_function(
return true;
}

inline bool invoke_function(const FuncType& func_type, uint32_t func_idx, Instance& instance,
OperandStack& stack, int depth)
{
const auto func = [func_idx](Instance& _instance, const Value* args, int _depth) {
return execute(_instance, func_idx, args, _depth);
};
return invoke_function(func_type, func, instance, stack, depth);
}
} // namespace

ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args, int depth)
Expand Down Expand Up @@ -630,16 +621,18 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
goto trap;

const auto called_func = (*instance.table)[elem_idx];
if (!called_func.has_value())
if (!called_func.instance) // Table element not initialized.
goto trap;

// check actual type against expected type
const auto& actual_type = called_func->type;
const auto& actual_type =
called_func.instance->module->get_function_type(called_func.func_idx);
const auto& expected_type = instance.module->typesec[expected_type_idx];
if (expected_type != actual_type)
goto trap;

if (!invoke_function(actual_type, called_func->function, instance, stack, depth))
if (!invoke_function(
actual_type, called_func.func_idx, *called_func.instance, stack, depth))
goto trap;
break;
}
Expand Down
14 changes: 3 additions & 11 deletions lib/fizzy/instantiate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,11 +330,7 @@ std::unique_ptr<Instance> instantiate(std::unique_ptr<const Module> module,
auto it_table = instance->table->begin() + elementsec_offsets[i];
for (const auto idx : instance->module->elementsec[i].init)
{
auto func = [idx, &instance_ref = *instance](fizzy::Instance&, const Value* args,
int depth) { return execute(instance_ref, idx, args, depth); };

*it_table++ =
ExternalFunction{std::move(func), instance->module->get_function_type(idx)};
*it_table++ = {instance.get(), idx, {}};
}
}

Expand All @@ -360,12 +356,8 @@ std::unique_ptr<Instance> instantiate(std::unique_ptr<const Module> module,
auto it_table = shared_instance->table->begin() + elementsec_offsets[i];
for ([[maybe_unused]] auto _ : shared_instance->module->elementsec[i].init)
{
// Wrap the function with the lambda capturing shared instance
auto& table_function = (*it_table)->function;
table_function = [shared_instance, func = std::move(table_function)](
fizzy::Instance& _instance, const Value* args,
int depth) { return func(_instance, args, depth); };
++it_table;
// Capture shared instance in table element.
(*it_table++).shared_instance = shared_instance;
}
}
}
Expand Down
15 changes: 14 additions & 1 deletion lib/fizzy/instantiate.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,20 @@ struct ExternalFunction
FuncType type;
};

using table_elements = std::vector<std::optional<ExternalFunction>>;
// Table element, which references a function in any instance.
struct TableElement
{
// Pointer to function's instance or nullptr when table element is not initialized.
Instance* instance = nullptr;
// Index of the function in instance.
FuncIdx func_idx = 0;
// This pointer is empty most of the time and is used only to keep instance alive in one edge
// case, when start function traps, but instantiate has already modified some elements of a
// shared (imported) table.
std::shared_ptr<Instance> shared_instance;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simple way to keep instance alive as long as there is function using it in some table. This was previously achieved by capturing this shared_ptr<Instance> in std::function of table element.

Now this looks like an overhead included in each table element every time, but needed only in weird edge case.
But on the other hand, previously there was std::function's dynamic allocation overhead for each table element all the time, and now this shared_ptr stays empty in normal cases, so this theoretically should be overall more efficient than before.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better now. But I'd still prefer to handle that differently (not necessarily in this PR): either separate start() from instantiate() or return tuble Instance*, bool from instantiate().

};

using table_elements = std::vector<TableElement>;
using table_ptr = std::unique_ptr<table_elements, void (*)(table_elements*)>;

struct ExternalTable
Expand Down
6 changes: 4 additions & 2 deletions test/unittests/api_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,10 @@ TEST(api, find_exported_table)
ASSERT_TRUE(opt_table);
EXPECT_EQ(opt_table->table, instance->table.get());
EXPECT_EQ(opt_table->table->size(), 2);
EXPECT_THAT((*opt_table->table)[0]->function(*instance, {}, 0), Result(2));
EXPECT_THAT((*opt_table->table)[1]->function(*instance, {}, 0), Result(1));
EXPECT_EQ((*opt_table->table)[0].instance, instance.get());
EXPECT_EQ((*opt_table->table)[0].func_idx, 1);
EXPECT_EQ((*opt_table->table)[1].instance, instance.get());
EXPECT_EQ((*opt_table->table)[1].func_idx, 0);
EXPECT_EQ(opt_table->limits.min, 2);
ASSERT_TRUE(opt_table->limits.max.has_value());
EXPECT_EQ(opt_table->limits.max, 20);
Expand Down
33 changes: 20 additions & 13 deletions test/unittests/execute_call_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,25 @@ TEST(execute_call, call_indirect_with_argument)

TEST(execute_call, call_indirect_imported_table)
{
/* wat2wasm
(module
(table 5 20 anyfunc)
(export "t" (table 0))
(elem (i32.const 0) $f3 $f2 $f1 $f4 $f5)
(func $f1 (result i32) (i32.const 1))
(func $f2 (result i32) (i32.const 2))
(func $f3 (result i32) (i32.const 3))
(func $f4 (result i64) (i64.const 4))
(func $f5 (result i32) unreachable)
)
*/
const auto bin_exported_table = from_hex(
"0061736d010000000109026000017f6000017e03060500000001000405017001051407050101740100090b0100"
"41000b0502010003040a1905040041010b040041020b040041030b040042040b0300000b");
auto instance_exported_table = instantiate(parse(bin_exported_table));
auto table = find_exported_table(*instance_exported_table, "t");
ASSERT_TRUE(table.has_value());

/* wat2wasm
(module
(type $out_i32 (func (result i32)))
Expand All @@ -171,19 +190,7 @@ TEST(execute_call, call_indirect_imported_table)
"0061736d01000000010a026000017f60017f017f020a01016d01740170010514030201010a0901070020001100"
"000b");

auto f1 = [](Instance&, const Value*, int) { return Value{1}; };
auto f2 = [](Instance&, const Value*, int) { return Value{2}; };
auto f3 = [](Instance&, const Value*, int) { return Value{3}; };
auto f4 = [](Instance&, const Value*, int) { return Value{4}; };
auto f5 = [](Instance&, const Value*, int) { return Trap; };

auto out_i32 = FuncType{{}, {ValType::i32}};
auto out_i64 = FuncType{{}, {ValType::i64}};

table_elements table{
{{f3, out_i32}}, {{f2, out_i32}}, {{f1, out_i32}}, {{f4, out_i64}}, {{f5, out_i32}}};

auto instance = instantiate(parse(bin), {}, {{&table, {5, 20}}});
auto instance = instantiate(parse(bin), {}, {*table});

for (const auto param : {0u, 1u, 2u})
{
Expand Down
77 changes: 50 additions & 27 deletions test/unittests/instantiate_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace
uint32_t call_table_func(Instance& instance, size_t idx)
{
const auto& elem = (*instance.table)[idx];
const auto res = elem->function(instance, {}, 0);
const auto res = execute(*elem.instance, elem.func_idx, {}, 0);
EXPECT_TRUE(res.has_value);
return as_uint32(res.value);
}
Expand Down Expand Up @@ -551,7 +551,7 @@ TEST(instantiate, element_section)
auto instance = instantiate(parse(bin));

ASSERT_EQ(instance->table->size(), 4);
EXPECT_FALSE((*instance->table)[0].has_value());
EXPECT_EQ((*instance->table)[0].instance, nullptr);
EXPECT_EQ(call_table_func(*instance, 1), 1);
EXPECT_EQ(call_table_func(*instance, 2), 3);
EXPECT_EQ(call_table_func(*instance, 3), 3);
Expand All @@ -576,10 +576,10 @@ TEST(instantiate, element_section_offset_from_global)
auto instance = instantiate(parse(bin));

ASSERT_EQ(instance->table->size(), 4);
EXPECT_FALSE((*instance->table)[0].has_value());
EXPECT_EQ((*instance->table)[0].instance, nullptr);
EXPECT_EQ(call_table_func(*instance, 1), 1);
EXPECT_EQ(call_table_func(*instance, 2), 2);
EXPECT_FALSE((*instance->table)[3].has_value());
EXPECT_EQ((*instance->table)[3].instance, nullptr);
}

TEST(instantiate, element_section_offset_from_imported_global)
Expand All @@ -601,10 +601,10 @@ TEST(instantiate, element_section_offset_from_imported_global)
auto instance = instantiate(parse(bin), {}, {}, {}, {g});

ASSERT_EQ(instance->table->size(), 4);
EXPECT_FALSE((*instance->table)[0].has_value());
EXPECT_EQ((*instance->table)[0].instance, nullptr);
EXPECT_EQ(call_table_func(*instance, 1), 1);
EXPECT_EQ(call_table_func(*instance, 2), 2);
EXPECT_FALSE((*instance->table)[3].has_value());
EXPECT_EQ((*instance->table)[3].instance, nullptr);
}

TEST(instantiate, element_section_offset_too_large)
Expand All @@ -622,6 +622,21 @@ TEST(instantiate, element_section_offset_too_large)

TEST(instantiate, element_section_fills_imported_table)
{
/* wat2wasm
(module
(table 4 anyfunc)
(export "t" (table 0))
(elem (i32.const 0) $f0)
(func $f0 (result i32) (i32.const 0))
)
*/
const auto bin_exported_table = from_hex(
"0061736d010000000105016000017f03020100040401700004070501017401000907010041000b01000a060104"
"0041000b");
auto instance_exported_table = instantiate(parse(bin_exported_table));
auto table = find_exported_table(*instance_exported_table, "t");
ASSERT_TRUE(table.has_value());

/* wat2wasm
(module
(table (import "m" "tab") 4 funcref)
Expand All @@ -637,11 +652,7 @@ TEST(instantiate, element_section_fills_imported_table)
"0061736d010000000105016000017f020b01016d037461620170000403050400000000090f020041010b020001"
"0041020b0202030a1504040041010b040041020b040041030b040041040b");

auto f0 = [](Instance&, const Value*, int) { return Value{0}; };

table_elements table(4);
table[0] = ExternalFunction{f0, FuncType{{}, {ValType::i32}}};
auto instance = instantiate(parse(bin), {}, {{&table, {4, std::nullopt}}});
auto instance = instantiate(parse(bin), {}, {*table});

ASSERT_EQ(instance->table->size(), 4);
EXPECT_EQ(call_table_func(*instance, 0), 0);
Expand All @@ -652,6 +663,21 @@ TEST(instantiate, element_section_fills_imported_table)

TEST(instantiate, element_section_out_of_bounds_doesnt_change_imported_table)
{
/* wat2wasm
(module
(table 3 anyfunc)
(export "t" (table 0))
(elem (i32.const 0) $f0)
(func $f0 (result i32) (i32.const 0))
)
*/
const auto bin_exported_table = from_hex(
"0061736d010000000105016000017f03020100040401700003070501017401000907010041000b01000a060104"
"0041000b");
auto instance_exported_table = instantiate(parse(bin_exported_table));
auto table = find_exported_table(*instance_exported_table, "t");
ASSERT_TRUE(table.has_value());

/* wat2wasm
(module
(table (import "m" "tab") 3 funcref)
Expand All @@ -664,18 +690,15 @@ TEST(instantiate, element_section_out_of_bounds_doesnt_change_imported_table)
"0061736d010000000105016000017f020b01016d037461620170000303020100090f020041000b020000004102"
"0b0200000a0601040041010b");

auto f0 = [](Instance&, const Value*, int) { return Value{0}; };

table_elements table(3);
table[0] = ExternalFunction{f0, FuncType{{}, {ValType::i32}}};

EXPECT_THROW_MESSAGE(instantiate(parse(bin), {}, {{&table, {3, std::nullopt}}}),
instantiate_error, "element segment is out of table bounds");
EXPECT_THROW_MESSAGE(instantiate(parse(bin), {}, {*table}), instantiate_error,
"element segment is out of table bounds");

ASSERT_EQ(table.size(), 3);
EXPECT_EQ(*table[0]->function.target<decltype(f0)>(), f0);
EXPECT_FALSE(table[1].has_value());
EXPECT_FALSE(table[2].has_value());
const auto& table_elements = *table->table;
ASSERT_EQ(table_elements.size(), 3);
EXPECT_EQ(table_elements[0].instance, instance_exported_table.get());
EXPECT_EQ(table_elements[0].func_idx, 0);
EXPECT_EQ(table_elements[1].instance, nullptr);
EXPECT_EQ(table_elements[2].instance, nullptr);
}

TEST(instantiate, data_section)
Expand Down Expand Up @@ -792,8 +815,8 @@ TEST(instantiate, data_elem_section_errors_dont_change_imports)
instantiate(parse(bin_data_error), {}, {{&table, {3, std::nullopt}}}, {{&memory, {1, 1}}}),
instantiate_error, "data segment is out of memory bounds");

EXPECT_FALSE(table[0].has_value());
EXPECT_FALSE(table[1].has_value());
EXPECT_EQ(table[0].instance, nullptr);
EXPECT_EQ(table[1].instance, nullptr);
EXPECT_EQ(memory[0], 0);

/* wat2wasm
Expand All @@ -814,9 +837,9 @@ TEST(instantiate, data_elem_section_errors_dont_change_imports)
instantiate(parse(bin_elem_error), {}, {{&table, {3, std::nullopt}}}, {{&memory, {1, 1}}}),
instantiate_error, "element segment is out of table bounds");

EXPECT_FALSE(table[0].has_value());
EXPECT_FALSE(table[1].has_value());
EXPECT_FALSE(table[2].has_value());
EXPECT_EQ(table[0].instance, nullptr);
EXPECT_EQ(table[1].instance, nullptr);
EXPECT_EQ(table[2].instance, nullptr);
EXPECT_EQ(memory[0], 0);
}

Expand Down