diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index b1f62f419..4ff877852 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -68,7 +68,6 @@ cc_library( "//base:memory", "//base/ast_internal:ast_impl", "//base/ast_internal:expr", - "//eval/eval:comprehension_slots", "//eval/eval:comprehension_step", "//eval/eval:const_value_step", "//eval/eval:container_access_step", @@ -78,6 +77,7 @@ cc_library( "//eval/eval:function_step", "//eval/eval:ident_step", "//eval/eval:jump_step", + "//eval/eval:lazy_init_step", "//eval/eval:logic_step", "//eval/eval:select_step", "//eval/eval:shadowable_value_step", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index c45d1fb92..01a5d4990 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -26,6 +27,7 @@ #include #include +#include "absl/algorithm/container.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_map.h" @@ -56,6 +58,7 @@ #include "eval/eval/function_step.h" #include "eval/eval/ident_step.h" #include "eval/eval/jump_step.h" +#include "eval/eval/lazy_init_step.h" #include "eval/eval/logic_step.h" #include "eval/eval/select_step.h" #include "eval/eval/shadowable_value_step.h" @@ -86,6 +89,9 @@ using ::cel::runtime_internal::IssueCollector; // Forward declare to resolve circular dependency for short_circuiting visitors. class FlatExprVisitor; +// Representation of extracted subexpressions. +using ExpressionTable = std::vector; + // Helper for bookkeeping variables mapped to indexes. class IndexManager { public: @@ -199,6 +205,58 @@ class ExhaustiveTernaryCondVisitor : public CondVisitor { FlatExprVisitor* visitor_; }; +// Returns whether this comprehension appears to be a standard map/filter +// macro implementation. It is not exhaustive, so it is unsafe to use with +// custom comprehensions outside of the standard macros or hand crafted ASTs. +bool IsOptimizableListAppend( + const cel::ast_internal::Comprehension* comprehension, + bool enable_comprehension_list_append) { + if (!enable_comprehension_list_append) { + return false; + } + absl::string_view accu_var = comprehension->accu_var(); + if (accu_var.empty() || + comprehension->result().ident_expr().name() != accu_var) { + return false; + } + if (!comprehension->accu_init().has_list_expr()) { + return false; + } + + if (!comprehension->loop_step().has_call_expr()) { + return false; + } + + // Macro loop_step for a filter() will contain a ternary: + // filter ? accu_var + [elem] : accu_var + // Macro loop_step for a map() will contain a list concat operation: + // accu_var + [elem] + const auto* call_expr = &comprehension->loop_step().call_expr(); + + if (call_expr->function() == cel::builtin::kTernary && + call_expr->args().size() == 3) { + if (!call_expr->args()[1].has_call_expr()) { + return false; + } + call_expr = &(call_expr->args()[1].call_expr()); + } + + return call_expr->function() == cel::builtin::kAdd && + call_expr->args().size() == 2 && + call_expr->args()[0].has_ident_expr() && + call_expr->args()[0].ident_expr().name() == accu_var; +} + +bool IsBind(const cel::ast_internal::Comprehension* comprehension) { + static constexpr absl::string_view kUnusedIterVar = "#unused"; + + return comprehension->loop_condition().const_expr().has_bool_value() && + comprehension->loop_condition().const_expr().bool_value() == false && + comprehension->iter_var() == kUnusedIterVar && + comprehension->iter_range().has_list_expr() && + comprehension->iter_range().list_expr().elements().empty(); +} + // Visitor for Comprehension expressions. class ComprehensionVisitor { public: @@ -210,6 +268,7 @@ class ComprehensionVisitor { cond_step_(nullptr), short_circuiting_(short_circuiting), is_trivial_(is_trivial), + accu_init_extracted_(false), iter_slot_(iter_slot), accu_slot_(accu_slot) {} @@ -224,6 +283,8 @@ class ComprehensionVisitor { } void PostVisit(const cel::ast_internal::Expr* expr); + void MarkAccuInitExtracted() { accu_init_extracted_ = true; } + private: void PostVisitArgTrivial(cel::ast_internal::ComprehensionArg arg_num, const cel::ast_internal::Expr* comprehension_expr); @@ -238,6 +299,7 @@ class ComprehensionVisitor { int cond_step_pos_; bool short_circuiting_; bool is_trivial_; + bool accu_init_extracted_; size_t iter_slot_; size_t accu_slot_; }; @@ -246,21 +308,22 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { public: FlatExprVisitor( const Resolver& resolver, const cel::RuntimeOptions& options, - absl::Span> program_optimizers, + std::vector> program_optimizers, const absl::flat_hash_map& reference_map, - ExecutionPath& path, ValueFactory& value_factory, - IssueCollector& issue_collector, + ExpressionTable& expression_table, ExecutionPath& path, + ValueFactory& value_factory, IssueCollector& issue_collector, PlannerContext::ProgramTree& program_tree, PlannerContext& extension_context) : resolver_(resolver), + expression_table_(expression_table), execution_path_(path), value_factory_(value_factory), progress_status_(absl::OkStatus()), resolved_select_expr_(nullptr), parent_expr_(nullptr), options_(options), - program_optimizers_(program_optimizers), + program_optimizers_(std::move(program_optimizers)), issue_collector_(issue_collector), program_tree_(program_tree), extension_context_(extension_context) {} @@ -277,11 +340,11 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { suppressed_branches_.find(expr) != suppressed_branches_.end()) { resume_from_suppressed_branch_ = expr; } - if (program_optimizers_.empty()) { + if (!ProgramStructureTrackingEnabled()) { return; } PlannerContext::ProgramInfo& info = program_tree_[expr]; - info.range_start = execution_path_.size(); + info.range_start = GetCurrentIndex(); info.parent = parent_expr_; if (parent_expr_ != nullptr) { program_tree_[parent_expr_].children.push_back(expr); @@ -305,12 +368,11 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { if (expr == resume_from_suppressed_branch_) { resume_from_suppressed_branch_ = nullptr; } - // TODO(uncreated-issue/27): this will be generalized later. - if (program_optimizers_.empty()) { + if (!ProgramStructureTrackingEnabled()) { return; } PlannerContext::ProgramInfo& info = program_tree_[expr]; - info.range_len = execution_path_.size() - info.range_start; + info.range_len = GetCurrentIndex() - info.range_start; parent_expr_ = info.parent; for (const std::unique_ptr& optimizer : @@ -318,8 +380,15 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { absl::Status status = optimizer->OnPostVisit(extension_context_, *expr); if (!status.ok()) { SetProgressStatusError(status); + return; } } + if (options_.enable_lazy_bind_initialization && + !comprehension_stack_.empty() && + (&comprehension_stack_.back().comprehension->accu_init() == expr)) { + SetProgressStatusError( + MaybeExtractSubexpression(expr, comprehension_stack_.back())); + } } void PostVisitConst(const cel::ast_internal::Constant* const_expr, @@ -332,6 +401,43 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { AddStep(CreateConstValueStep(*const_expr, expr->id(), value_factory_)); } + struct SlotLookupResult { + int slot; + int subexpression; + }; + + // Helper to lookup a variable mapped to a slot. + // + // If lazy evaluation enabled and ided as a lazy expression, + // subexpression and slot will be set. + SlotLookupResult LookupSlot(absl::string_view path) { + if (!comprehension_stack_.empty()) { + for (int i = comprehension_stack_.size() - 1; i >= 0; i--) { + const ComprehensionStackRecord& record = comprehension_stack_[i]; + if (record.iter_var_in_scope && + record.comprehension->iter_var() == path) { + if (record.is_optimizable_bind) { + SetProgressStatusError(issue_collector_.AddIssue( + RuntimeIssue::CreateWarning(absl::InvalidArgumentError( + "Unexpected iter_var access in trivial comprehension")))); + return {-1, -1}; + } + return {static_cast(record.iter_slot), -1}; + } + if (record.accu_var_in_scope && + record.comprehension->accu_var() == path) { + int slot = record.accu_slot; + int subexpression = -1; + if (record.should_lazy_eval) { + subexpression = record.subexpression; + } + return {slot, subexpression}; + } + } + } + return {-1, -1}; + } + // Ident node handler. // Invoked after child nodes are processed. void PostVisitIdent(const cel::ast_internal::Ident* ident_expr, @@ -381,31 +487,15 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { } // If this is a comprehension variable, check for the assigned slot. - int slot = -1; - if (!comprehension_stack_.empty()) { - for (int i = comprehension_stack_.size() - 1; i >= 0; i--) { - const ComprehensionStackRecord& record = comprehension_stack_[i]; - if (record.iter_var_in_scope && - record.comprehension->iter_var() == path) { - if (record.is_optimizable_bind) { - SetProgressStatusError(issue_collector_.AddIssue( - RuntimeIssue::CreateWarning(absl::InvalidArgumentError( - "Unexpected iter_var access in trivial comprehension")))); - break; - } - slot = record.iter_slot; - break; - } - if (record.accu_var_in_scope && - record.comprehension->accu_var() == path) { - slot = record.accu_slot; - break; - } - } - } + SlotLookupResult slot = LookupSlot(path); - if (slot >= 0) { - AddStep(CreateIdentStepForSlot(*ident_expr, slot, expr->id())); + if (slot.subexpression >= 0) { + AddStep( + CreateCheckLazyInitStep(slot.slot, slot.subexpression, expr->id())); + AddStep(CreateAssignSlotStep(slot.slot)); + return; + } else if (slot.slot >= 0) { + AddStep(CreateIdentStepForSlot(*ident_expr, slot.slot, expr->id())); return; } @@ -598,58 +688,6 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { AddStep(CreateFunctionStep(*call_expr, expr->id(), std::move(overloads))); } - // Returns whether this comprehension appears to be a standard map/filter - // macro implementation. It is not exhaustive, so it is unsafe to use with - // custom comprehensions outside of the standard macros or hand crafted ASTs. - bool IsOptimizableListAppend( - const cel::ast_internal::Comprehension* comprehension, - bool enable_comprehension_list_append) { - if (!enable_comprehension_list_append) { - return false; - } - absl::string_view accu_var = comprehension->accu_var(); - if (accu_var.empty() || - comprehension->result().ident_expr().name() != accu_var) { - return false; - } - if (!comprehension->accu_init().has_list_expr()) { - return false; - } - - if (!comprehension->loop_step().has_call_expr()) { - return false; - } - - // Macro loop_step for a filter() will contain a ternary: - // filter ? accu_var + [elem] : accu_var - // Macro loop_step for a map() will contain a list concat operation: - // accu_var + [elem] - const auto* call_expr = &comprehension->loop_step().call_expr(); - - if (call_expr->function() == cel::builtin::kTernary && - call_expr->args().size() == 3) { - if (!call_expr->args()[1].has_call_expr()) { - return false; - } - call_expr = &(call_expr->args()[1].call_expr()); - } - - return call_expr->function() == cel::builtin::kAdd && - call_expr->args().size() == 2 && - call_expr->args()[0].has_ident_expr() && - call_expr->args()[0].ident_expr().name() == accu_var; - } - - bool IsBind(const cel::ast_internal::Comprehension* comprehension) { - static constexpr absl::string_view kUnusedIterVar = "#unused"; - - return comprehension->loop_condition().const_expr().has_bool_value() && - comprehension->loop_condition().const_expr().bool_value() == false && - comprehension->iter_var() == kUnusedIterVar && - comprehension->iter_range().has_list_expr() && - comprehension->iter_range().list_expr().elements().empty(); - } - void PreVisitComprehension( const cel::ast_internal::Comprehension* comprehension, const cel::ast_internal::Expr* expr, @@ -689,11 +727,15 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { } comprehension_stack_.push_back( {expr, comprehension, iter_slot, accu_slot, + /*subexpression=*/-1, IsOptimizableListAppend(comprehension, options_.enable_comprehension_list_append), is_bind, /*.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); @@ -738,26 +780,31 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { switch (comprehension_arg) { case cel::ast_internal::ITER_RANGE: { + record.in_accu_init = false; record.iter_var_in_scope = false; record.accu_var_in_scope = false; break; } case cel::ast_internal::ACCU_INIT: { + record.in_accu_init = true; record.iter_var_in_scope = false; record.accu_var_in_scope = false; break; } case cel::ast_internal::LOOP_CONDITION: { + record.in_accu_init = false; record.iter_var_in_scope = true; record.accu_var_in_scope = true; break; } case cel::ast_internal::LOOP_STEP: { + record.in_accu_init = false; record.iter_var_in_scope = true; record.accu_var_in_scope = true; break; } case cel::ast_internal::RESULT: { + record.in_accu_init = false; record.iter_var_in_scope = false; record.accu_var_in_scope = true; break; @@ -907,6 +954,10 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { size_t slot_count() const { return index_manager_.max_slot_count(); } + void AddOptimizer(std::unique_ptr optimizer) { + program_optimizers_.push_back(std::move(optimizer)); + } + // Tests the boolean predicate, and if false produces an InvalidArgumentError // which concatenates the error_message and any optional message_parts as the // error status message. @@ -927,10 +978,14 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { const cel::ast_internal::Comprehension* comprehension; size_t iter_slot; size_t accu_slot; + // -1 indicates this shouldn't be used. + int subexpression; bool is_optimizable_list_append; bool is_optimizable_bind; bool iter_var_in_scope; bool accu_var_in_scope; + bool in_accu_init; + bool should_lazy_eval; std::unique_ptr visitor; }; @@ -938,7 +993,33 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { return resume_from_suppressed_branch_ != nullptr; } + bool ProgramStructureTrackingEnabled() { + return options_.enable_lazy_bind_initialization || + !program_optimizers_.empty(); + } + + absl::Status MaybeExtractSubexpression(const cel::ast_internal::Expr* expr, + ComprehensionStackRecord& record) { + if (!record.should_lazy_eval) { + return absl::OkStatus(); + } + CEL_ASSIGN_OR_RETURN(auto subexpr, + extension_context_.ExtractSubplan(*expr)); + + CEL_RETURN_IF_ERROR(extension_context_.ReplaceSubplan(*expr, {})); + + expression_table_.push_back(std::move(subexpr)); + // off by one since mainline expression is handled separately. + size_t id = expression_table_.size(); + record.subexpression = id; + + record.visitor->MarkAccuInitExtracted(); + + return absl::OkStatus(); + } + const Resolver& resolver_; + ExpressionTable& expression_table_; ExecutionPath& execution_path_; ValueFactory& value_factory_; absl::Status progress_status_; @@ -968,7 +1049,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { std::vector comprehension_stack_; absl::flat_hash_set suppressed_branches_; const cel::ast_internal::Expr* resume_from_suppressed_branch_ = nullptr; - absl::Span> program_optimizers_; + std::vector> program_optimizers_; IssueCollector& issue_collector_; PlannerContext::ProgramTree& program_tree_; @@ -1155,7 +1236,9 @@ void ComprehensionVisitor::PostVisitArgTrivial( break; } case cel::ast_internal::ACCU_INIT: { - visitor_->AddStep(CreateSetSlotVarStep(accu_slot_, expr->id())); + if (!accu_init_extracted_) { + visitor_->AddStep(CreateAssignSlotAndPopStep(accu_slot_)); + } break; } case cel::ast_internal::LOOP_CONDITION: { @@ -1165,7 +1248,7 @@ void ComprehensionVisitor::PostVisitArgTrivial( break; } case cel::ast_internal::RESULT: { - visitor_->AddStep(CreateClearSlotVarStep(accu_slot_, expr->id())); + visitor_->AddStep(CreateClearSlotStep(accu_slot_, expr->id())); break; } } @@ -1173,11 +1256,35 @@ void ComprehensionVisitor::PostVisitArgTrivial( void ComprehensionVisitor::PostVisit(const cel::ast_internal::Expr* expr) {} +// Flattens the expression table into the end of the mainline expression vector +// and returns an index to the individual sub expressions. +std::vector FlattenExpressionTable( + ExpressionTable& expression_table, ExecutionPath& main) { + std::vector> ranges; + ranges.push_back(std::make_pair(0, main.size())); + + for (int i = 0; i < expression_table.size(); ++i) { + ExecutionPath& subexpression = expression_table[i]; + ranges.push_back(std::make_pair(main.size(), subexpression.size())); + absl::c_move(subexpression, std::back_inserter(main)); + } + expression_table.clear(); + // the main program is now stable, can make the indexes. + std::vector subexpressions; + subexpressions.reserve(ranges.size()); + for (const auto& range : ranges) { + subexpressions.push_back( + absl::MakeSpan(main).subspan(range.first, range.second)); + } + return subexpressions; +} + } // namespace absl::StatusOr FlatExprBuilder::CreateExpressionImpl( std::unique_ptr ast, std::vector* issues) const { ExecutionPath execution_path; + ExpressionTable expression_table; // These objects are expected to remain scoped to one build call -- references // to them shouldn't be persisted in any part of the result expression. @@ -1215,9 +1322,11 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( CEL_ASSIGN_OR_RETURN(optimizers.emplace_back(), optimizer_factory(extension_context, ast_impl)); } - FlatExprVisitor visitor( - resolver, options_, optimizers, ast_impl.reference_map(), execution_path, - value_factory, issue_collector, program_tree, extension_context); + + FlatExprVisitor visitor(resolver, options_, std::move(optimizers), + ast_impl.reference_map(), expression_table, + execution_path, value_factory, issue_collector, + program_tree, extension_context); cel::ast_internal::TraversalOptions opts; opts.use_comprehension_callbacks = true; @@ -1231,7 +1340,11 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( (*issues) = issue_collector.ExtractIssues(); } - return FlatExpression(std::move(execution_path), visitor.slot_count(), + std::vector subexpressions = + FlattenExpressionTable(expression_table, execution_path); + + return FlatExpression(std::move(execution_path), std::move(subexpressions), + visitor.slot_count(), type_registry_.GetComposedTypeProvider(), options_); } diff --git a/eval/eval/BUILD b/eval/eval/BUILD index bcc9d1dd6..3712424b3 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -41,6 +41,7 @@ cc_library( "//runtime:activation_interface", "//runtime:managed_value_factory", "//runtime:runtime_options", + "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", @@ -855,3 +856,32 @@ cc_test( "//runtime:runtime_options", ], ) + +cc_library( + name = "lazy_init_step", + srcs = ["lazy_init_step.cc"], + hdrs = ["lazy_init_step.h"], + deps = [ + ":evaluator_core", + ":expression_step_base", + "@com_google_absl//absl/status", + ], +) + +cc_test( + name = "lazy_init_step_test", + srcs = ["lazy_init_step_test.cc"], + deps = [ + ":const_value_step", + ":evaluator_core", + ":lazy_init_step", + "//base:data", + "//extensions/protobuf:memory_manager", + "//internal:testing", + "//runtime:activation", + "//runtime:managed_value_factory", + "//runtime:runtime_options", + "@com_google_absl//absl/status", + "@com_google_protobuf//:protobuf", + ], +) diff --git a/eval/eval/comprehension_step.cc b/eval/eval/comprehension_step.cc index 7df179a23..79d448484 100644 --- a/eval/eval/comprehension_step.cc +++ b/eval/eval/comprehension_step.cc @@ -127,42 +127,6 @@ absl::Status ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { return absl::OkStatus(); } -class SetSlotVarStep : public ExpressionStepBase { - public: - SetSlotVarStep(size_t slot_index, int64_t expr_id) - : ExpressionStepBase(expr_id, false), slot_index_(slot_index) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, - "Value stack underflow in accu_init"); - } - frame->comprehension_slots().Set(slot_index_, frame->value_stack().Peek(), - frame->value_stack().PeekAttribute()); - - frame->value_stack().Pop(1); - - return absl::OkStatus(); - } - - private: - size_t slot_index_; -}; - -class ClearSlotVarStep : public ExpressionStepBase { - public: - ClearSlotVarStep(size_t slot_index, int64_t expr_id) - : ExpressionStepBase(expr_id), slot_index_(slot_index) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - frame->comprehension_slots().ClearSlot(slot_index_); - return absl::OkStatus(); - } - - private: - size_t slot_index_; -}; - } // namespace // Stack variables during comprehension evaluation: @@ -349,14 +313,4 @@ std::unique_ptr CreateComprehensionInitStep(int64_t expr_id) { return std::make_unique(expr_id); } -std::unique_ptr CreateSetSlotVarStep(size_t slot_index, - int64_t expr_id) { - return std::make_unique(slot_index, expr_id); -} - -std::unique_ptr CreateClearSlotVarStep(size_t slot_index, - int64_t expr_id) { - return std::make_unique(slot_index, expr_id); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step.h b/eval/eval/comprehension_step.h index 3715604e1..a95e271b3 100644 --- a/eval/eval/comprehension_step.h +++ b/eval/eval/comprehension_step.h @@ -55,15 +55,6 @@ std::unique_ptr CreateComprehensionFinishStep(size_t accu_slot, // context for the comprehension. std::unique_ptr CreateComprehensionInitStep(int64_t expr_id); -// Creates a step that pops the given variable from the stack and assigns it to -// a slot. -std::unique_ptr CreateSetSlotVarStep(size_t slot_index, - int64_t expr_id); - -// Creates a step that clears a slot variable. -std::unique_ptr CreateClearSlotVarStep(size_t slot_index, - int64_t expr_id); - } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_COMPREHENSION_STEP_H_ diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 0b30985d8..af988ea35 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -18,9 +18,11 @@ #include #include +#include "absl/log/absl_check.h" #include "absl/log/absl_log.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "absl/utility/utility.h" @@ -60,6 +62,14 @@ const ExpressionStep* ExecutionFrame::Next() { size_t end_pos = execution_path_.size(); if (pc_ < end_pos) return execution_path_[pc_++].get(); + if (pc_ == end_pos && !call_stack_.empty()) { + pc_ = call_stack_.back().return_pc; + execution_path_ = call_stack_.back().return_expression; + ABSL_DCHECK_EQ(value_stack().size(), + call_stack_.back().expected_stack_size); + call_stack_.pop_back(); + return Next(); + } if (pc_ > end_pos) { ABSL_LOG(ERROR) << "Attempting to step beyond the end of execution path."; } @@ -91,8 +101,9 @@ absl::StatusOr> ExecutionFrame::Evaluate( size_t final_stack_size = value_stack().size(); if (final_stack_size != initial_stack_size + 1 || final_stack_size == 0) { - return absl::Status(absl::StatusCode::kInternal, - "Stack error during evaluation"); + return absl::InternalError(absl::StrCat( + "Stack error during evaluation: expected=", initial_stack_size + 1, + ", actual=", final_stack_size)); } cel::Handle value = value_stack().Peek(); value_stack().Pop(1); @@ -116,7 +127,7 @@ absl::StatusOr> FlatExpression::EvaluateWithCallback( FlatExpressionEvaluatorState& state) const { state.Reset(); - ExecutionFrame frame(path_, activation, options_, state); + ExecutionFrame frame(subexpressions_, activation, options_, state); return frame.Evaluate(std::move(listener)); } diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 892c58a0d..1defa9a10 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -21,6 +21,7 @@ #include #include +#include "absl/log/absl_check.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" @@ -143,7 +144,26 @@ class ExecutionFrame { activation_.GetMissingAttributes(), state_.value_factory()), max_iterations_(options_.comprehension_max_iterations), - iterations_(0) {} + iterations_(0), + subexpressions_() {} + + ExecutionFrame(absl::Span subexpressions, + const cel::ActivationInterface& activation, + const cel::RuntimeOptions& options, + FlatExpressionEvaluatorState& state) + : pc_(0UL), + execution_path_(subexpressions[0]), + activation_(activation), + options_(options), + state_(state), + attribute_utility_(activation_.GetUnknownAttributes(), + activation_.GetMissingAttributes(), + state_.value_factory()), + max_iterations_(options_.comprehension_max_iterations), + iterations_(0), + subexpressions_(subexpressions) { + ABSL_DCHECK(!subexpressions.empty()); + } // Returns next expression to evaluate. const ExpressionStep* Next(); @@ -151,7 +171,10 @@ class ExecutionFrame { // Evaluate the execution frame to completion. absl::StatusOr> Evaluate(EvaluationListener listener); - // Intended for use only in conditionals. + // Intended for use in builtin shortcutting operations. + // + // Offset applies after normal pc increment. For example, JumpTo(0) is a + // no-op, JumpTo(1) skips the expected next step. absl::Status JumpTo(int offset) { int new_pc = static_cast(pc_) + offset; if (new_pc < 0 || new_pc > static_cast(execution_path_.size())) { @@ -164,6 +187,27 @@ class ExecutionFrame { return absl::OkStatus(); } + // Move pc to a subexpression. + // + // Unlike a `Call` in a programming language, the subexpression is evaluated + // in the same context as the caller (e.g. no stack isolation or scope change) + // + // Only intended for use in built-in notion of lazily evaluated + // subexpressions. + void Call(int return_pc_offset, size_t subexpression_index) { + ABSL_DCHECK(subexpression_index < subexpressions_.size()); + ExecutionPathView subexpression = subexpressions_[subexpression_index]; + ABSL_DCHECK(subexpression != execution_path_); + int return_pc = static_cast(pc_) + return_pc_offset; + // return pc == size() is supported (a tail call). + ABSL_DCHECK(return_pc >= 0 && + return_pc <= static_cast(execution_path_.size())); + call_stack_.push_back(SubFrame{static_cast(return_pc), + value_stack().size() + 1, execution_path_}); + pc_ = 0UL; + execution_path_ = subexpression; + } + EvaluatorStack& value_stack() { return state_.value_stack(); } ComprehensionSlots& comprehension_slots() { return state_.comprehension_slots(); @@ -229,6 +273,12 @@ class ExecutionFrame { } private: + struct SubFrame { + size_t return_pc; + size_t expected_stack_size; + ExecutionPathView return_expression; + }; + size_t pc_; // pc_ - Program Counter. Current position on execution path. ExecutionPathView execution_path_; const cel::ActivationInterface& activation_; @@ -237,6 +287,8 @@ class ExecutionFrame { AttributeUtility attribute_utility_; const int max_iterations_; int iterations_; + absl::Span subexpressions_; + std::vector call_stack_; }; // A flattened representation of the input CEL AST. @@ -249,6 +301,18 @@ class FlatExpression { const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options) : path_(std::move(path)), + subexpressions_({path_}), + comprehension_slots_size_(comprehension_slots_size), + type_provider_(type_provider), + options_(options) {} + + FlatExpression(ExecutionPath path, + std::vector subexpressions, + size_t comprehension_slots_size, + const cel::TypeProvider& type_provider, + const cel::RuntimeOptions& options) + : path_(std::move(path)), + subexpressions_(std::move(subexpressions)), comprehension_slots_size_(comprehension_slots_size), type_provider_(type_provider), options_(options) {} @@ -280,6 +344,7 @@ class FlatExpression { private: ExecutionPath path_; + std::vector subexpressions_; size_t comprehension_slots_size_; const cel::TypeProvider& type_provider_; cel::RuntimeOptions options_; diff --git a/eval/eval/lazy_init_step.cc b/eval/eval/lazy_init_step.cc new file mode 100644 index 000000000..da06c7bb7 --- /dev/null +++ b/eval/eval/lazy_init_step.cc @@ -0,0 +1,117 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "eval/eval/lazy_init_step.h" + +#include +#include +#include + +#include "absl/status/status.h" +#include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_base.h" + +namespace google::api::expr::runtime { + +namespace { + +class CheckLazyInitStep : public ExpressionStepBase { + public: + CheckLazyInitStep(size_t slot_index, size_t subexpression_index, + int64_t expr_id) + : ExpressionStepBase(expr_id), + slot_index_(slot_index), + subexpression_index_(subexpression_index) {} + + absl::Status Evaluate(ExecutionFrame* frame) const override { + auto* slot = frame->comprehension_slots().Get(slot_index_); + if (slot != nullptr) { + frame->value_stack().Push(slot->value, slot->attribute); + // skip next step (assign to slot) + return frame->JumpTo(1); + } + + // return to next step (assign to slot) + frame->Call(0, subexpression_index_); + return absl::OkStatus(); + } + + private: + size_t slot_index_; + size_t subexpression_index_; +}; + +class AssignSlotStep : public ExpressionStepBase { + public: + explicit AssignSlotStep(size_t slot_index, bool should_pop) + : ExpressionStepBase(/*expr_id=*/-1, /*comes_from_ast=*/false), + slot_index_(slot_index), + should_pop_(should_pop) {} + + absl::Status Evaluate(ExecutionFrame* frame) const override { + if (!frame->value_stack().HasEnough(1)) { + return absl::InternalError("Stack underflow assigning lazy value"); + } + + frame->comprehension_slots().Set(slot_index_, frame->value_stack().Peek(), + frame->value_stack().PeekAttribute()); + + if (should_pop_) { + frame->value_stack().Pop(1); + } + + return absl::OkStatus(); + } + + private: + size_t slot_index_; + bool should_pop_; +}; + +class ClearSlotStep : public ExpressionStepBase { + public: + explicit ClearSlotStep(size_t slot_index, int64_t expr_id) + : ExpressionStepBase(expr_id), slot_index_(slot_index) {} + + absl::Status Evaluate(ExecutionFrame* frame) const override { + frame->comprehension_slots().ClearSlot(slot_index_); + return absl::OkStatus(); + } + + private: + size_t slot_index_; +}; + +} // namespace + +std::unique_ptr CreateCheckLazyInitStep( + size_t slot_index, size_t subexpression_index, int64_t expr_id) { + return std::make_unique(slot_index, subexpression_index, + expr_id); +} + +std::unique_ptr CreateAssignSlotStep(size_t slot_index) { + return std::make_unique(slot_index, /*should_pop=*/false); +} + +std::unique_ptr CreateAssignSlotAndPopStep(size_t slot_index) { + return std::make_unique(slot_index, /*should_pop=*/true); +} + +std::unique_ptr CreateClearSlotStep(size_t slot_index, + int64_t expr_id) { + return std::make_unique(slot_index, expr_id); +} + +} // namespace google::api::expr::runtime diff --git a/eval/eval/lazy_init_step.h b/eval/eval/lazy_init_step.h new file mode 100644 index 000000000..5733afa7f --- /dev/null +++ b/eval/eval/lazy_init_step.h @@ -0,0 +1,67 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Program steps for lazily initialized aliases (e.g. cel.bind). +// +// When used, any reference to variable should be replaced with a conditional +// step that either runs the initialization routine or pushes the already +// initialized variable to the stack. +// +// All references to the variable should be replaced with: +// +// +-----------------+-------------------+--------------------+ +// | stack | pc | step | +// +-----------------+-------------------+--------------------+ +// | {} | 0 | check init slot(i) | +// +-----------------+-------------------+--------------------+ +// | {value} | 1 | assign slot(i) | +// +-----------------+-------------------+--------------------+ +// | {value} | 2 | | +// +-----------------+-------------------+--------------------+ +// | .... | +// +-----------------+-------------------+--------------------+ +// | {...} | n (end of scope) | clear slot(i) | +// +-----------------+-------------------+--------------------+ + +#ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_LAZY_INIT_STEP_H_ +#define THIRD_PARTY_CEL_CPP_EVAL_EVAL_LAZY_INIT_STEP_H_ + +#include +#include +#include + +#include "eval/eval/evaluator_core.h" + +namespace google::api::expr::runtime { + +// Creates a guard step that checks that an alias is initialized. +// If it is, push to stack and jump to the step that depends on the value. +// Otherwise, run the initialization routine (which pushes the value to top of +// stack) and set the corresponding slot. +std::unique_ptr CreateCheckLazyInitStep( + size_t slot_index, size_t subexpression_index, int64_t expr_id); + +// Helper step to assign a slot value from the top of stack on initialization. +std::unique_ptr CreateAssignSlotStep(size_t slot_index); +std::unique_ptr CreateAssignSlotAndPopStep(size_t slot_index); + +// Helper step to clear a slot. +// Slots may be reused in different contexts so need to be cleared after a +// context is done. +std::unique_ptr CreateClearSlotStep(size_t slot_index, + int64_t expr_id); + +} // namespace google::api::expr::runtime + +#endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_LAZY_INIT_STEP_H_ diff --git a/eval/eval/lazy_init_step_test.cc b/eval/eval/lazy_init_step_test.cc new file mode 100644 index 000000000..f0cb2d487 --- /dev/null +++ b/eval/eval/lazy_init_step_test.cc @@ -0,0 +1,185 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "eval/eval/lazy_init_step.h" + +#include +#include + +#include "absl/status/status.h" +#include "base/type_provider.h" +#include "base/value_factory.h" +#include "base/values/int_value.h" +#include "eval/eval/const_value_step.h" +#include "eval/eval/evaluator_core.h" +#include "extensions/protobuf/memory_manager.h" +#include "internal/testing.h" +#include "runtime/activation.h" +#include "runtime/managed_value_factory.h" +#include "runtime/runtime_options.h" +#include "google/protobuf/arena.h" + +namespace google::api::expr::runtime { +namespace { + +using ::cel::Activation; +using ::cel::IntValue; +using ::cel::ManagedValueFactory; +using ::cel::RuntimeOptions; +using ::cel::TypeProvider; +using ::cel::ValueFactory; +using ::cel::extensions::ProtoMemoryManagerRef; +using testing::HasSubstr; +using cel::internal::StatusIs; + +class LazyInitStepTest : public testing::Test { + private: + // arbitrary numbers enough for basic tests. + static constexpr size_t kValueStack = 5; + static constexpr size_t kComprehensionSlotCount = 3; + + public: + LazyInitStepTest() + : value_factory_(TypeProvider::Builtin(), ProtoMemoryManagerRef(&arena_)), + evaluator_state_(kValueStack, kComprehensionSlotCount, + value_factory_.get()) {} + + protected: + ValueFactory& value_factory() { return value_factory_.get(); }; + + google::protobuf::Arena arena_; + ManagedValueFactory value_factory_; + FlatExpressionEvaluatorState evaluator_state_; + RuntimeOptions runtime_options_; + Activation activation_; +}; + +TEST_F(LazyInitStepTest, CreateCheckInitStepDoesInit) { + ExecutionPath path; + ExecutionPath subpath; + + path.push_back(CreateCheckLazyInitStep(/*slot_index=*/0, + /*subexpression_index=*/1, -1)); + + ASSERT_OK_AND_ASSIGN( + subpath.emplace_back(), + CreateConstValueStep(value_factory().CreateIntValue(42), -1, false)); + + std::vector expression_table{path, subpath}; + + ExecutionFrame frame(expression_table, activation_, runtime_options_, + evaluator_state_); + ASSERT_OK_AND_ASSIGN(auto value, frame.Evaluate(EvaluationListener())); + + EXPECT_TRUE(value->Is() && + value->As().NativeValue() == 42); +} + +TEST_F(LazyInitStepTest, CreateCheckInitStepSkipInit) { + ExecutionPath path; + ExecutionPath subpath; + + path.push_back(CreateCheckLazyInitStep(/*slot_index=*/0, + /*subexpression_index=*/1, -1)); + // This is the expected usage, but in this test we are just depending on the + // fact that these don't change the stack and fit the program layout + // requirements. + path.push_back(CreateAssignSlotStep(/*slot_index=*/0)); + path.push_back(CreateClearSlotStep(/*slot_index=*/0, -1)); + + ASSERT_OK_AND_ASSIGN( + subpath.emplace_back(), + CreateConstValueStep(value_factory().CreateIntValue(42), -1, false)); + + std::vector expression_table{path, subpath}; + + ExecutionFrame frame(expression_table, activation_, runtime_options_, + evaluator_state_); + frame.comprehension_slots().Set(0, value_factory().CreateIntValue(42)); + ASSERT_OK_AND_ASSIGN(auto value, frame.Evaluate(EvaluationListener())); + + EXPECT_TRUE(value->Is() && + value->As().NativeValue() == 42); +} + +TEST_F(LazyInitStepTest, CreateAssignSlotStepBasic) { + ExecutionPath path; + + path.push_back(CreateAssignSlotStep(0)); + + ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); + frame.comprehension_slots().ClearSlot(0); + + frame.value_stack().Push(value_factory().CreateIntValue(42)); + + // This will error because no return value, step will still evaluate. + frame.Evaluate(EvaluationListener()).IgnoreError(); + + auto* slot = frame.comprehension_slots().Get(0); + ASSERT_TRUE(slot != nullptr); + EXPECT_TRUE(slot->value->Is() && + slot->value->As().NativeValue() == 42); + EXPECT_FALSE(frame.value_stack().empty()); +} + +TEST_F(LazyInitStepTest, CreateAssignSlotAndPopStepBasic) { + ExecutionPath path; + + path.push_back(CreateAssignSlotAndPopStep(0)); + + ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); + frame.comprehension_slots().ClearSlot(0); + + frame.value_stack().Push(value_factory().CreateIntValue(42)); + + // This will error because no return value, step will still evaluate. + frame.Evaluate(EvaluationListener()).IgnoreError(); + + auto* slot = frame.comprehension_slots().Get(0); + ASSERT_TRUE(slot != nullptr); + EXPECT_TRUE(slot->value->Is() && + slot->value->As().NativeValue() == 42); + EXPECT_TRUE(frame.value_stack().empty()); +} + +TEST_F(LazyInitStepTest, CreateAssignSlotStepStackUnderflow) { + ExecutionPath path; + + path.push_back(CreateAssignSlotStep(0)); + + ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); + frame.comprehension_slots().ClearSlot(0); + + EXPECT_THAT(frame.Evaluate(EvaluationListener()), + StatusIs(absl::StatusCode::kInternal, + HasSubstr("Stack underflow assigning lazy value"))); +} + +TEST_F(LazyInitStepTest, CreateClearSlotStepBasic) { + ExecutionPath path; + + path.push_back(CreateClearSlotStep(0, -1)); + + ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); + frame.comprehension_slots().Set(0, value_factory().CreateIntValue(42)); + + // This will error because no return value, step will still evaluate. + frame.Evaluate(EvaluationListener()).IgnoreError(); + + auto* slot = frame.comprehension_slots().Get(0); + ASSERT_TRUE(slot == nullptr); +} + +} // namespace +} // namespace google::api::expr::runtime diff --git a/eval/public/cel_options.cc b/eval/public/cel_options.cc index b9df73623..d22434eaf 100644 --- a/eval/public/cel_options.cc +++ b/eval/public/cel_options.cc @@ -19,26 +19,25 @@ namespace google::api::expr::runtime { cel::RuntimeOptions ConvertToRuntimeOptions(const InterpreterOptions& options) { - return cel::RuntimeOptions{ - /*.container=*/"", - options.unknown_processing, - options.enable_missing_attribute_errors, - options.enable_timestamp_duration_overflow_errors, - options.short_circuiting, - options.enable_comprehension, - options.comprehension_max_iterations, - options.enable_comprehension_list_append, - options.enable_regex, - options.regex_max_program_size, - options.enable_string_conversion, - options.enable_string_concat, - options.enable_list_concat, - options.enable_list_contains, - options.fail_on_warnings, - options.enable_qualified_type_identifiers, - options.enable_heterogeneous_equality, - options.enable_empty_wrapper_null_unboxing, - }; + return cel::RuntimeOptions{/*.container=*/"", + options.unknown_processing, + options.enable_missing_attribute_errors, + options.enable_timestamp_duration_overflow_errors, + options.short_circuiting, + options.enable_comprehension, + options.comprehension_max_iterations, + options.enable_comprehension_list_append, + options.enable_regex, + options.regex_max_program_size, + options.enable_string_conversion, + options.enable_string_concat, + options.enable_list_concat, + options.enable_list_contains, + options.fail_on_warnings, + options.enable_qualified_type_identifiers, + options.enable_heterogeneous_equality, + options.enable_empty_wrapper_null_unboxing, + options.enable_lazy_bind_initialization}; } } // namespace google::api::expr::runtime diff --git a/eval/public/cel_options.h b/eval/public/cel_options.h index 6adbb15bc..80697c266 100644 --- a/eval/public/cel_options.h +++ b/eval/public/cel_options.h @@ -157,6 +157,9 @@ struct InterpreterOptions { // Note: implementation in progress -- please consult the CEL team before // enabling in an existing environment. bool enable_select_optimization = false; + + // TODO(uncreated-issue/63): Do not use -- implementation in progress. + bool enable_lazy_bind_initialization = false; }; // LINT.ThenChange(//depot/google3/runtime/runtime_options.h) diff --git a/extensions/BUILD b/extensions/BUILD index b514a216e..604989aca 100644 --- a/extensions/BUILD +++ b/extensions/BUILD @@ -154,6 +154,7 @@ cc_test( "//eval/public:builtin_func_registrar", "//eval/public:cel_expr_builder_factory", "//eval/public:cel_expression", + "//eval/public:cel_options", "//eval/public:cel_value", "//eval/public/testing:matchers", "//internal:benchmark", diff --git a/extensions/bindings_ext_benchmark_test.cc b/extensions/bindings_ext_benchmark_test.cc index 6a43d00c6..f0395bbcc 100644 --- a/extensions/bindings_ext_benchmark_test.cc +++ b/extensions/bindings_ext_benchmark_test.cc @@ -22,6 +22,7 @@ #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_expr_builder_factory.h" #include "eval/public/cel_expression.h" +#include "eval/public/cel_options.h" #include "eval/public/cel_value.h" #include "eval/public/testing/matchers.h" #include "extensions/bindings_ext.h" @@ -38,6 +39,7 @@ namespace { using ::google::api::expr::parser::ParseWithMacros; using ::google::api::expr::runtime::Activation; using ::google::api::expr::runtime::CelValue; +using ::google::api::expr::runtime::InterpreterOptions; using ::google::api::expr::runtime::test::CelValueMatcher; using ::google::api::expr::runtime::test::IsCelBool; using ::google::api::expr::runtime::test::IsCelString; @@ -62,8 +64,8 @@ const std::vector& BenchmarkCases() { cel.bind( y, "cd", - x + y + x)))", - IsCelString("abcdab")}, + x + y + "ef")))", + IsCelString("abcdef")}, {"nested_defintion", R"( cel.bind( @@ -72,9 +74,9 @@ const std::vector& BenchmarkCases() { cel.bind( y, x + "cd", - y + x + y + "ef" )))", - IsCelString("abcdab")}, + IsCelString("abcdef")}, {"bind_outside_loop", R"( cel.bind( @@ -106,14 +108,36 @@ const std::vector& BenchmarkCases() { ) )))", IsCelBool(true)}, - {"ternary_dependant", + {"ternary_depends_on_bind", R"( cel.bind( a, "ab", - (false || a.startsWith("c")) ? a : "cd" + (true && a.startsWith("c")) ? a : "cd" ))", IsCelString("cd")}, + {"ternary_does_not_depend_on_bind", + R"( + cel.bind( + a, + "ab", + (false && a.startsWith("c")) ? a : "cd" + ))", + IsCelString("cd")}, + {"twice_nested_defintion", + R"( + cel.bind( + x, + "ab", + cel.bind( + y, + x + "cd", + cel.bind( + z, + y + "ef", + z))) + )", + IsCelString("abcdef")}, }); return *cases; @@ -134,7 +158,10 @@ TEST_P(BindingsBenchmarkTest, CheckBenchmarkCaseWorks) { ASSERT_OK_AND_ASSIGN( auto expr, ParseWithMacros(benchmark.expression, all_macros, "")); - auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(); + InterpreterOptions options; + options.enable_lazy_bind_initialization = true; + auto builder = + google::api::expr::runtime::CreateCelExpressionBuilder(options); ASSERT_OK(google::api::expr::runtime::RegisterBuiltinFunctions( builder->GetRegistry())); @@ -157,7 +184,10 @@ void RunBenchmark(const BenchmarkCase& benchmark, benchmark::State& state) { ASSERT_OK_AND_ASSIGN( auto expr, ParseWithMacros(benchmark.expression, all_macros, "")); - auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(); + InterpreterOptions options; + options.enable_lazy_bind_initialization = true; + auto builder = + google::api::expr::runtime::CreateCelExpressionBuilder(options); ASSERT_OK(google::api::expr::runtime::RegisterBuiltinFunctions( builder->GetRegistry())); @@ -196,9 +226,15 @@ void BM_BindInsideLoop(benchmark::State& state) { void BM_BindLoopBind(benchmark::State& state) { RunBenchmark(BenchmarkCases()[6], state); } -void BM_TernaryDependant(benchmark::State& state) { +void BM_TernaryDependsOnBind(benchmark::State& state) { RunBenchmark(BenchmarkCases()[7], state); } +void BM_TernaryDoesNotDependOnBind(benchmark::State& state) { + RunBenchmark(BenchmarkCases()[8], state); +} +void BM_TwiceNestedDefinition(benchmark::State& state) { + RunBenchmark(BenchmarkCases()[9], state); +} BENCHMARK(BM_Simple); BENCHMARK(BM_MultipleReferences); @@ -207,7 +243,9 @@ BENCHMARK(BM_NestedDefinition); BENCHMARK(BM_BindOusideLoop); BENCHMARK(BM_BindInsideLoop); BENCHMARK(BM_BindLoopBind); -BENCHMARK(BM_TernaryDependant); +BENCHMARK(BM_TernaryDependsOnBind); +BENCHMARK(BM_TernaryDoesNotDependOnBind); +BENCHMARK(BM_TwiceNestedDefinition); INSTANTIATE_TEST_SUITE_P(BindingsBenchmarkTest, BindingsBenchmarkTest, ::testing::ValuesIn(BenchmarkCases())); diff --git a/extensions/bindings_ext_test.cc b/extensions/bindings_ext_test.cc index 3b70c5bb3..27deca9aa 100644 --- a/extensions/bindings_ext_test.cc +++ b/extensions/bindings_ext_test.cc @@ -78,10 +78,11 @@ 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) { @@ -109,6 +110,7 @@ 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())); @@ -148,7 +150,8 @@ 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())); + /*constant_folding*/ testing::Bool(), + /*lazy_bindings*/ testing::Bool())); } // namespace } // namespace cel::extensions diff --git a/runtime/runtime_options.h b/runtime/runtime_options.h index 7cf1fcc58..c69a53c0d 100644 --- a/runtime/runtime_options.h +++ b/runtime/runtime_options.h @@ -128,6 +128,9 @@ struct RuntimeOptions { // that will result in a Null cel value, as opposed to returning the // cel representation of the proto defined default int64_t: 0. bool enable_empty_wrapper_null_unboxing = false; + + // TODO(uncreated-issue/63): Do not use -- implementation in progress. + bool enable_lazy_bind_initialization = false; }; // LINT.ThenChange(//depot/google3/eval/public/cel_options.h)