Skip to content

Commit

Permalink
Update evaluator to assume lazy bind evaluation is always enabled. Th…
Browse files Browse the repository at this point in the history 
…is has been enabled by default for several weeks.

PiperOrigin-RevId: 602563427
  • Loading branch information
@jnthntatum @copybara-github
jnthntatum authored and copybara-github committed Jan 31, 2024
1 parent 8121008 commit 4393131
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 63 deletions.
35 changes: 7 additions & 28 deletions eval/compiler/flat_expr_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
suppressed_branches_.find(expr) != suppressed_branches_.end()) {
resume_from_suppressed_branch_ = expr;
}
if (!ProgramStructureTrackingEnabled()) {
return;
}

PlannerContext::ProgramInfo& info = program_tree_[expr];
info.range_start = GetCurrentIndex();
info.parent = parent_expr_;
Expand All @@ -369,9 +367,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
if (expr == resume_from_suppressed_branch_) {
resume_from_suppressed_branch_ = nullptr;
}
if (!ProgramStructureTrackingEnabled()) {
return;
}

PlannerContext::ProgramInfo& info = program_tree_[expr];
info.range_len = GetCurrentIndex() - info.range_start;
parent_expr_ = info.parent;
Expand All @@ -384,8 +380,8 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
return;
}
}
if (options_.enable_lazy_bind_initialization &&
!comprehension_stack_.empty() &&
if (!comprehension_stack_.empty() &&
comprehension_stack_.back().is_optimizable_bind &&
(&comprehension_stack_.back().comprehension->accu_init() == expr)) {
SetProgressStatusError(
MaybeExtractSubexpression(expr, comprehension_stack_.back()));
Expand Down Expand Up @@ -429,7 +425,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
record.comprehension->accu_var() == path) {
int slot = record.accu_slot;
int subexpression = -1;
if (record.should_lazy_eval) {
if (record.is_optimizable_bind) {
subexpression = record.subexpression;
}
return {slot, subexpression};
Expand Down Expand Up @@ -735,7 +731,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
// critical path (which is unknown at plan time), so the used slots need to
// be dedicated for the entire scope of that bind.
for (ComprehensionStackRecord& record : comprehension_stack_) {
if (record.in_accu_init && record.should_lazy_eval) {
if (record.in_accu_init && record.is_optimizable_bind) {
record.slot_count += slot_count;
slot_count = 0;
break;
Expand All @@ -752,8 +748,6 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
/*.iter_var_in_scope=*/false,
/*.accu_var_in_scope=*/false,
/*.in_accu_init=*/false,
/*.should_lazy_eval=*/is_bind &&
options_.enable_lazy_bind_initialization,
std::make_unique<ComprehensionVisitor>(
this, options_.short_circuiting, is_bind, iter_slot, accu_slot)});
comprehension_stack_.back().visitor->PreVisit(expr);
Expand Down Expand Up @@ -1003,31 +997,16 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
bool iter_var_in_scope;
bool accu_var_in_scope;
bool in_accu_init;
bool should_lazy_eval;
std::unique_ptr<ComprehensionVisitor> visitor;
};

bool PlanningSuppressed() const {
return resume_from_suppressed_branch_ != nullptr;
}

bool ProgramStructureTrackingEnabled() {
return options_.enable_lazy_bind_initialization ||
!program_optimizers_.empty();
}

bool InBindScope() {
for (const auto& record : comprehension_stack_) {
if (record.is_optimizable_bind) {
return true;
}
}
return false;
}

absl::Status MaybeExtractSubexpression(const cel::ast_internal::Expr* expr,
ComprehensionStackRecord& record) {
if (!record.should_lazy_eval) {
if (!record.is_optimizable_bind) {
return absl::OkStatus();
}
CEL_ASSIGN_OR_RETURN(auto subexpr,
Expand Down
10 changes: 2 additions & 8 deletions eval/public/cel_options.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,8 @@ struct InterpreterOptions {

// Enable lazy cel.bind alias initialization.
//
// When enabled, cel.bind definition subexpression will be evaluated when the
// evaluator first requires the alias in the subexpression using the bound
// alias. When disabled, the cel.bind definition is eagerly evaluated.
//
// This prevents eagerly evaluating the definition subexpression if it is
// never used in the result subexpression of the cel.bind() macro. This allows
// for consistent behavior for CEL compiler optimized expressions that extract
// subexpressions to cel.bind calls.
// This is now always enabled. Setting this option has no effect. It will be
// removed in a later update.
bool enable_lazy_bind_initialization = true;
};
// LINT.ThenChange(//depot/google3/runtime/runtime_options.h)
Expand Down
2 changes: 0 additions & 2 deletions extensions/bindings_ext_benchmark_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ TEST_P(BindingsBenchmarkTest, CheckBenchmarkCaseWorks) {
auto expr, ParseWithMacros(benchmark.expression, all_macros, "<input>"));

InterpreterOptions options;
options.enable_lazy_bind_initialization = true;
auto builder =
google::api::expr::runtime::CreateCelExpressionBuilder(options);

Expand All @@ -185,7 +184,6 @@ void RunBenchmark(const BenchmarkCase& benchmark, benchmark::State& state) {
auto expr, ParseWithMacros(benchmark.expression, all_macros, "<input>"));

InterpreterOptions options;
options.enable_lazy_bind_initialization = true;
auto builder =
google::api::expr::runtime::CreateCelExpressionBuilder(options);

Expand Down
22 changes: 5 additions & 17 deletions extensions/bindings_ext_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,10 @@ std::unique_ptr<CelFunction> CreateBindFunction() {
}

class BindingsExtTest
: public testing::TestWithParam<std::tuple<TestInfo, bool, bool>> {
: public testing::TestWithParam<std::tuple<TestInfo, bool>> {
protected:
const TestInfo& GetTestInfo() { return std::get<0>(GetParam()); }
bool GetEnableConstantFolding() { return std::get<1>(GetParam()); }
bool GetLazyBindings() { return std::get<2>(GetParam()); }
};

TEST_P(BindingsExtTest, EndToEnd) {
Expand Down Expand Up @@ -122,7 +121,6 @@ TEST_P(BindingsExtTest, EndToEnd) {
options.enable_empty_wrapper_null_unboxing = true;
options.constant_folding = GetEnableConstantFolding();
options.constant_arena = &arena;
options.enable_lazy_bind_initialization = GetLazyBindings();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
ASSERT_OK(builder->GetRegistry()->Register(CreateBindFunction()));
Expand Down Expand Up @@ -171,8 +169,7 @@ INSTANTIATE_TEST_SUITE_P(
// Error case where the variable name is not a simple identifier.
{"cel.bind(bad.name, true, bad.name)",
"variable name must be a simple identifier"}}),
/*constant_folding*/ testing::Bool(),
/*lazy_bindings*/ testing::Bool()));
/*constant_folding*/ testing::Bool()));

// Test bind expression with nested field selection.
//
Expand Down Expand Up @@ -445,19 +442,16 @@ constexpr absl::string_view kFieldSelectTestExpr = R"pb(
}
})pb";

class BindingsExtInteractionsTest
: public testing::TestWithParam<std::tuple<bool, bool>> {
class BindingsExtInteractionsTest : public testing::TestWithParam<bool> {
protected:
bool GetEnableLazyBinding() { return std::get<0>(GetParam()); }
bool GetEnableSelectOptimization() { return std::get<1>(GetParam()); }
bool GetEnableSelectOptimization() { return GetParam(); }
};

TEST_P(BindingsExtInteractionsTest, SelectOptimization) {
CheckedExpr expr;
ASSERT_TRUE(TextFormat::ParseFromString(kFieldSelectTestExpr, &expr));
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -489,7 +483,6 @@ TEST_P(BindingsExtInteractionsTest, UnknownAttributesSelectOptimization) {
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.unknown_processing = UnknownProcessingOptions::kAttributeOnly;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -528,7 +521,6 @@ TEST_P(BindingsExtInteractionsTest,
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.unknown_processing = UnknownProcessingOptions::kAttributeOnly;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -570,7 +562,6 @@ TEST_P(BindingsExtInteractionsTest, MissingAttributesSelectOptimization) {
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.enable_missing_attribute_errors = true;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -618,7 +609,6 @@ TEST_P(BindingsExtInteractionsTest, UnknownAttribute) {
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.unknown_processing = UnknownProcessingOptions::kAttributeOnly;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -665,7 +655,6 @@ TEST_P(BindingsExtInteractionsTest, UnknownAttributeReturnValue) {
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.unknown_processing = UnknownProcessingOptions::kAttributeOnly;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -716,7 +705,6 @@ TEST_P(BindingsExtInteractionsTest, MissingAttribute) {
InterpreterOptions options;
options.enable_empty_wrapper_null_unboxing = true;
options.enable_missing_attribute_errors = true;
options.enable_lazy_bind_initialization = GetEnableLazyBinding();
options.enable_select_optimization = GetEnableSelectOptimization();
std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
Expand Down Expand Up @@ -750,7 +738,7 @@ TEST_P(BindingsExtInteractionsTest, MissingAttribute) {

INSTANTIATE_TEST_SUITE_P(BindingsExtInteractionsTest,
BindingsExtInteractionsTest,
testing::Combine(testing::Bool(), testing::Bool()));
/*enable_select_optimization=*/testing::Bool());

} // namespace
} // namespace cel::extensions
10 changes: 2 additions & 8 deletions runtime/runtime_options.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,8 @@ struct RuntimeOptions {

// Enable lazy cel.bind alias initialization.
//
// When enabled, cel.bind definition subexpression will be evaluated when the
// evaluator first requires the alias in the subexpression using the bound
// alias. When disabled, the cel.bind definition is eagerly evaluated.
//
// This prevents eagerly evaluating the definition subexpression if it is
// never used in the result subexpression of the cel.bind() macro. This allows
// for consistent behavior for CEL compiler optimized expressions that extract
// subexpressions to cel.bind calls.
// This is now always enabled. Setting this option has no effect. It will be
// removed in a later update.
bool enable_lazy_bind_initialization = true;
};
// LINT.ThenChange(//depot/google3/eval/public/cel_options.h)
Expand Down

0 comments on commit 4393131

Please # to comment.