diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 9f85f9da2..9fa4b7b6a 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -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_; @@ -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; @@ -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())); @@ -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}; @@ -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; @@ -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( this, options_.short_circuiting, is_bind, iter_slot, accu_slot)}); comprehension_stack_.back().visitor->PreVisit(expr); @@ -1003,7 +997,6 @@ 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 visitor; }; @@ -1011,23 +1004,9 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { 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, diff --git a/eval/public/cel_options.h b/eval/public/cel_options.h index 152c2a77a..e45f10eee 100644 --- a/eval/public/cel_options.h +++ b/eval/public/cel_options.h @@ -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) diff --git a/extensions/bindings_ext_benchmark_test.cc b/extensions/bindings_ext_benchmark_test.cc index 199c015ac..8c4ccc603 100644 --- a/extensions/bindings_ext_benchmark_test.cc +++ b/extensions/bindings_ext_benchmark_test.cc @@ -159,7 +159,6 @@ TEST_P(BindingsBenchmarkTest, CheckBenchmarkCaseWorks) { auto expr, ParseWithMacros(benchmark.expression, all_macros, "")); InterpreterOptions options; - options.enable_lazy_bind_initialization = true; auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); @@ -185,7 +184,6 @@ void RunBenchmark(const BenchmarkCase& benchmark, benchmark::State& state) { auto expr, ParseWithMacros(benchmark.expression, all_macros, "")); InterpreterOptions options; - options.enable_lazy_bind_initialization = true; auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); diff --git a/extensions/bindings_ext_test.cc b/extensions/bindings_ext_test.cc index 0e6c970ee..7aa7ad0ab 100644 --- a/extensions/bindings_ext_test.cc +++ b/extensions/bindings_ext_test.cc @@ -90,11 +90,10 @@ std::unique_ptr CreateBindFunction() { } class BindingsExtTest - : public testing::TestWithParam> { + : public testing::TestWithParam> { 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) { @@ -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 builder = CreateCelExpressionBuilder(options); ASSERT_OK(builder->GetRegistry()->Register(CreateBindFunction())); @@ -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. // @@ -445,11 +442,9 @@ constexpr absl::string_view kFieldSelectTestExpr = R"pb( } })pb"; -class BindingsExtInteractionsTest - : public testing::TestWithParam> { +class BindingsExtInteractionsTest : public testing::TestWithParam { protected: - bool GetEnableLazyBinding() { return std::get<0>(GetParam()); } - bool GetEnableSelectOptimization() { return std::get<1>(GetParam()); } + bool GetEnableSelectOptimization() { return GetParam(); } }; TEST_P(BindingsExtInteractionsTest, SelectOptimization) { @@ -457,7 +452,6 @@ TEST_P(BindingsExtInteractionsTest, SelectOptimization) { 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 builder = CreateCelExpressionBuilder(options); @@ -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 builder = CreateCelExpressionBuilder(options); @@ -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 builder = CreateCelExpressionBuilder(options); @@ -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 builder = CreateCelExpressionBuilder(options); @@ -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 builder = CreateCelExpressionBuilder(options); @@ -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 builder = CreateCelExpressionBuilder(options); @@ -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 builder = CreateCelExpressionBuilder(options); @@ -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 diff --git a/runtime/runtime_options.h b/runtime/runtime_options.h index 57bbdf50b..351213b7e 100644 --- a/runtime/runtime_options.h +++ b/runtime/runtime_options.h @@ -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)