diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 01a5d4990..4ffbd83f4 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -16,6 +16,7 @@ #include "eval/compiler/flat_expr_builder.h" +#include #include #include #include @@ -118,6 +119,28 @@ class IndexManager { size_t max_slot_count_; }; +// Estimate the maximum stack requirements for a subexpression. +// +// Maintain a running sum of the program Steps reported estimated stack change +// upper bound. +// +// Assumptions: Jump steps and Call steps complicate this, but this should still +// be valid for supported CEL expressions. +// - While abstractly a Jump may invalidate this algorithm (e.g. deterministic +// jump over a stack shrinking step), the expression builder should not create +// programs where this is possible. +// - Call steps are currently limited to lazy initializing cel.bind expressions, +// and report the estimated stack requirements for the called subexpression. +size_t EstimateStackRequirement(const ExecutionPath& execution_path) { + int stack_estimate = 0; + int stack_limit = 1; + for (const auto& step : execution_path) { + stack_estimate += step->stack_delta(); + stack_limit = std::max(stack_limit, stack_estimate); + } + return static_cast(stack_limit); +} + // A convenience wrapper for offset-calculating logic. class Jump { public: @@ -404,6 +427,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { struct SlotLookupResult { int slot; int subexpression; + int stack_requirement; }; // Helper to lookup a variable mapped to a slot. @@ -428,10 +452,12 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { record.comprehension->accu_var() == path) { int slot = record.accu_slot; int subexpression = -1; + int stack_requirement = -1; if (record.should_lazy_eval) { subexpression = record.subexpression; + stack_requirement = record.subexpression_stack_requirement; } - return {slot, subexpression}; + return {slot, subexpression, stack_requirement}; } } } @@ -490,9 +516,9 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { SlotLookupResult slot = LookupSlot(path); if (slot.subexpression >= 0) { - AddStep( - CreateCheckLazyInitStep(slot.slot, slot.subexpression, expr->id())); - AddStep(CreateAssignSlotStep(slot.slot)); + AddStep(CreateCheckLazyInitStep(slot.slot, slot.subexpression, + slot.stack_requirement, expr->id())); + AddStep(CreateAssignSlotStep(slot.slot, (1 - slot.stack_requirement))); return; } else if (slot.slot >= 0) { AddStep(CreateIdentStepForSlot(*ident_expr, slot.slot, expr->id())); @@ -728,6 +754,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { comprehension_stack_.push_back( {expr, comprehension, iter_slot, accu_slot, /*subexpression=*/-1, + /*subexpression_stack_requirement=*/-1, IsOptimizableListAppend(comprehension, options_.enable_comprehension_list_append), is_bind, @@ -980,6 +1007,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { size_t accu_slot; // -1 indicates this shouldn't be used. int subexpression; + int subexpression_stack_requirement; bool is_optimizable_list_append; bool is_optimizable_bind; bool iter_var_in_scope; @@ -1012,6 +1040,8 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor { // off by one since mainline expression is handled separately. size_t id = expression_table_.size(); record.subexpression = id; + record.subexpression_stack_requirement = + static_cast(EstimateStackRequirement(expression_table_.back())); record.visitor->MarkAccuInitExtracted(); @@ -1340,11 +1370,13 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( (*issues) = issue_collector.ExtractIssues(); } + size_t stack_limit = EstimateStackRequirement(execution_path); + std::vector subexpressions = FlattenExpressionTable(expression_table, execution_path); return FlatExpression(std::move(execution_path), std::move(subexpressions), - visitor.slot_count(), + stack_limit, visitor.slot_count(), type_registry_.GetComposedTypeProvider(), options_); } diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 3712424b3..25d51338c 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -438,6 +438,7 @@ cc_test( deps = [ ":cel_expression_flat_impl", ":evaluator_core", + ":expression_step_base", "//base:data", "//eval/compiler:cel_expression_builder_flat_impl", "//eval/internal:interop", diff --git a/eval/eval/compiler_constant_step.h b/eval/eval/compiler_constant_step.h index 3813e69c6..c50d7e60a 100644 --- a/eval/eval/compiler_constant_step.h +++ b/eval/eval/compiler_constant_step.h @@ -30,7 +30,8 @@ class CompilerConstantStep : public ExpressionStepBase { public: CompilerConstantStep(cel::Handle value, int64_t expr_id, bool comes_from_ast) - : ExpressionStepBase(expr_id, comes_from_ast), value_(std::move(value)) {} + : ExpressionStepBase(expr_id, comes_from_ast, /*stack_delta=*/1), + value_(std::move(value)) {} absl::Status Evaluate(ExecutionFrame* frame) const override; diff --git a/eval/eval/comprehension_step.cc b/eval/eval/comprehension_step.cc index 79d448484..88f53604c 100644 --- a/eval/eval/comprehension_step.cc +++ b/eval/eval/comprehension_step.cc @@ -44,7 +44,8 @@ class ComprehensionFinish : public ExpressionStepBase { }; ComprehensionFinish::ComprehensionFinish(size_t accu_slot, int64_t expr_id) - : ExpressionStepBase(expr_id), accu_slot_(accu_slot) {} + : ExpressionStepBase(expr_id, true, /*stack_delta=*/-2), + accu_slot_(accu_slot) {} // Stack changes of ComprehensionFinish. // @@ -73,7 +74,7 @@ absl::Status ComprehensionFinish::Evaluate(ExecutionFrame* frame) const { class ComprehensionInitStep : public ExpressionStepBase { public: explicit ComprehensionInitStep(int64_t expr_id) - : ExpressionStepBase(expr_id, false) {} + : ExpressionStepBase(expr_id, false, /*stack_delta=*/1) {} absl::Status Evaluate(ExecutionFrame* frame) const override; private: @@ -148,7 +149,7 @@ absl::Status ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { ComprehensionNextStep::ComprehensionNextStep(size_t iter_slot, size_t accu_slot, int64_t expr_id) - : ExpressionStepBase(expr_id, false), + : ExpressionStepBase(expr_id, false, /*stack_delta=*/-1), iter_slot_(iter_slot), accu_slot_(accu_slot) {} @@ -252,7 +253,7 @@ absl::Status ComprehensionNextStep::Evaluate(ExecutionFrame* frame) const { ComprehensionCondStep::ComprehensionCondStep(size_t iter_slot, size_t accu_slot, bool shortcircuiting, int64_t expr_id) - : ExpressionStepBase(expr_id, false), + : ExpressionStepBase(expr_id, false, /*stack_delta=*/-1), iter_slot_(iter_slot), accu_slot_(accu_slot), shortcircuiting_(shortcircuiting) {} diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index 978450d9c..ec82f4b29 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -49,9 +49,9 @@ class ListKeysStepTest : public testing::Test { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; } - return std::make_unique( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + return std::make_unique(FlatExpression( + std::move(path), /*value_stack_size=*/2, /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), options)); } private: diff --git a/eval/eval/const_value_step_test.cc b/eval/eval/const_value_step_test.cc index 293414977..7ae2c6dc9 100644 --- a/eval/eval/const_value_step_test.cc +++ b/eval/eval/const_value_step_test.cc @@ -44,9 +44,11 @@ absl::StatusOr RunConstantExpression( google::api::expr::runtime::ExecutionPath path; path.push_back(std::move(step)); - CelExpressionFlatImpl impl( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); google::api::expr::runtime::Activation activation; diff --git a/eval/eval/container_access_step.cc b/eval/eval/container_access_step.cc index 112eb572a..6ceb62797 100644 --- a/eval/eval/container_access_step.cc +++ b/eval/eval/container_access_step.cc @@ -48,7 +48,8 @@ inline constexpr int kNumContainerAccessArguments = 2; // message. class ContainerAccessStep : public ExpressionStepBase { public: - explicit ContainerAccessStep(int64_t expr_id) : ExpressionStepBase(expr_id) {} + explicit ContainerAccessStep(int64_t expr_id) + : ExpressionStepBase(expr_id, true, /*stack_delta=*/-1) {} absl::Status Evaluate(ExecutionFrame* frame) const override; diff --git a/eval/eval/container_access_step_test.cc b/eval/eval/container_access_step_test.cc index 880b0faec..0a3e787a8 100644 --- a/eval/eval/container_access_step_test.cc +++ b/eval/eval/container_access_step_test.cc @@ -72,9 +72,10 @@ CelValue EvaluateAttributeHelper( cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; options.enable_heterogeneous_equality = false; - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/2, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; activation.InsertValue("container", container); diff --git a/eval/eval/create_list_step.cc b/eval/eval/create_list_step.cc index 1f7dfe649..7903dbd16 100644 --- a/eval/eval/create_list_step.cc +++ b/eval/eval/create_list_step.cc @@ -31,7 +31,7 @@ using ::cel::runtime_internal::MutableListValue; class CreateListStep : public ExpressionStepBase { public: CreateListStep(int64_t expr_id, int list_size, bool immutable) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(expr_id, true, /*stack_delta=*/1 - list_size), list_size_(list_size), immutable_(immutable) {} diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index c49a22777..adfefdf80 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -54,11 +54,11 @@ absl::StatusOr RunExpression(const std::vector& values, if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl cel_expr( - - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, TypeProvider::Builtin(), - options)); + size_t stack_size = path.size(); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/stack_size, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; return cel_expr.Evaluate(activation, arena); @@ -95,9 +95,10 @@ absl::StatusOr RunExpressionWithCelValues( options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/values.size(), + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); return cel_expr.Evaluate(activation, arena); } @@ -118,9 +119,11 @@ TEST(CreateListStepTest, TestCreateListStackUnderflow) { CreateCreateListStep(create_list, dummy_expr.id())); path.push_back(std::move(step0)); - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl cel_expr(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); Activation activation; google::protobuf::Arena arena; diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index 74fd00aed..afe5fe789 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -1,5 +1,6 @@ #include "eval/eval/create_struct_step.h" +#include #include #include #include @@ -815,7 +816,7 @@ class CreateStructStepForStruct final : public ExpressionStepBase { public: CreateStructStepForStruct(int64_t expr_id, Handle type, std::vector entries) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(expr_id, true, 1 - entries.size()), type_(std::move(type)), entries_(std::move(entries)) {} @@ -853,7 +854,8 @@ class CreateStructStepForWellKnownType final : public ExpressionStepBase { class CreateStructStepForMap final : public ExpressionStepBase { public: CreateStructStepForMap(int64_t expr_id, size_t entry_count) - : ExpressionStepBase(expr_id), entry_count_(entry_count) {} + : ExpressionStepBase(expr_id, true, 1 - (2 * entry_count)), + entry_count_(entry_count) {} absl::Status Evaluate(ExecutionFrame* frame) const override; diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index 49c095486..2b2434461 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -94,9 +94,10 @@ absl::StatusOr RunExpression(absl::string_view field, if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; activation.InsertValue("message", value); @@ -187,9 +188,10 @@ absl::StatusOr RunCreateMapExpression( options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/2 * values.size(), + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); return cel_expr.Evaluate(activation, arena); } @@ -226,9 +228,10 @@ TEST_P(CreateCreateStructStepTest, TestEmptyMessageCreation) { if (GetParam()) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena)); diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index af988ea35..313b66491 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -112,14 +112,14 @@ absl::StatusOr> ExecutionFrame::Evaluate( FlatExpressionEvaluatorState FlatExpression::MakeEvaluatorState( cel::MemoryManagerRef manager) const { - return FlatExpressionEvaluatorState(path_.size(), comprehension_slots_size_, - type_provider_, manager); + return FlatExpressionEvaluatorState( + value_stack_size_, comprehension_slots_size_, type_provider_, manager); } FlatExpressionEvaluatorState FlatExpression::MakeEvaluatorState( cel::ValueFactory& value_factory) const { - return FlatExpressionEvaluatorState(path_.size(), comprehension_slots_size_, - value_factory); + return FlatExpressionEvaluatorState(value_stack_size_, + comprehension_slots_size_, value_factory); } absl::StatusOr> FlatExpression::EvaluateWithCallback( diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 4723bf755..7791e8676 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -76,6 +76,11 @@ class ExpressionStep { // Returns if the execution step comes from AST. virtual bool ComesFromAst() const = 0; + // Returns a tight upper bound for stack changes required for this expression. + // This is used to compute a reasonable size for the value stack to + // pre-allocate. + virtual int stack_delta() const = 0; + // Return the type of the underlying expression step for special handling in // the planning phase. This should only be overridden by special cases, and // callers must not make any assumptions about the default case. @@ -297,22 +302,25 @@ class FlatExpression { // path is flat execution path that is based upon the flattened AST tree // type_provider is the configured type system that should be used for // value creation in evaluation - FlatExpression(ExecutionPath path, size_t comprehension_slots_size, + FlatExpression(ExecutionPath path, size_t value_stack_size, + size_t comprehension_slots_size, const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options) : path_(std::move(path)), subexpressions_({path_}), + value_stack_size_(value_stack_size), comprehension_slots_size_(comprehension_slots_size), type_provider_(type_provider), options_(options) {} FlatExpression(ExecutionPath path, std::vector subexpressions, - size_t comprehension_slots_size, + size_t value_stack_size, size_t comprehension_slots_size, const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options) : path_(std::move(path)), subexpressions_(std::move(subexpressions)), + value_stack_size_(value_stack_size), comprehension_slots_size_(comprehension_slots_size), type_provider_(type_provider), options_(options) {} @@ -345,6 +353,7 @@ class FlatExpression { private: ExecutionPath path_; std::vector subexpressions_; + size_t value_stack_size_; size_t comprehension_slots_size_; const cel::TypeProvider& type_provider_; cel::RuntimeOptions options_; diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index 21d47101f..1c5073a42 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -7,6 +7,7 @@ #include "base/type_provider.h" #include "eval/compiler/cel_expression_builder_flat_impl.h" #include "eval/eval/cel_expression_flat_impl.h" +#include "eval/eval/expression_step_base.h" #include "eval/internal/interop.h" #include "eval/public/activation.h" #include "eval/public/builtin_func_registrar.h" @@ -28,26 +29,21 @@ using testing::Eq; // Fake expression implementation // Pushes int64_t(0) on top of value stack. -class FakeConstExpressionStep : public ExpressionStep { +class FakeConstExpressionStep : public ExpressionStepBase { public: + FakeConstExpressionStep() : ExpressionStepBase(0) {} absl::Status Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(CreateIntValue(0)); return absl::OkStatus(); } - - int64_t id() const override { return 0; } - - bool ComesFromAst() const override { return true; } - - cel::NativeTypeId GetNativeTypeId() const override { - return cel::NativeTypeId(); - } }; // Fake expression implementation // Increments argument on top of the stack. -class FakeIncrementExpressionStep : public ExpressionStep { +class FakeIncrementExpressionStep : public ExpressionStepBase { public: + FakeIncrementExpressionStep() : ExpressionStepBase(0) {} + absl::Status Evaluate(ExecutionFrame* frame) const override { CelValue value = cel::interop_internal::ModernValueToLegacyValueOrDie( frame->memory_manager(), frame->value_stack().Peek()); @@ -57,14 +53,6 @@ class FakeIncrementExpressionStep : public ExpressionStep { frame->value_stack().Push(CreateIntValue(val + 1)); return absl::OkStatus(); } - - int64_t id() const override { return 0; } - - bool ComesFromAst() const override { return true; } - - cel::NativeTypeId GetNativeTypeId() const override { - return cel::NativeTypeId(); - } }; TEST(EvaluatorCoreTest, ExecutionFrameNext) { @@ -105,8 +93,9 @@ TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { path.push_back(std::move(incr_step1)); path.push_back(std::move(incr_step2)); - CelExpressionFlatImpl impl(FlatExpression( - std::move(path), 0, cel::TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl impl(FlatExpression(std::move(path), 1, 0, + cel::TypeProvider::Builtin(), + cel::RuntimeOptions{})); Activation activation; google::protobuf::Arena arena; diff --git a/eval/eval/expression_step_base.h b/eval/eval/expression_step_base.h index 2da38aef3..277623434 100644 --- a/eval/eval/expression_step_base.h +++ b/eval/eval/expression_step_base.h @@ -9,8 +9,11 @@ namespace google::api::expr::runtime { class ExpressionStepBase : public ExpressionStep { public: - explicit ExpressionStepBase(int64_t expr_id, bool comes_from_ast = true) - : id_(expr_id), comes_from_ast_(comes_from_ast) {} + explicit ExpressionStepBase(int64_t expr_id, bool comes_from_ast = true, + int stack_delta = 1) + : id_(expr_id), + stack_delta_(stack_delta), + comes_from_ast_(comes_from_ast) {} // Non-copyable ExpressionStepBase(const ExpressionStepBase&) = delete; @@ -19,6 +22,8 @@ class ExpressionStepBase : public ExpressionStep { // Returns corresponding expression object ID. int64_t id() const override { return id_; } + int stack_delta() const override { return stack_delta_; } + // Returns if the execution step comes from AST. bool ComesFromAst() const override { return comes_from_ast_; } @@ -28,6 +33,7 @@ class ExpressionStepBase : public ExpressionStep { private: int64_t id_; + int stack_delta_; bool comes_from_ast_; }; diff --git a/eval/eval/function_step.cc b/eval/eval/function_step.cc index e79caa791..c9396603f 100644 --- a/eval/eval/function_step.cc +++ b/eval/eval/function_step.cc @@ -135,7 +135,7 @@ class AbstractFunctionStep : public ExpressionStepBase { // Constructs FunctionStep that uses overloads specified. AbstractFunctionStep(const std::string& name, size_t num_arguments, int64_t expr_id) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(expr_id, true, /*stack_delta=*/1 - num_arguments), name_(name), num_arguments_(num_arguments) {} diff --git a/eval/eval/function_step_test.cc b/eval/eval/function_step_test.cc index 83caa3c63..7310c3061 100644 --- a/eval/eval/function_step_test.cc +++ b/eval/eval/function_step_test.cc @@ -1,6 +1,7 @@ #include "eval/eval/function_step.h" #include +#include #include #include #include @@ -217,10 +218,11 @@ class FunctionStepTest std::unique_ptr GetExpression(ExecutionPath&& path) { cel::RuntimeOptions options; options.unknown_processing = GetParam(); - - return std::make_unique( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + size_t value_stack_size = path.size(); + return std::make_unique(FlatExpression( + std::move(path), + /*value_stack_size=*/value_stack_size, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); } }; @@ -563,10 +565,11 @@ class FunctionStepTestUnknowns std::unique_ptr GetExpression(ExecutionPath&& path) { cel::RuntimeOptions options; options.unknown_processing = GetParam(); - - return std::make_unique( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + size_t stack_size = path.size(); + return std::make_unique(FlatExpression( + std::move(path), + /*value_stack_size=*/stack_size, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); } }; @@ -702,6 +705,8 @@ TEST(FunctionStepTestUnknownFunctionResults, CaptureArgs) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); @@ -749,6 +754,7 @@ TEST(FunctionStepTestUnknownFunctionResults, MergeDownCaptureArgs) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/3, /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); @@ -796,6 +802,7 @@ TEST(FunctionStepTestUnknownFunctionResults, MergeCaptureArgs) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/3, /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); @@ -838,6 +845,7 @@ TEST(FunctionStepTestUnknownFunctionResults, UnknownVsErrorPrecedenceTest) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/2, /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); @@ -925,6 +933,7 @@ TEST(FunctionStepStrictnessTest, options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/2, /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; @@ -954,6 +963,8 @@ TEST(FunctionStepStrictnessTest, IfFunctionNonStrictAndGivenUnknownInvokesIt) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/2, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index c1915c647..5b6a14975 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -28,7 +28,7 @@ using ::cel::runtime_internal::CreateMissingAttributeError; class IdentStep : public ExpressionStepBase { public: IdentStep(absl::string_view name, int64_t expr_id) - : ExpressionStepBase(expr_id), name_(name) {} + : ExpressionStepBase(expr_id, true, /*stack_delta=*/1), name_(name) {} absl::Status Evaluate(ExecutionFrame* frame) const override; @@ -93,7 +93,9 @@ absl::Status IdentStep::Evaluate(ExecutionFrame* frame) const { class SlotStep : public ExpressionStepBase { public: SlotStep(absl::string_view name, size_t slot_index, int64_t expr_id) - : ExpressionStepBase(expr_id), name_(name), slot_index_(slot_index) {} + : ExpressionStepBase(expr_id, true, /*stack_delta=*/1), + name_(name), + slot_index_(slot_index) {} absl::Status Evaluate(ExecutionFrame* frame) const override; diff --git a/eval/eval/ident_step_test.cc b/eval/eval/ident_step_test.cc index f21b447ca..021338830 100644 --- a/eval/eval/ident_step_test.cc +++ b/eval/eval/ident_step_test.cc @@ -31,9 +31,11 @@ TEST(IdentStepTest, TestIdentStep) { ExecutionPath path; path.push_back(std::move(step)); - CelExpressionFlatImpl impl( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); Activation activation; Arena arena; @@ -59,9 +61,11 @@ TEST(IdentStepTest, TestIdentStepNameNotFound) { ExecutionPath path; path.push_back(std::move(step)); - CelExpressionFlatImpl impl( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); Activation activation; Arena arena; @@ -86,6 +90,8 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); @@ -126,6 +132,8 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { options.enable_missing_attribute_errors = true; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); @@ -166,6 +174,8 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) { cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); diff --git a/eval/eval/jump_step.h b/eval/eval/jump_step.h index ef52ca343..1d688d878 100644 --- a/eval/eval/jump_step.h +++ b/eval/eval/jump_step.h @@ -14,7 +14,8 @@ namespace google::api::expr::runtime { class JumpStepBase : public ExpressionStepBase { public: JumpStepBase(absl::optional jump_offset, int64_t expr_id) - : ExpressionStepBase(expr_id, false), jump_offset_(jump_offset) {} + : ExpressionStepBase(expr_id, false, /*stack_delta=*/0), + jump_offset_(jump_offset) {} void set_jump_offset(int offset) { jump_offset_ = offset; } diff --git a/eval/eval/lazy_init_step.cc b/eval/eval/lazy_init_step.cc index da06c7bb7..9e8eab3a0 100644 --- a/eval/eval/lazy_init_step.cc +++ b/eval/eval/lazy_init_step.cc @@ -29,8 +29,8 @@ namespace { class CheckLazyInitStep : public ExpressionStepBase { public: CheckLazyInitStep(size_t slot_index, size_t subexpression_index, - int64_t expr_id) - : ExpressionStepBase(expr_id), + int stack_delta, int64_t expr_id) + : ExpressionStepBase(expr_id, /*comes_from_ast=*/true, stack_delta), slot_index_(slot_index), subexpression_index_(subexpression_index) {} @@ -54,8 +54,10 @@ class CheckLazyInitStep : public ExpressionStepBase { class AssignSlotStep : public ExpressionStepBase { public: - explicit AssignSlotStep(size_t slot_index, bool should_pop) - : ExpressionStepBase(/*expr_id=*/-1, /*comes_from_ast=*/false), + explicit AssignSlotStep(size_t slot_index, bool should_pop, int stack_delta) + : ExpressionStepBase(/*expr_id=*/-1, + /*comes_from_ast=*/false, + /*stack_delta=*/stack_delta), slot_index_(slot_index), should_pop_(should_pop) {} @@ -82,7 +84,10 @@ class AssignSlotStep : public ExpressionStepBase { class ClearSlotStep : public ExpressionStepBase { public: explicit ClearSlotStep(size_t slot_index, int64_t expr_id) - : ExpressionStepBase(expr_id), slot_index_(slot_index) {} + : ExpressionStepBase(expr_id, + /*comes_from_ast=*/true, + /*stack_delta=*/0), + slot_index_(slot_index) {} absl::Status Evaluate(ExecutionFrame* frame) const override { frame->comprehension_slots().ClearSlot(slot_index_); @@ -96,17 +101,21 @@ class ClearSlotStep : public ExpressionStepBase { } // namespace std::unique_ptr CreateCheckLazyInitStep( - size_t slot_index, size_t subexpression_index, int64_t expr_id) { + size_t slot_index, size_t subexpression_index, int stack_delta, + int64_t expr_id) { return std::make_unique(slot_index, subexpression_index, - expr_id); + stack_delta, expr_id); } -std::unique_ptr CreateAssignSlotStep(size_t slot_index) { - return std::make_unique(slot_index, /*should_pop=*/false); +std::unique_ptr CreateAssignSlotStep(size_t slot_index, + int stack_delta) { + return std::make_unique(slot_index, /*should_pop=*/false, + stack_delta); } std::unique_ptr CreateAssignSlotAndPopStep(size_t slot_index) { - return std::make_unique(slot_index, /*should_pop=*/true); + return std::make_unique(slot_index, /*should_pop=*/true, + /*stack_delta=*/-1); } std::unique_ptr CreateClearSlotStep(size_t slot_index, diff --git a/eval/eval/lazy_init_step.h b/eval/eval/lazy_init_step.h index 5733afa7f..afbbb7a60 100644 --- a/eval/eval/lazy_init_step.h +++ b/eval/eval/lazy_init_step.h @@ -49,11 +49,20 @@ namespace google::api::expr::runtime { // 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. +// +// stack_delta should be the worst case stack requirement initializing (calling +// the subexpression). std::unique_ptr CreateCheckLazyInitStep( - size_t slot_index, size_t subexpression_index, int64_t expr_id); + size_t slot_index, size_t subexpression_index, int stack_delta, + 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); +// +// stack_delta is used along with the corresponding CheckLazyInitStep to offset +// the worst case stack growth if the subexpression is initialized at that +// point. +std::unique_ptr CreateAssignSlotStep(size_t slot_index, + int stack_delta); std::unique_ptr CreateAssignSlotAndPopStep(size_t slot_index); // Helper step to clear a slot. diff --git a/eval/eval/lazy_init_step_test.cc b/eval/eval/lazy_init_step_test.cc index f0cb2d487..5d4eb46f9 100644 --- a/eval/eval/lazy_init_step_test.cc +++ b/eval/eval/lazy_init_step_test.cc @@ -46,13 +46,13 @@ 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 kValueStackSize = 5; static constexpr size_t kComprehensionSlotCount = 3; public: LazyInitStepTest() : value_factory_(TypeProvider::Builtin(), ProtoMemoryManagerRef(&arena_)), - evaluator_state_(kValueStack, kComprehensionSlotCount, + evaluator_state_(kValueStackSize, kComprehensionSlotCount, value_factory_.get()) {} protected: @@ -70,7 +70,7 @@ TEST_F(LazyInitStepTest, CreateCheckInitStepDoesInit) { ExecutionPath subpath; path.push_back(CreateCheckLazyInitStep(/*slot_index=*/0, - /*subexpression_index=*/1, -1)); + /*subexpression_index=*/1, 1, -1)); ASSERT_OK_AND_ASSIGN( subpath.emplace_back(), @@ -91,11 +91,12 @@ TEST_F(LazyInitStepTest, CreateCheckInitStepSkipInit) { ExecutionPath subpath; path.push_back(CreateCheckLazyInitStep(/*slot_index=*/0, - /*subexpression_index=*/1, -1)); + /*subexpression_index=*/1, + /*stack_requirement=*/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(CreateAssignSlotStep(/*slot_index=*/0, 1)); path.push_back(CreateClearSlotStep(/*slot_index=*/0, -1)); ASSERT_OK_AND_ASSIGN( @@ -116,7 +117,7 @@ TEST_F(LazyInitStepTest, CreateCheckInitStepSkipInit) { TEST_F(LazyInitStepTest, CreateAssignSlotStepBasic) { ExecutionPath path; - path.push_back(CreateAssignSlotStep(0)); + path.push_back(CreateAssignSlotStep(0, 1)); ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); frame.comprehension_slots().ClearSlot(0); @@ -156,7 +157,7 @@ TEST_F(LazyInitStepTest, CreateAssignSlotAndPopStepBasic) { TEST_F(LazyInitStepTest, CreateAssignSlotStepStackUnderflow) { ExecutionPath path; - path.push_back(CreateAssignSlotStep(0)); + path.push_back(CreateAssignSlotStep(0, 1)); ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); frame.comprehension_slots().ClearSlot(0); diff --git a/eval/eval/logic_step.cc b/eval/eval/logic_step.cc index 941a60726..e58d4fec7 100644 --- a/eval/eval/logic_step.cc +++ b/eval/eval/logic_step.cc @@ -30,7 +30,8 @@ class LogicalOpStep : public ExpressionStepBase { // Constructs FunctionStep that uses overloads specified. LogicalOpStep(OpType op_type, int64_t expr_id) - : ExpressionStepBase(expr_id), op_type_(op_type) { + : ExpressionStepBase(expr_id, true, /*stack_delta=*/-1), + op_type_(op_type) { shortcircuit_ = (op_type_ == OpType::OR); } diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index e2a083635..7f474a63a 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -51,9 +51,10 @@ class LogicStepTest : public testing::TestWithParam { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl impl( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl impl(FlatExpression( + std::move(path), + /*value_stack_size=*/2, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; activation.InsertValue("name0", arg0); diff --git a/eval/eval/regex_match_step.cc b/eval/eval/regex_match_step.cc index 674383cfc..839ab2dd2 100644 --- a/eval/eval/regex_match_step.cc +++ b/eval/eval/regex_match_step.cc @@ -32,7 +32,7 @@ inline constexpr size_t kRegexMatchStepSubject = 0; class RegexMatchStep final : public ExpressionStepBase { public: RegexMatchStep(int64_t expr_id, std::shared_ptr re2) - : ExpressionStepBase(expr_id, /*comes_from_ast=*/true), + : ExpressionStepBase(expr_id, /*comes_from_ast=*/true, /*stack_delta=*/0), re2_(std::move(re2)) {} absl::Status Evaluate(ExecutionFrame* frame) const override { diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 2f98529d0..5f199e29f 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -115,7 +115,7 @@ class SelectStep : public ExpressionStepBase { SelectStep(Handle value, bool test_field_presence, int64_t expr_id, absl::string_view select_path, bool enable_wrapper_type_null_unboxing) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(expr_id, true, /*stack_delta=*/0), field_value_(std::move(value)), field_(field_value_->ToString()), test_field_presence_(test_field_presence), diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index 55f72c081..55a2f89c4 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -114,8 +114,10 @@ class SelectStepTest : public testing::Test { cel::UnknownProcessingOptions::kAttributeOnly; } CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), runtime_options)); + FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), + runtime_options)); Activation activation; activation.InsertValue("target", target); @@ -311,9 +313,11 @@ TEST_F(SelectStepTest, MapPresenseIsErrorTest) { path.push_back(std::move(step0)); path.push_back(std::move(step1)); path.push_back(std::move(step2)); - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl cel_expr(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); Activation activation; activation.InsertValue("target", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -818,9 +822,10 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { if (GetParam()) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; activation.InsertValue("message", CelValue::CreateError(&error)); @@ -853,9 +858,11 @@ TEST_F(SelectStepTest, DisableMissingAttributeOK) { path.push_back(std::move(step0)); path.push_back(std::move(step1)); - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl cel_expr(FlatExpression(std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); Activation activation; activation.InsertValue("message", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -897,9 +904,10 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { cel::RuntimeOptions options; options.enable_missing_attribute_errors = true; - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; activation.InsertValue("message", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -946,9 +954,10 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; - CelExpressionFlatImpl cel_expr( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl cel_expr(FlatExpression( + std::move(path), + /*value_stack_size=*/1, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); { std::vector unknown_patterns; diff --git a/eval/eval/shadowable_value_step.cc b/eval/eval/shadowable_value_step.cc index ab4d83b38..f61857ccd 100644 --- a/eval/eval/shadowable_value_step.cc +++ b/eval/eval/shadowable_value_step.cc @@ -16,7 +16,7 @@ class ShadowableValueStep : public ExpressionStepBase { public: ShadowableValueStep(std::string identifier, cel::Handle value, int64_t expr_id) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(expr_id, true, /*stack_delta=*/1), identifier_(std::move(identifier)), value_(std::move(value)) {} diff --git a/eval/eval/shadowable_value_step_test.cc b/eval/eval/shadowable_value_step_test.cc index 38937b75c..6886b3d36 100644 --- a/eval/eval/shadowable_value_step_test.cc +++ b/eval/eval/shadowable_value_step_test.cc @@ -35,9 +35,11 @@ absl::StatusOr RunShadowableExpression(std::string identifier, ExecutionPath path; path.push_back(std::move(step)); - CelExpressionFlatImpl impl( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), cel::RuntimeOptions{})); + CelExpressionFlatImpl impl(FlatExpression(std::move(path), + /*stack_size=*/1, + /*comprehension_slot_count=*/0, + TypeProvider::Builtin(), + cel::RuntimeOptions{})); return impl.Evaluate(activation, arena); } diff --git a/eval/eval/ternary_step.cc b/eval/eval/ternary_step.cc index 2ca8ebbda..4f1a55c47 100644 --- a/eval/eval/ternary_step.cc +++ b/eval/eval/ternary_step.cc @@ -28,7 +28,8 @@ inline constexpr size_t kTernaryStepFalse = 2; class TernaryStep : public ExpressionStepBase { public: // Constructs FunctionStep that uses overloads specified. - explicit TernaryStep(int64_t expr_id) : ExpressionStepBase(expr_id) {} + explicit TernaryStep(int64_t expr_id) + : ExpressionStepBase(expr_id, true, /*stack_delta=*/-2) {} absl::Status Evaluate(ExecutionFrame* frame) const override; }; diff --git a/eval/eval/ternary_step_test.cc b/eval/eval/ternary_step_test.cc index 2f4e71dbc..a86559fef 100644 --- a/eval/eval/ternary_step_test.cc +++ b/eval/eval/ternary_step_test.cc @@ -62,9 +62,10 @@ class LogicStepTest : public testing::TestWithParam { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } - CelExpressionFlatImpl impl( - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - TypeProvider::Builtin(), options)); + CelExpressionFlatImpl impl(FlatExpression( + std::move(path), + /*value_stack_size=*/3, + /*comprehension_slot_count=*/0, TypeProvider::Builtin(), options)); Activation activation; std::string value("test"); diff --git a/extensions/select_optimization.cc b/extensions/select_optimization.cc index 6dff8a8de..c3eab0f27 100644 --- a/extensions/select_optimization.cc +++ b/extensions/select_optimization.cc @@ -551,7 +551,7 @@ class OptimizedSelectStep : public ExpressionStepBase { bool presence_test, bool enable_wrapper_type_null_unboxing, SelectOptimizationOptions options) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(expr_id, true, 0), select_path_(std::move(select_path)), qualifiers_(std::move(qualifiers)), presence_test_(presence_test),