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.

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

Remove recursion in binary writer #3

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

Harrm
Copy link
Collaborator

@Harrm Harrm commented Dec 13, 2024

Make WriteExpr non-recursive, since recursion there overflows stack.

@Harrm Harrm marked this pull request as ready for review December 13, 2024 11:57
@Harrm Harrm self-assigned this Dec 13, 2024
@Harrm Harrm requested review from turuslan and iceseer December 13, 2024 12:00
Copy link
Collaborator

@turuslan turuslan left a comment

Choose a reason for hiding this comment

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

Other possible implementations (for reference):


You made good optimization by moving (BinaryWriter, Func) from Coro{BinaryWriter, Func, ...}.resume() to Coro{...}.resume(BinaryWriter, Func).
Your coroutines only work with single BinaryWriter and Func instance, so they can be removed from Coro frame.

src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Show resolved Hide resolved
Comment on lines 477 to 479
void WriteExpr(const Func* func, const Expr* expr);
std::optional<StateVar> WriteExprImpl(const Func* func, const Expr* expr);
void WriteExprList(const Func* func, const ExprList& exprs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void WriteExpr(const Func* func, const Expr* expr);
std::optional<StateVar> WriteExprImpl(const Func* func, const Expr* expr);
void WriteExprList(const Func* func, const ExprList& exprs);
void WriteExpr(Deque &stack, const Func* func, const Expr* expr);
void WriteExprList(Deque &stack, const Func* func, const ExprList& exprs);
void WriteExprListImpl(const Func* func, const ExprList& exprs);

Comment on lines 1292 to 1313
void BinaryWriter::WriteExpr(const Func* func, const Expr* top_expr) {
std::list<StateVar> stack;

auto write_expr = [&stack, func, this](const Expr* expr) {
auto opt_state = WriteExprImpl(func, expr);
if (opt_state.has_value()) {
auto& state = *opt_state;
stack.push_back(state);
}
};
write_expr(top_expr);

while (!stack.empty()) {
State* state =
&std::visit([](auto& state) -> State& { return state; }, stack.back());
auto expr = state->advance(*this, func);
if (expr) {
write_expr(expr.value());
} else {
stack.pop_back();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

(WriteInitExpr | WriteFunc) -> WriteExprList -> (WriteExpr | WriteExprList)*

Suggested change
void BinaryWriter::WriteExpr(const Func* func, const Expr* top_expr) {
std::list<StateVar> stack;
auto write_expr = [&stack, func, this](const Expr* expr) {
auto opt_state = WriteExprImpl(func, expr);
if (opt_state.has_value()) {
auto& state = *opt_state;
stack.push_back(state);
}
};
write_expr(top_expr);
while (!stack.empty()) {
State* state =
&std::visit([](auto& state) -> State& { return state; }, stack.back());
auto expr = state->advance(*this, func);
if (expr) {
write_expr(expr.value());
} else {
stack.pop_back();
}
}
void BinaryWriter::WriteExprListImpl(const Func* func, const ExprList& exprs) {
std::deque<StateVar> stack;
WriteExprList(stack, func, exprs);
while (not stack.empty()) {
auto expr = std::visit([&](auto& state) { return state.advance(*this, func); }, stack.back());
if (expr) {
WriteExpr(stack, expr.value());
} else {
stack.pop_back();
}
}

@@ -857,13 +1039,15 @@ void BinaryWriter::WriteExpr(const Func* func, const Expr* expr) {
auto* if_expr = cast<IfExpr>(expr);
WriteOpcode(stream_, Opcode::If);
WriteBlockDecl(if_expr->true_.decl);
WriteExprList(func, if_expr->true_.exprs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another possible implementation may use stack<queue<variant<Opcode, ExprList *, U32Leb128>>>,
rewriting like

queue = stack.push([])
queue.push(if.then) // co_await, waits for recursion
if (if.else)
  queue.push(opcode.else) // after co_await, enqueue
  queue.push(if.else)
queue.push(opcode.end)
return // unwind

Comment on lines 438 to 439
bool in_catch = false;
size_t current_catch = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool in_catch = false;
size_t current_catch = 0;
bool setCatch(size_t i) {
i_catch = i;
if (i >= expr->catches.size()) {
return;
}
current_expr = expr->catches[i].exprs.begin();
}
std::optional<size_t> i_catch;

Comment on lines 582 to 644
if (!in_catch) {
if (current_expr != expr->block.exprs.end()) {
auto res = &*current_expr;
current_expr++;
return res;
} else {
switch (expr->kind) {
case TryKind::Catch:
in_catch = true;
break;
case TryKind::Delegate:
WriteOpcode(writer.stream_, Opcode::Delegate);
WriteU32Leb128(writer.stream_,
writer.GetLabelVarDepth(&expr->delegate_target),
"delegate depth");
return std::nullopt;
case TryKind::Plain:
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
}
}
// deliberately no else
if (in_catch) {
if (current_catch < expr->catches.size()) {
auto& catch_block = expr->catches[current_catch];
if (current_expr != catch_block.exprs.end()) {
if (current_expr == catch_block.exprs.begin()) {
if (catch_block.IsCatchAll()) {
WriteOpcode(writer.stream_, Opcode::CatchAll);
} else {
WriteOpcode(writer.stream_, Opcode::Catch);
WriteU32Leb128(writer.stream_,
writer.GetTagVarDepth(&catch_block.var),
"catch tag");
}
}
auto res = &*current_expr;
current_expr++;
return res;
} else {
do {
current_catch++;
} while (current_catch < expr->catches.size() &&
expr->catches[current_catch].exprs.empty());
if (current_catch < expr->catches.size()) {
auto& catch_block = expr->catches[current_catch];
current_expr = catch_block.exprs.begin();
auto res = &*current_expr;
current_expr++;
return res;

} else {
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
}
} else {
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
}
WABT_UNREACHABLE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!in_catch) {
if (current_expr != expr->block.exprs.end()) {
auto res = &*current_expr;
current_expr++;
return res;
} else {
switch (expr->kind) {
case TryKind::Catch:
in_catch = true;
break;
case TryKind::Delegate:
WriteOpcode(writer.stream_, Opcode::Delegate);
WriteU32Leb128(writer.stream_,
writer.GetLabelVarDepth(&expr->delegate_target),
"delegate depth");
return std::nullopt;
case TryKind::Plain:
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
}
}
// deliberately no else
if (in_catch) {
if (current_catch < expr->catches.size()) {
auto& catch_block = expr->catches[current_catch];
if (current_expr != catch_block.exprs.end()) {
if (current_expr == catch_block.exprs.begin()) {
if (catch_block.IsCatchAll()) {
WriteOpcode(writer.stream_, Opcode::CatchAll);
} else {
WriteOpcode(writer.stream_, Opcode::Catch);
WriteU32Leb128(writer.stream_,
writer.GetTagVarDepth(&catch_block.var),
"catch tag");
}
}
auto res = &*current_expr;
current_expr++;
return res;
} else {
do {
current_catch++;
} while (current_catch < expr->catches.size() &&
expr->catches[current_catch].exprs.empty());
if (current_catch < expr->catches.size()) {
auto& catch_block = expr->catches[current_catch];
current_expr = catch_block.exprs.begin();
auto res = &*current_expr;
current_expr++;
return res;
} else {
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
}
} else {
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
}
WABT_UNREACHABLE;
if (not i_catch) {
if (current_expr != expr->block.exprs.end()) {
return next();
}
switch (expr->kind) {
case TryKind::Catch:
break;
case TryKind::Delegate:
WriteOpcode(writer.stream_, Opcode::Delegate);
WriteU32Leb128(writer.stream_,
writer.GetLabelVarDepth(&expr->delegate_target),
"delegate depth");
return std::nullopt;
case TryKind::Plain:
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
setCatch(0);
}
while (true) {
if (*i_catch >= expr->catches.size()) {
WriteOpcode(writer.stream_, Opcode::End);
return std::nullopt;
}
auto& catch_block = expr->catches[*i_catch];
if (current_expr == catch_block.exprs.begin()) {
if (catch_block.IsCatchAll()) {
WriteOpcode(writer.stream_, Opcode::CatchAll);
} else {
WriteOpcode(writer.stream_, Opcode::Catch);
WriteU32Leb128(writer.stream_,
writer.GetTagVarDepth(&catch_block.var),
"catch tag");
}
}
if (current_expr != catch_block.exprs.end()) {
return next();
}
setCatch(*i_catch + 1);
}

src/binary-writer.cc Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
src/binary-writer.cc Show resolved Hide resolved
src/binary-writer.cc Show resolved Hide resolved
src/binary-writer.cc Show resolved Hide resolved
src/binary-writer.cc Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants