Merge remote-tracking branch 'origin/topic/vern/zam-memory-reduction'

* origin/topic/vern/zam-memory-reduction:
  Baseline "-a zam" update
  increase BTest wait time to abide ZAM compilation times
  avoid script coverage overhead (especially memory) when using ZAM
  fixes for correctly tracking which functions have been fully inlined
  support for discarding ASTs once compiled via ZAM script optimization
  some code simplifications and streamlining

CHANGES

@@ -1,3 +1,15 @@
+6.1.0-dev.243 | 2023-07-26 14:46:06 -0700
+
+  * increase BTest wait time to abide ZAM compilation times (Vern Paxson, Corelight)
+
+  * avoid script coverage overhead (especially memory) when using ZAM (Vern Paxson, Corelight)
+
+  * fixes for correctly tracking which functions have been fully inlined (Vern Paxson, Corelight)
+
+  * support for discarding ASTs once compiled via ZAM script optimization (Vern Paxson, Corelight)
+
+  * some code simplifications and streamlining (Vern Paxson, Corelight)
+
 6.1.0-dev.236 | 2023-07-25 10:23:47 -0700
 
   * fix for installing identifiers for lambdas into the global scope (Vern Paxson, Corelight)

VERSION

@@ -1 +1 @@
-6.1.0-dev.236
+6.1.0-dev.243

auxil/gen-zam

@@ -1 +1 @@
-Subproject commit 116be85c38f2f5dabd01f42b8b53e38004df41c0
+Subproject commit cbba05dbaa58fdabe863f4e8a122ca92809b52d6

src/Expr.cc

@@ -254,7 +254,7 @@ ExprPtr Expr::MakeLvalue()
 	if ( ! IsError() )
 		ExprError("can't be assigned to");
 
-	return {NewRef{}, this};
+	return ThisPtr();
 	}
 
 bool Expr::InvertSense()
@@ -539,7 +539,7 @@ ExprPtr NameExpr::MakeLvalue()
 	if ( id->IsOption() && ! in_const_init )
 		ExprError("option is not a modifiable lvalue");
 
-	return make_intrusive<RefExpr>(IntrusivePtr{NewRef{}, this});
+	return make_intrusive<RefExpr>(ThisPtr());
 	}
 
 void NameExpr::Assign(Frame* f, ValPtr v)
@@ -2492,7 +2492,7 @@ RefExpr::RefExpr(ExprPtr arg_op) : UnaryExpr(EXPR_REF, std::move(arg_op))
 
 ExprPtr RefExpr::MakeLvalue()
 	{
-	return {NewRef{}, this};
+	return ThisPtr();
 	}
 
 void RefExpr::Assign(Frame* f, ValPtr v)
@@ -2876,7 +2876,7 @@ ExprPtr IndexExpr::MakeLvalue()
 	if ( IsString(op1->GetType()->Tag()) )
 		ExprError("cannot assign to string index expression");
 
-	return make_intrusive<RefExpr>(IntrusivePtr{NewRef{}, this});
+	return make_intrusive<RefExpr>(ThisPtr());
 	}
 
 ValPtr IndexExpr::Eval(Frame* f) const
@@ -3113,7 +3113,7 @@ FieldExpr::~FieldExpr()
 
 ExprPtr FieldExpr::MakeLvalue()
 	{
-	return make_intrusive<RefExpr>(IntrusivePtr{NewRef{}, this});
+	return make_intrusive<RefExpr>(ThisPtr());
 	}
 
 bool FieldExpr::CanDel() const
@@ -5130,7 +5130,7 @@ ExprPtr ListExpr::MakeLvalue()
 		if ( expr->Tag() != EXPR_NAME )
 			ExprError("can only assign to list of identifiers");
 
-	return make_intrusive<RefExpr>(IntrusivePtr{NewRef{}, this});
+	return make_intrusive<RefExpr>(ThisPtr());
 	}
 
 void ListExpr::Assign(Frame* f, ValPtr v)

src/ID.cc

@@ -132,8 +132,8 @@ ID::ID(const char* arg_name, IDScope arg_scope, bool arg_is_export)
 
 ID::~ID()
 	{
+	ClearOptInfo();
 	delete[] name;
-	delete opt_info;
 	}
 
 std::string ID::ModuleName() const
@@ -687,6 +687,12 @@ std::vector<Func*> ID::GetOptionHandlers() const
 	return v;
 	}
 
+void ID::ClearOptInfo()
+	{
+	delete opt_info;
+	opt_info = nullptr;
+	}
+
 	} // namespace detail
 
 	} // namespace zeek

