Skip to content

Refactor short-circuit planning logic. #2139

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

Open
copybara-service wants to merge 1 commit into
base: master
Choose a base branch
from
+81 −125
Open

Refactor short-circuit planning logic. #2139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
206 changes: 81 additions & 125 deletions eval/compiler/flat_expr_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,7 @@ class CondVisitor {
virtual void PostVisitTarget(const cel::Expr* expr) {}
};

enum class BinaryCond {
kAnd = 0,
kOr,
kOptionalOr,
kOptionalOrValue,
};

// Visitor managing the "&&" and "||" operatiions.
// Visitor managing the "&&" and "||" (boolean logic) operations.
// Implements short-circuiting if enabled.
//
// With short-circuiting enabled, generates a program like:
Expand All @@ -218,20 +211,41 @@ enum class BinaryCond {
// | i + 3 | BooleanOperator | Op(arg1, arg2) |
// | i + 4 | <rest of program> | arg1 | Op(arg1, arg2) |
// +-------------+------------------------+------------------------+
class BinaryCondVisitor : public CondVisitor {
class LogicalCondVisitor : public CondVisitor {
public:
explicit BinaryCondVisitor(FlatExprVisitor* visitor, BinaryCond cond,
bool short_circuiting)
: visitor_(visitor), cond_(cond), short_circuiting_(short_circuiting) {}
explicit LogicalCondVisitor(FlatExprVisitor* visitor, bool is_or,
bool short_circuiting)
: visitor_(visitor), is_or_(is_or), short_circuiting_(short_circuiting) {}

void PreVisit(const cel::Expr* expr) override;
void PostVisitArg(int arg_num, const cel::Expr* expr) override;
void PostVisit(const cel::Expr* expr) override;

private:
FlatExprVisitor* visitor_;
const bool is_or_;
std::vector<Jump> jump_steps_;
bool short_circuiting_;
};

// Visitor managing optional "or" and "orValue" operations.
// Implements short-circuiting if enabled.
class OptionalOrCondVisitor : public CondVisitor {
public:
explicit OptionalOrCondVisitor(FlatExprVisitor* visitor, bool is_or_value,
bool short_circuiting)
: visitor_(visitor),
is_or_value_(is_or_value),
short_circuiting_(short_circuiting) {}

void PreVisit(const cel::Expr* expr) override;
void PostVisitArg(int arg_num, const cel::Expr* expr) override {}
void PostVisitTarget(const cel::Expr* expr) override;
void PostVisit(const cel::Expr* expr) override;

private:
FlatExprVisitor* visitor_;
const BinaryCond cond_;
const bool is_or_value_;
std::vector<Jump> jump_steps_;
bool short_circuiting_;
};
Expand Down Expand Up @@ -997,11 +1011,11 @@ class FlatExprVisitor : public cel::AstVisitor {

std::unique_ptr<CondVisitor> cond_visitor;
if (call_expr.function() == cel::builtin::kAnd) {
cond_visitor = std::make_unique<BinaryCondVisitor>(
this, BinaryCond::kAnd, options_.short_circuiting);
cond_visitor = std::make_unique<LogicalCondVisitor>(
this, /*is_or=*/false, options_.short_circuiting);
} else if (call_expr.function() == cel::builtin::kOr) {
cond_visitor = std::make_unique<BinaryCondVisitor>(
this, BinaryCond::kOr, options_.short_circuiting);
cond_visitor = std::make_unique<LogicalCondVisitor>(
this, /*is_or=*/true, options_.short_circuiting);
} else if (call_expr.function() == cel::builtin::kTernary) {
if (options_.short_circuiting) {
cond_visitor = std::make_unique<TernaryCondVisitor>(this);
Expand All @@ -1011,13 +1025,13 @@ class FlatExprVisitor : public cel::AstVisitor {
} else if (enable_optional_types_ &&
call_expr.function() == kOptionalOrFn &&
call_expr.has_target() && call_expr.args().size() == 1) {
cond_visitor = std::make_unique<BinaryCondVisitor>(
this, BinaryCond::kOptionalOr, options_.short_circuiting);
cond_visitor = std::make_unique<OptionalOrCondVisitor>(
this, /*is_or_value=*/false, options_.short_circuiting);
} else if (enable_optional_types_ &&
call_expr.function() == kOptionalOrValueFn &&
call_expr.has_target() && call_expr.args().size() == 1) {
cond_visitor = std::make_unique<BinaryCondVisitor>(
this, BinaryCond::kOptionalOrValue, options_.short_circuiting);
cond_visitor = std::make_unique<OptionalOrCondVisitor>(
this, /*is_or_value=*/true, options_.short_circuiting);
} else if (IsBlock(&call_expr)) {
// cel.@block
if (block_.has_value()) {
Expand Down Expand Up @@ -2147,91 +2161,64 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr,
return CallHandlerResult::kIntercepted;
}

void BinaryCondVisitor::PreVisit(const cel::Expr* expr) {
switch (cond_) {
case BinaryCond::kAnd:
ABSL_FALLTHROUGH_INTENDED;
case BinaryCond::kOr:
visitor_->ValidateOrError(
!expr->call_expr().has_target() &&
expr->call_expr().args().size() >= 2,
"Invalid argument count for a binary function call.");
break;
case BinaryCond::kOptionalOr:
ABSL_FALLTHROUGH_INTENDED;
case BinaryCond::kOptionalOrValue:
visitor_->ValidateOrError(expr->call_expr().has_target() &&
expr->call_expr().args().size() == 1,
"Invalid argument count for or/orValue call.");
break;
}
void LogicalCondVisitor::PreVisit(const cel::Expr* expr) {
visitor_->ValidateOrError(
!expr->call_expr().has_target() && expr->call_expr().args().size() >= 2,
"Invalid argument count for a binary function call.");
}

void BinaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) {
void LogicalCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) {
if (visitor_->PlanRecursiveProgram()) {
return;
}
const int last_arg_index = expr->call_expr().args().size() - 1;
if (cond_ == BinaryCond::kAnd || cond_ == BinaryCond::kOr) {
if (arg_num > 0) {
switch (cond_) {
case BinaryCond::kAnd:
visitor_->AddStep(CreateAndStep(expr->id()));
break;
case BinaryCond::kOr:
visitor_->AddStep(CreateOrStep(expr->id()));
break;
default:
break;
}
if (short_circuiting_ && !jump_steps_.empty()) {
visitor_->SetProgressStatusIfError(
jump_steps_.back().set_target(visitor_->GetCurrentIndex()));
}
if (arg_num > 0) {
if (is_or_) {
visitor_->AddStep(CreateOrStep(expr->id()));
} else {
visitor_->AddStep(CreateAndStep(expr->id()));
}
if (short_circuiting_ && arg_num < last_arg_index) {
std::unique_ptr<JumpStepBase> jump_step;
switch (cond_) {
case BinaryCond::kAnd:
jump_step = CreateCondJumpStep(false, {}, expr->id());
break;
case BinaryCond::kOr:
jump_step = CreateCondJumpStep(true, {}, expr->id());
break;
default:
ABSL_UNREACHABLE();
}
ProgramStepIndex index = visitor_->GetCurrentIndex();
if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step));
jump_step_ptr) {
jump_steps_.push_back(Jump(index, jump_step_ptr));
}
if (short_circuiting_ && !jump_steps_.empty()) {
visitor_->SetProgressStatusIfError(
jump_steps_.back().set_target(visitor_->GetCurrentIndex()));
}
}
if (short_circuiting_ && arg_num < last_arg_index) {
std::unique_ptr<JumpStepBase> jump_step =
is_or_ ? CreateCondJumpStep(true, {}, expr->id())
: CreateCondJumpStep(false, {}, expr->id());
ProgramStepIndex index = visitor_->GetCurrentIndex();
if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step));
jump_step_ptr) {
jump_steps_.push_back(Jump(index, jump_step_ptr));
}
}
}

void LogicalCondVisitor::PostVisit(const cel::Expr* expr) {
if (visitor_->PlanRecursiveProgram()) {
visitor_->MakeShortcircuitRecursive(expr, is_or_);
}
}

void OptionalOrCondVisitor::PreVisit(const cel::Expr* expr) {
visitor_->ValidateOrError(
expr->call_expr().has_target() && expr->call_expr().args().size() == 1,
"Invalid argument count for or/orValue call.");
}

void BinaryCondVisitor::PostVisitTarget(const cel::Expr* expr) {
void OptionalOrCondVisitor::PostVisitTarget(const cel::Expr* expr) {
if (visitor_->PlanRecursiveProgram()) {
return;
}
if (short_circuiting_ && (cond_ == BinaryCond::kOptionalOr ||
cond_ == BinaryCond::kOptionalOrValue)) {
if (short_circuiting_) {
// If first branch evaluation result is enough to determine output,
// jump over the second branch and provide result of the first argument as
// final output.
// Retain a pointer to the jump step so we can update the target after
// planning the second argument.
std::unique_ptr<JumpStepBase> jump_step;
switch (cond_) {
case BinaryCond::kOptionalOr:
jump_step = CreateOptionalHasValueJumpStep(false, expr->id());
break;
case BinaryCond::kOptionalOrValue:
jump_step = CreateOptionalHasValueJumpStep(true, expr->id());
break;
default:
ABSL_UNREACHABLE();
}
std::unique_ptr<JumpStepBase> jump_step =
CreateOptionalHasValueJumpStep(is_or_value_, expr->id());
ProgramStepIndex index = visitor_->GetCurrentIndex();
if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step));
jump_step_ptr) {
Expand All @@ -2240,48 +2227,17 @@ void BinaryCondVisitor::PostVisitTarget(const cel::Expr* expr) {
}
}

void BinaryCondVisitor::PostVisit(const cel::Expr* expr) {
void OptionalOrCondVisitor::PostVisit(const cel::Expr* expr) {
if (visitor_->PlanRecursiveProgram()) {
switch (cond_) {
case BinaryCond::kAnd:
visitor_->MakeShortcircuitRecursive(expr, /*is_or=*/false);
break;
case BinaryCond::kOr:
visitor_->MakeShortcircuitRecursive(expr, /*is_or=*/true);
break;
case BinaryCond::kOptionalOr:
visitor_->MakeOptionalShortcircuit(expr,
/*is_or_value=*/false);
break;
case BinaryCond::kOptionalOrValue:
visitor_->MakeOptionalShortcircuit(expr,
/*is_or_value=*/true);
break;
default:
ABSL_UNREACHABLE();
}
visitor_->MakeOptionalShortcircuit(expr, is_or_value_);
return;
}

if (cond_ == BinaryCond::kOptionalOr ||
cond_ == BinaryCond::kOptionalOrValue) {
switch (cond_) {
case BinaryCond::kOptionalOr:
visitor_->AddStep(
CreateOptionalOrStep(/*is_or_value=*/false, expr->id()));
break;
case BinaryCond::kOptionalOrValue:
visitor_->AddStep(
CreateOptionalOrStep(/*is_or_value=*/true, expr->id()));
break;
default:
ABSL_UNREACHABLE();
}
if (short_circuiting_) {
for (auto& jump : jump_steps_) {
visitor_->SetProgressStatusIfError(
jump.set_target(visitor_->GetCurrentIndex()));
}
visitor_->AddStep(CreateOptionalOrStep(is_or_value_, expr->id()));
if (short_circuiting_) {
for (auto& jump : jump_steps_) {
visitor_->SetProgressStatusIfError(
jump.set_target(visitor_->GetCurrentIndex()));
}
}
}
Expand Down
Loading