diff --git a/CHANGES b/CHANGES index 3293709613..83997fc6bc 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5.0.0-dev.462 | 2022-05-19 11:45:38 -0700 + + * speed up ZAM compilation by capping function size when inlining (Vern Paxson, Corelight) + 5.0.0-dev.460 | 2022-05-19 11:24:50 -0700 * Zeekify the scripts.base.utils.paths test (Christian Kreibich, Corelight) diff --git a/VERSION b/VERSION index 266c3be4d5..c81e4ef5cf 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -5.0.0-dev.460 +5.0.0-dev.462 diff --git a/src/Expr.cc b/src/Expr.cc index fa3dfaafdf..98fcb637ec 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -110,10 +110,13 @@ const char* expr_name(BroExprTag t) return expr_names[int(t)]; } +int Expr::num_exprs = 0; + Expr::Expr(BroExprTag arg_tag) : tag(arg_tag), paren(false), type(nullptr) { SetLocationInfo(&start_location, &end_location); opt_info = new ExprOptInfo(); + ++num_exprs; } Expr::~Expr() diff --git a/src/Expr.h b/src/Expr.h index f6e45561a7..d000d88538 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -406,6 +406,12 @@ public: // this statement. ExprOptInfo* GetOptInfo() const { return opt_info; } + // Returns the number of expressions created since the last reset. + static int GetNumExprs() { return num_exprs; } + + // Clears the number of expressions created. + static void ResetNumExprs() { num_exprs = 0; } + ~Expr() override; protected: @@ -441,6 +447,9 @@ protected: // Information associated with the Expr for purposes of // script optimization. ExprOptInfo* opt_info; + + // Number of expressions created thus far. + static int num_exprs; }; class NameExpr final : public Expr diff --git a/src/Stmt.cc b/src/Stmt.cc index 00d29e8eb6..0c1265fbfb 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -59,6 +59,8 @@ const char* stmt_name(StmtTag t) return stmt_names[int(t)]; } +int Stmt::num_stmts = 0; + Stmt::Stmt(StmtTag arg_tag) { tag = arg_tag; @@ -69,6 +71,8 @@ Stmt::Stmt(StmtTag arg_tag) opt_info = new StmtOptInfo(); SetLocationInfo(&start_location, &end_location); + + ++num_stmts; } Stmt::~Stmt() diff --git a/src/StmtBase.h b/src/StmtBase.h index 58683e6767..d70cf8b0aa 100644 --- a/src/StmtBase.h +++ b/src/StmtBase.h @@ -177,6 +177,12 @@ public: // this statement. StmtOptInfo* GetOptInfo() const { return opt_info; } + // Returns the number of statements created since the last reset. + static int GetNumStmts() { return num_stmts; } + + // Clears the number of statements created. + static void ResetNumStmts() { num_stmts = 0; } + protected: explicit Stmt(StmtTag arg_tag); @@ -203,6 +209,9 @@ protected: // Information associated with the Stmt for purposes of // script optimization. StmtOptInfo* opt_info; + + // Number of statements created thus far. + static int num_stmts; }; } // namespace detail diff --git a/src/Var.cc b/src/Var.cc index dba0e620fb..35b37ac56b 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -718,6 +718,10 @@ void begin_func(IDPtr id, const char* module_name, FunctionFlavor flavor, bool i if ( Attr* depr_attr = find_attr(current_scope()->Attrs().get(), ATTR_DEPRECATED) ) current_scope()->GetID()->MakeDeprecated(depr_attr->GetExpr()); + + // Reset the AST node statistics to track afresh for this function. + Stmt::ResetNumStmts(); + Expr::ResetNumExprs(); } class OuterIDBindingFinder : public TraversalCallback @@ -804,7 +808,10 @@ void end_func(StmtPtr body, bool free_of_conditionals) // by duplicating can itself be correctly duplicated. body = body->Duplicate()->Duplicate(); - body->GetOptInfo()->is_free_of_conditionals = free_of_conditionals; + auto oi = body->GetOptInfo(); + oi->is_free_of_conditionals = free_of_conditionals; + oi->num_stmts = Stmt::GetNumStmts(); + oi->num_exprs = Expr::GetNumExprs(); auto ingredients = std::make_unique(pop_scope(), std::move(body)); diff --git a/src/script_opt/Expr.cc b/src/script_opt/Expr.cc index b76f4b16bd..47eac78351 100644 --- a/src/script_opt/Expr.cc +++ b/src/script_opt/Expr.cc @@ -2329,6 +2329,10 @@ ExprPtr CallExpr::Inline(Inliner* inl) { auto new_me = inl->CheckForInlining({NewRef{}, this}); + if ( ! new_me ) + // All done with inlining. + return ThisPtr(); + if ( new_me.get() != this ) return new_me; diff --git a/src/script_opt/Inline.cc b/src/script_opt/Inline.cc index f9b3e12fd5..cc2ed52100 100644 --- a/src/script_opt/Inline.cc +++ b/src/script_opt/Inline.cc @@ -5,10 +5,13 @@ #include "zeek/Desc.h" #include "zeek/script_opt/ProfileFunc.h" #include "zeek/script_opt/ScriptOpt.h" +#include "zeek/script_opt/StmtOptInfo.h" namespace zeek::detail { +constexpr int MAX_INLINE_SIZE = 1000; + void Inliner::Analyze() { // Locate self- and indirectly recursive functions. @@ -139,8 +142,15 @@ void Inliner::InlineFunction(FuncInfo* f) // particular body. curr_frame_size = f->Scope()->Length(); + auto oi = f->Body()->GetOptInfo(); + num_stmts = oi->num_stmts; + num_exprs = oi->num_exprs; + f->Body()->Inline(this); + oi->num_stmts = num_stmts; + oi->num_exprs = num_exprs; + int new_frame_size = curr_frame_size + max_inlined_frame_size; if ( new_frame_size > f->Func()->FrameSize() ) @@ -175,9 +185,19 @@ ExprPtr Inliner::CheckForInlining(CallExprPtr c) if ( inline_ables.count(func_vf) == 0 ) return c; - ListExprPtr args = {NewRef{}, c->Args()}; + // We're going to inline the body, unless it's too large. auto body = func_vf->GetBodies()[0].stmts; // there's only 1 body - auto t = c->GetType(); + auto oi = body->GetOptInfo(); + + if ( num_stmts + oi->num_stmts + num_exprs + oi->num_exprs > MAX_INLINE_SIZE ) + return nullptr; + + num_stmts += oi->num_stmts; + num_exprs += oi->num_exprs; + + auto body_dup = body->Duplicate(); + body_dup->GetOptInfo()->num_stmts = oi->num_stmts; + body_dup->GetOptInfo()->num_exprs = oi->num_exprs; // Getting the names of the parameters is tricky. It's tempting // to take them from the function's type declaration, but alas @@ -198,8 +218,6 @@ ExprPtr Inliner::CheckForInlining(CallExprPtr c) for ( int i = 0; i < nparam; ++i ) params.emplace_back(vars[i]); - auto body_dup = body->Duplicate(); - // Recursively inline the body. This is safe to do because we've // ensured there are no recursive loops ... but we have to be // careful in accounting for the frame sizes. @@ -221,6 +239,8 @@ ExprPtr Inliner::CheckForInlining(CallExprPtr c) else max_inlined_frame_size = hold_max_inlined_frame_size; + ListExprPtr args = {NewRef{}, c->Args()}; + auto t = c->GetType(); auto ie = make_intrusive(args, std::move(params), body_dup, curr_frame_size, t); ie->SetOriginal(c); diff --git a/src/script_opt/Inline.h b/src/script_opt/Inline.h index 3b8d24b9c7..738096c919 100644 --- a/src/script_opt/Inline.h +++ b/src/script_opt/Inline.h @@ -27,8 +27,8 @@ public: Analyze(); } - // Either returns the original CallExpr if it's not inline-able, - // or an InlineExpr if it is. + // Either returns the original CallExpr if it's not inline-able; + // or an InlineExpr if it is; or nil if further inlining should stop. ExprPtr CheckForInlining(CallExprPtr c); // True if the given function has been inlined. @@ -57,6 +57,12 @@ protected: // prior to increasing it to accommodate inlining. int curr_frame_size; + // The number of statements and expressions in the function being + // inlined. Dynamically updated as the inlining proceeds. Used + // to cap inlining complexity. + int num_stmts; + int num_exprs; + // Whether to generate a report about functions either directly and // indirectly recursive. bool report_recursive; diff --git a/src/script_opt/StmtOptInfo.h b/src/script_opt/StmtOptInfo.h index 449ecc0e61..cbc9eae4da 100644 --- a/src/script_opt/StmtOptInfo.h +++ b/src/script_opt/StmtOptInfo.h @@ -26,6 +26,10 @@ public: // Whether this statement is free of the possible influence // of conditional code. bool is_free_of_conditionals = true; + + // Number of statements and expressions in a function body. + int num_stmts = 0; + int num_exprs = 0; }; } // namespace zeek::detail diff --git a/testing/btest/Baseline.zam/coverage.coverage-blacklist/output b/testing/btest/Baseline.zam/coverage.coverage-blacklist/output index 863a383031..d54a7d5645 100644 --- a/testing/btest/Baseline.zam/coverage.coverage-blacklist/output +++ b/testing/btest/Baseline.zam/coverage.coverage-blacklist/output @@ -1,6 +1,10 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. 0 <...>/coverage-blacklist.zeek, line 13 print cover me; +0 <...>/coverage-blacklist.zeek, line 15 if (T) print always executed; 0 <...>/coverage-blacklist.zeek, line 17 print always executed; +0 <...>/coverage-blacklist.zeek, line 22 if (F) print impossible; +0 <...>/coverage-blacklist.zeek, line 24 if (F) print also impossible, but included in code coverage analysis; 0 <...>/coverage-blacklist.zeek, line 26 print also impossible, but included in code coverage analysis; 0 <...>/coverage-blacklist.zeek, line 29 print success; 0 <...>/coverage-blacklist.zeek, line 5 print first; +0 <...>/coverage-blacklist.zeek, line 7 if (F) { print hello; print world; } diff --git a/testing/btest/Baseline.zam/language.type-cast-error-dynamic/err b/testing/btest/Baseline.zam/language.type-cast-error-dynamic/err index 36890af23b..16bde5304a 100644 --- a/testing/btest/Baseline.zam/language.type-cast-error-dynamic/err +++ b/testing/btest/Baseline.zam/language.type-cast-error-dynamic/err @@ -1,4 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. runtime error in <...>/type-cast-error-dynamic.zeek, line 12: invalid cast of value with type 'count' to type 'string' runtime error in <...>/type-cast-error-dynamic.zeek, line 12: invalid cast of value with type 'record { a:addr; b:port; }' to type 'string' -runtime error in <...>/type-cast-error-dynamic.zeek, line 12: invalid cast of value with type 'record { data:opaque of Broker::Data &optional; }' to type 'string' (nil $data field) +runtime error in <...>/type-cast-error-dynamic.zeek, line 12: invalid cast of value with type 'Broker::Data' to type 'string' (nil $data field) diff --git a/testing/btest/scripts/base/frameworks/netcontrol/basic-cluster.zeek b/testing/btest/scripts/base/frameworks/netcontrol/basic-cluster.zeek index 55bc9776db..6dd28aa1e8 100644 --- a/testing/btest/scripts/base/frameworks/netcontrol/basic-cluster.zeek +++ b/testing/btest/scripts/base/frameworks/netcontrol/basic-cluster.zeek @@ -9,7 +9,7 @@ # @TEST-EXEC: btest-bg-run worker-2 "cp ../cluster-layout.zeek . && CLUSTER_NODE=worker-2 zeek -b --pseudo-realtime -C -r $TRACES/tls/ecdhe.pcap %INPUT" # This timeout needs to be large to accommodate ZAM compilation delays. -# @TEST-EXEC: btest-bg-wait 90 +# @TEST-EXEC: btest-bg-wait 45 # @TEST-EXEC: btest-diff worker-1/.stdout # @TEST-EXEC: btest-diff worker-2/.stdout diff --git a/testing/btest/scripts/base/frameworks/sumstats/sample-cluster.zeek b/testing/btest/scripts/base/frameworks/sumstats/sample-cluster.zeek index c190df57ea..ec1524e7a5 100644 --- a/testing/btest/scripts/base/frameworks/sumstats/sample-cluster.zeek +++ b/testing/btest/scripts/base/frameworks/sumstats/sample-cluster.zeek @@ -1,7 +1,3 @@ -# the scripts and triggers the timeout. Ultimately we need to address this -# by capping the size of inlined functions, since the main delay comes from -# traversing enormous AST function bodies. -# @TEST-REQUIRES: test "${ZEEK_ZAM}" != "1" # @TEST-PORT: BROKER_PORT1 # @TEST-PORT: BROKER_PORT2 # @TEST-PORT: BROKER_PORT3 @@ -10,7 +6,7 @@ # @TEST-EXEC: btest-bg-run worker-1 ZEEKPATH=$ZEEKPATH:.. CLUSTER_NODE=worker-1 zeek -b %INPUT # @TEST-EXEC: btest-bg-run worker-2 ZEEKPATH=$ZEEKPATH:.. CLUSTER_NODE=worker-2 zeek -b %INPUT # This timeout needs to be large to accommodate ZAM compilation delays. -# @TEST-EXEC: btest-bg-wait 90 +# @TEST-EXEC: btest-bg-wait 45 # @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-sort btest-diff manager-1/.stdout @load base/frameworks/sumstats