src/ID.h

@@ -151,6 +151,7 @@ public:
 	std::vector<Func*> GetOptionHandlers() const;
 
 	IDOptInfo* GetOptInfo() const { return opt_info; }
+	void ClearOptInfo();
 
 protected:
 	void EvalFunc(ExprPtr ef, ExprPtr ev);

src/ScriptCoverageManager.cc

@@ -11,6 +11,7 @@
 #include "zeek/Desc.h"
 #include "zeek/Reporter.h"
 #include "zeek/Type.h"
+#include "zeek/script_opt/ScriptOpt.h"
 
 using namespace std;
 
@@ -21,7 +22,7 @@ ScriptCoverageManager::ScriptCoverageManager() : ignoring(0), delim('\t') { }
 
 void ScriptCoverageManager::AddStmt(Stmt* s)
 	{
-	if ( ignoring != 0 )
+	if ( ignoring != 0 || analysis_options.gen_ZAM )
 		return;
 
 	stmts.emplace_back(NewRef{}, s);
@@ -29,6 +30,9 @@ void ScriptCoverageManager::AddStmt(Stmt* s)
 
 void ScriptCoverageManager::AddFunction(IDPtr func_id, StmtPtr body)
 	{
+	if ( analysis_options.gen_ZAM )
+		return;
+
 	func_instances.emplace_back(func_id, body);
 	}
 

src/Stmt.cc

@@ -206,6 +206,9 @@ bool Stmt::IsPure() const
 
 void Stmt::Describe(ODesc* d) const
 	{
+	// The following is a handy add-on when doing AST debugging.
+	// d->Add(util::fmt("%p: ", this));
+
 	StmtDescribe(d);
 	}
 

src/StmtBase.h

@@ -165,7 +165,7 @@ public:
 	// into a StmtPtr.
 	virtual StmtPtr SetSucc(Stmt* succ)
 		{
-		succ->SetOriginal({NewRef{}, this});
+		succ->SetOriginal(ThisPtr());
 		return {AdoptRef{}, succ};
 		}
 

src/Var.cc

@@ -845,6 +845,7 @@ void end_func(StmtPtr body, const char* module_name, bool free_of_conditionals)
 
 	id->GetVal()->AsFunc()->AddBody(*ingredients);
 
+	if ( ! analysis_options.gen_ZAM )
 		script_coverage_mgr.AddFunction(id, ingredients->Body());
 
 	auto func_ptr = cast_intrusive<FuncVal>(id->GetVal())->AsFuncPtr();

src/script_opt/Expr.cc

@@ -1770,7 +1770,7 @@ ExprPtr AssignExpr::Reduce(Reducer* c, StmtPtr& red_stmt)
 
 		red_stmt = MergeStmts(rhs_reduce, lhs_stmt, rhs_stmt);
 
-		auto field_name = field_e->FieldName();
+		auto field_name = util::copy_string(field_e->FieldName());
 		auto field = field_e->Field();
 		auto field_assign = make_intrusive<FieldLHSAssignExpr>(lhs_e, rhs_e, field_name, field);
 
@@ -1909,7 +1909,7 @@ ExprPtr IndexExprWhen::Duplicate()
 
 ExprPtr FieldExpr::Duplicate()
 	{
-	return SetSucc(new FieldExpr(op->Duplicate(), field_name));
+	return SetSucc(new FieldExpr(op->Duplicate(), util::copy_string(field_name)));
 	}
 
 ExprPtr HasFieldExpr::Duplicate()

src/script_opt/IDOptInfo.cc

@@ -141,7 +141,7 @@ void IDOptInfo::DefinedAfter(const Stmt* s, const ExprPtr& e,
 	// This needs to come after filling out the confluence
 	// blocks, since they'll create their own (earlier) regions.
 	usage_regions.emplace_back(s, true, stmt_num);
-	usage_regions.back().SetDefExpr(e);
+	usage_regions.back().SetDefExpr(std::move(e));
 
 	if ( tracing )
 		DumpBlocks();

src/script_opt/IDOptInfo.h

@@ -148,6 +148,7 @@ public:
 	// Returns a list of the initialization expressions seen for all
 	// globals, ordered by when they were processed.
 	static auto& GetGlobalInitExprs() { return global_init_exprs; }
+	static void ClearGlobalInitExprs() { global_init_exprs.clear(); }
 
 	// Associated constant expression, if any.  This is only set
 	// for identifiers that are aliases for a constant (i.e., there

src/script_opt/Inline.cc

@@ -218,6 +218,8 @@ ExprPtr Inliner::CheckForInlining(CallExprPtr c)
 		return nullptr; // signals "stop inlining"
 		}
 
+	did_inline.insert(func_vf);
+
 	num_stmts += oi->num_stmts;
 	num_exprs += oi->num_exprs;
 
@@ -265,9 +267,9 @@ 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<InlineExpr>(args, std::move(params), body_dup, curr_frame_size, t);
+	auto ie = make_intrusive<InlineExpr>(c->ArgsPtr(), std::move(params), body_dup, curr_frame_size,
+	                                     t);
 	ie->SetOriginal(c);
 
 	return ie;

src/script_opt/Inline.h

@@ -31,10 +31,10 @@ public:
 	// 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.
+	// True if every instance of the function was inlined.
-	bool WasInlined(const Func* f)
+	bool WasFullyInlined(const Func* f)
 		{
-		return inline_ables.count(f) > 0 && skipped_inlining.count(f) == 0;
+		return did_inline.count(f) > 0 && skipped_inlining.count(f) == 0;
 		}
 
 protected:
@@ -52,6 +52,9 @@ protected:
 	// Functions that we've determined to be suitable for inlining.
 	std::unordered_set<const Func*> inline_ables;
 
+	// Functions that we inlined.
+	std::unordered_set<const Func*> did_inline;
+
 	// Functions that we didn't fully inline, so require separate
 	// compilation.
 	std::unordered_set<const Func*> skipped_inlining;

src/script_opt/ScriptOpt.cc

@@ -31,8 +31,6 @@ void (*CPP_activation_hook)() = nullptr;
 // Tracks all of the loaded functions (including event handlers and hooks).
 static std::vector<FuncInfo> funcs;
 
-static ZAMCompiler* ZAM = nullptr;
-
 static bool generating_CPP = false;
 static std::string CPP_dir; // where to generate C++ code
 
@@ -78,12 +76,10 @@ const FuncInfo* analyze_global_stmts(Stmt* stmts)
 
 	auto sc = current_scope();
 	std::vector<IDPtr> empty_inits;
-	StmtPtr stmts_p{NewRef{}, stmts};
 	global_stmts = make_intrusive<ScriptFunc>(id);
-	global_stmts->AddBody(stmts_p, empty_inits, sc->Length());
+	global_stmts->AddBody(stmts->ThisPtr(), empty_inits, sc->Length());
-
-	funcs.emplace_back(global_stmts, sc, stmts_p, 0);
 
+	funcs.emplace_back(global_stmts, sc, stmts->ThisPtr(), 0);
 	return &funcs.back();
 	}
 
@@ -253,15 +249,15 @@ static void optimize_func(ScriptFunc* f, std::shared_ptr<ProfileFunc> pf, ScopeP
 
 	if ( analysis_options.gen_ZAM_code )
 		{
-		ZAM = new ZAMCompiler(f, pf, scope, new_body, ud, rc);
+		ZAMCompiler ZAM(f, pf, scope, new_body, ud, rc);
 
-		new_body = ZAM->CompileBody();
+		new_body = ZAM.CompileBody();
 
 		if ( reporter->Errors() > 0 )
 			return;
 
 		if ( analysis_options.dump_ZAM )
-			ZAM->Dump();
+			ZAM.Dump();
 
 		f->ReplaceBody(body, new_body);
 		body = new_body;
@@ -491,24 +487,19 @@ static void analyze_scripts_for_ZAM(std::unique_ptr<ProfileFuncs>& pfs)
 	// since it won't be consulted in that case.
 	std::unordered_set<Func*> func_used_indirectly;
 
+	if ( inl )
+		{
 		if ( global_stmts )
 			func_used_indirectly.insert(global_stmts.get());
 
-	if ( inl )
-		{
 		for ( auto& g : pfs->Globals() )
 			{
 			if ( g->GetType()->Tag() != TYPE_FUNC )
 				continue;
 
 			auto v = g->GetVal();
-			if ( ! v )
+			if ( v )
-				continue;
+				func_used_indirectly.insert(v->AsFunc());
-
-			auto func = v->AsFunc();
-
-			if ( inl->WasInlined(func) )
-				func_used_indirectly.insert(func);
 			}
 		}
 
@@ -526,10 +517,15 @@ static void analyze_scripts_for_ZAM(std::unique_ptr<ProfileFuncs>& pfs)
 				continue;
 			}
 
-		else if ( ! analysis_options.compile_all && ! is_lambda && inl && inl->WasInlined(func) &&
+		else if ( ! analysis_options.compile_all && ! is_lambda && inl &&
-		          func_used_indirectly.count(func) == 0 )
+		          inl->WasFullyInlined(func) && func_used_indirectly.count(func) == 0 )
 			{
 			// No need to compile as it won't be called directly.
+			// We'd like to zero out the body to recover the
+			// memory, but a *few* such functions do get called,
+			// such as by the event engine reaching up, or
+			// BiFs looking for them, so we can't safely zero
+			// them.
 			continue;
 			}
 
@@ -549,6 +545,25 @@ static void analyze_scripts_for_ZAM(std::unique_ptr<ProfileFuncs>& pfs)
 	finalize_functions(funcs);
 	}
 
+void clear_script_analysis()
+	{
+	IDOptInfo::ClearGlobalInitExprs();
+
+	// We need to explicitly clear out the optimization information
+	// associated with identifiers.  They have reference loops with
+	// the parent identifier that will prevent reclamation of the
+	// identifiers (and the optimization information) upon Unref'ing
+	// when discarding the scopes and ASTs.
+	for ( auto& f : funcs )
+		for ( auto& id : f.Scope()->OrderedVars() )
+			id->ClearOptInfo();
+
+	funcs.clear();
+	non_recursive_funcs.clear();
+	lambdas.clear();
+	when_lambdas.clear();
+	}
+
 void analyze_scripts(bool no_unused_warnings)
 	{
 	init_options();

src/script_opt/ScriptOpt.h

@@ -196,6 +196,10 @@ extern bool should_analyze(const ScriptFuncPtr& f, const StmtPtr& body);
 // suppressed by the flag) and optimization.
 extern void analyze_scripts(bool no_unused_warnings);
 
+// Called when all script processing is complete and we can discard
+// unused ASTs and associated state.
+extern void clear_script_analysis();
+
 // Called when Zeek is terminating.
 extern void finish_script_execution();
 

src/script_opt/Stmt.cc

@@ -787,10 +787,10 @@ StmtPtr StmtList::DoReduce(Reducer* c)
 bool StmtList::ReduceStmt(int& s_i, std::vector<StmtPtr>& f_stmts, Reducer* c)
 	{
 	bool did_change = false;
-	auto stmt = stmts[s_i];
+	auto& stmt_i = stmts[s_i];
-	auto old_stmt = stmt;
+	auto old_stmt = stmt_i.get();
 
-	stmt = stmt->Reduce(c);
+	auto stmt = stmt_i->Reduce(c);
 
 	if ( stmt != old_stmt )
 		did_change = true;

src/script_opt/ZAM/Compile.h

@@ -60,6 +60,7 @@ class ZAMCompiler
 public:
 	ZAMCompiler(ScriptFunc* f, std::shared_ptr<ProfileFunc> pf, ScopePtr scope, StmtPtr body,
 	            std::shared_ptr<UseDefs> ud, std::shared_ptr<Reducer> rd);
+	~ZAMCompiler();
 
 	StmtPtr CompileBody();
 
@@ -124,8 +125,6 @@ private:
 	const ZAMStmt CompileStmt(const StmtPtr& body) { return CompileStmt(body.get()); }
 	const ZAMStmt CompileStmt(const Stmt* body);
 
-	void SetCurrStmt(const Stmt* stmt) { curr_stmt = stmt; }
-
 	const ZAMStmt CompilePrint(const PrintStmt* ps);
 	const ZAMStmt CompileExpr(const ExprStmt* es);
 	const ZAMStmt CompileIf(const IfStmt* is);

src/script_opt/ZAM/Driver.cc

@@ -29,6 +29,14 @@ ZAMCompiler::ZAMCompiler(ScriptFunc* f, std::shared_ptr<ProfileFunc> _pf, ScopeP
 	Init();
 	}
 
+ZAMCompiler::~ZAMCompiler()
+	{
+	curr_stmt = nullptr;
+
+	for ( auto i : insts1 )
+		delete i;
+	}
+
 void ZAMCompiler::Init()
 	{
 	InitGlobals();

src/script_opt/ZAM/Expr.cc

@@ -1096,7 +1096,7 @@ const ZAMStmt ZAMCompiler::DoCall(const CallExpr* c, const NameExpr* n)
 
 	z.aux->can_change_non_locals = true;
 
-	z.call_expr = c;
+	z.call_expr = {NewRef{}, const_cast<CallExpr*>(c)};
 
 	if ( in_when )
 		z.SetType(n->GetType());

src/script_opt/ZAM/Ops.in

@@ -1581,7 +1581,7 @@ macro WhenCall(func)
 		throw ZAMDelayedCallException();
 	auto& lhs = frame[z.v1];
 	auto trigger = f->GetTrigger();
-	Val* v = trigger ? trigger->Lookup(z.call_expr) : nullptr;
+	Val* v = trigger ? trigger->Lookup(z.call_expr.get()) : nullptr;
 	ValPtr vp;
 	if ( v )
 		vp = {NewRef{}, v};
@@ -1593,7 +1593,7 @@ macro WhenCall(func)
 		std::vector<ValPtr> args;
 		for ( auto i = 0; i < n; ++i )
 			args.push_back(aux->ToVal(frame, i));
-		f->SetCall(z.call_expr);
+		f->SetCall(z.call_expr.get());
 		/* It's possible that this function will call another that
 		 * itself returns null because *it* is the actual blocker.
 		 * That will set ZAM_error, which we need to ignore.

src/script_opt/ZAM/Stmt.cc

@@ -13,7 +13,7 @@ namespace zeek::detail
 
 const ZAMStmt ZAMCompiler::CompileStmt(const Stmt* s)
 	{
-	SetCurrStmt(s);
+	curr_stmt = const_cast<Stmt*>(s)->ThisPtr();
 
 	switch ( s->Tag() )
 		{

src/script_opt/ZAM/Support.cc

@@ -13,7 +13,7 @@
 namespace zeek::detail
 	{
 
-const Stmt* curr_stmt;
+StmtPtr curr_stmt;
 TypePtr log_ID_enum_type;
 TypePtr any_base_type;
 bool ZAM_error = false;

src/script_opt/ZAM/Support.h

@@ -14,7 +14,7 @@ using ValVec = std::vector<ValPtr>;
 
 // The (reduced) statement currently being compiled.  Used for both
 // tracking "use" and "reaching" definitions, and for error messages.
-extern const Stmt* curr_stmt;
+extern StmtPtr curr_stmt;
 
 // True if a function with the given profile can be compiled to ZAM.
 // If not, returns the reason in *reason, if non-nil.

src/script_opt/ZAM/ZInst.h

@@ -130,7 +130,7 @@ public:
 
 	// Interpreter call expression associated with this instruction,
 	// for error reporting and stack backtraces.
-	const CallExpr* call_expr = nullptr;
+	CallExprPtr call_expr = nullptr;
 
 	// Whether v1 represents a frame slot type for which we
 	// explicitly manage the memory.
@@ -306,7 +306,7 @@ public:
 	int num_labels = 0;
 
 	// Used for debugging.  Transformed into the ZInst "loc" field.
-	const Stmt* stmt = curr_stmt;
+	StmtPtr stmt = curr_stmt;
 
 private:
 	// Initialize 'c' from the given ConstExpr.

src/zeek-setup.cc

@@ -1114,6 +1114,8 @@ SetupResult setup(int argc, char** argv, Options* zopts)
 			g_frame_stack.pop_back();
 			}
 
+		clear_script_analysis();
+
 		if ( zeek_script_loaded )
 			{
 			// Queue events reporting loaded scripts.

testing/btest/Baseline.zam/scripts.base.frameworks.logging.rotate-custom-fmt-func-bad/.stderr

@@ -0,0 +1,5 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+1299481205.000000 warning: non-void function returning without a value: Test::my_rotation_format_func
+1299481205.000000 error: Failed to call Log::rotation_format_func for path test continuing with rotation to: ./test-11-03-07_06.00.05
+1299495605.000000 runtime error in <...>/rotate-custom-fmt-func-bad.zeek, line 34: division by zero
+1299495605.000000 error: Failed to call Log::rotation_format_func for path test continuing with rotation to: ./test-11-03-07_10.00.05

testing/btest/Baseline.zam/scripts.base.frameworks.logging.rotate-custom-fmt-func-bad/out

@@ -0,0 +1,11 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+test-1.log
+test-10.log
+test-11-03-07_06.00.05.log
+test-11-03-07_10.00.05.log
+test-2.log
+test-3.log
+test-5.log
+test-6.log
+test-7.log
+test-9.log

testing/btest/broker/remote_event_ts_compat.zeek

@@ -8,7 +8,7 @@
 #
 # @TEST-EXEC: TOPIC=/zeek/my_topic btest-bg-run server "zeek %INPUT >output"
 # Leave room for Zeek to start up, which can be slow when using -O ZAM
-# @TEST-EXEC: sleep 5
+# @TEST-EXEC: sleep 15
 # @TEST-EXEC: TOPIC=/zeek/my_topic btest-bg-run client "python3 ../client.py >output"
 #
 # @TEST-EXEC: btest-bg-wait 45