to avoid recursion, track all aggregates, not just records

isolate the internal methods

src/Val.cc

@@ -1322,12 +1322,12 @@ ValPtr ListVal::DoClone(CloneState* state)
 	return lv;
 	}
 
-unsigned int ListVal::Footprint(std::set<const RecordVal*>* analyzed_records) const
+unsigned int ListVal::ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const
 	{
 	unsigned int fp = vals.size();
 
 	for ( const auto& val : vals )
-		fp += val->Footprint(analyzed_records);
+		fp += val->Footprint(analyzed_vals);
 
 	return fp;
 	}
@@ -2683,7 +2683,7 @@ ValPtr TableVal::DoClone(CloneState* state)
 	return tv;
 	}
 
-unsigned int TableVal::Footprint(std::set<const RecordVal*>* analyzed_records) const
+unsigned int TableVal::ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const
 	{
 	unsigned int fp = table_val->Length();
 
@@ -2693,9 +2693,9 @@ unsigned int TableVal::Footprint(std::set<const RecordVal*>* analyzed_records) c
 		auto vl = table_hash->RecoverVals(*k);
 		auto v = iter.GetValue<TableEntryVal*>()->GetVal();
 
-		fp += vl->Footprint(analyzed_records);
+		fp += vl->Footprint(analyzed_vals);
 		if ( v )
-			fp += v->Footprint(analyzed_records);
+			fp += v->Footprint(analyzed_vals);
 		}
 
 	return fp;
@@ -3066,14 +3066,8 @@ ValPtr RecordVal::DoClone(CloneState* state)
 	return rv;
 	}
 
-unsigned int RecordVal::Footprint(std::set<const RecordVal*>* analyzed_records) const
+unsigned int RecordVal::ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const
 	{
-	if ( analyzed_records->count(this) > 0 )
-		// Footprint is 1 for the RecordVal itself.
-		return 1;
-
-	analyzed_records->insert(this);
-
 	int n = NumFields();
 	unsigned int fp = n;
 
@@ -3084,11 +3078,9 @@ unsigned int RecordVal::Footprint(std::set<const RecordVal*>* analyzed_records)
 
 		auto f_i = GetField(i);
 		if ( f_i )
-			fp += f_i->Footprint(analyzed_records);
+			fp += f_i->Footprint(analyzed_vals);
 		}
 
-	analyzed_records->erase(this);
-
 	return fp;
 	}
 
@@ -3615,7 +3607,7 @@ bool VectorVal::Concretize(const TypePtr& t)
 	return true;
 	}
 
-unsigned int VectorVal::Footprint(std::set<const RecordVal*>* analyzed_records) const
+unsigned int VectorVal::ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const
 	{
 	auto n = vector_val->size();
 	unsigned int fp = n;
@@ -3624,7 +3616,7 @@ unsigned int VectorVal::Footprint(std::set<const RecordVal*>* analyzed_records)
 		{
 		auto v = At(i);
 		if ( v )
-			fp += v->Footprint(analyzed_records);
+			fp += v->Footprint(analyzed_vals);
 		}
 
 	return fp;
@@ -4011,6 +4003,31 @@ ValPtr Val::MakeCount(bro_uint_t u)
 	return make_intrusive<CountVal>(u);
 	}
 
+unsigned int Val::Footprint(std::unordered_set<const Val*>* analyzed_vals) const
+	{
+	auto is_aggr = IsAggr(type);
+
+	// We only need to check containers for possible recursion, as there's
+	// no way to construct a cycle using only non-aggregates.
+	if ( is_aggr )
+		{
+		if ( analyzed_vals->count(this) > 0 )
+			// Footprint is 1 for generating a cycle.
+			return 1;
+
+		analyzed_vals->insert(this);
+		}
+
+	auto fp = ComputeFootprint(analyzed_vals);
+
+	if ( is_aggr )
+		// Allow the aggregate to be revisited providing it's not
+		// in the context of a cycle.
+		analyzed_vals->erase(this);
+
+	return fp;
+	}
+
 ValManager::ValManager()
 	{
 	empty_string = make_intrusive<StringVal>("");

src/Val.h

@@ -131,21 +131,10 @@ public:
 	 */
 	unsigned int Footprint() const
 		{
-		std::set<const RecordVal*> analyzed_records;
+		std::unordered_set<const Val*> analyzed_vals;
-		return Footprint(&analyzed_records);
+		return Footprint(&analyzed_vals);
 		}
 
-	/**
-	 * Internal function for computing a Val's "footprint".
-	 *
-	 * @param analyzed_records  A pointer to a set used to track which
-	 * records have been analyzed to date, used to prevent infinite
-	 * recursion.  The set should be empty (but not nil) on the first call.
-	 *
-	 * @return  The total footprint.
-	 */
-	virtual unsigned int Footprint(std::set<const RecordVal*>* analyzed_records) const { return 1; }
-
 	// Bytes in total value object.
 	[[deprecated("Remove in v5.1. MemoryAllocation() is deprecated and will be removed. See "
 	             "GHI-572.")]] virtual unsigned int
@@ -255,6 +244,7 @@ protected:
 	friend class EnumType;
 	friend class ListVal;
 	friend class RecordVal;
+	friend class TableVal;
 	friend class VectorVal;
 	friend class ValManager;
 	friend class TableEntryVal;
@@ -268,6 +258,21 @@ protected:
 
 	explicit Val(TypePtr t) noexcept : type(std::move(t)) { }
 
+	/**
+	 * Internal function for computing a Val's "footprint".
+	 *
+	 * @param analyzed_vals  A pointer to a set used to track which values
+	 * have been analyzed to date, used to prevent infinite recursion.
+	 * The set should be empty (but not nil) on the first call.
+	 *
+	 * @return  The total footprint.
+	 */
+	unsigned int Footprint(std::unordered_set<const Val*>* analyzed_vals) const;
+	virtual unsigned int ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const
+		{
+		return 1;
+		}
+
 	// For internal use by the Val::Clone() methods.
 	struct CloneState
 		{
@@ -691,13 +696,13 @@ public:
 
 	void Describe(ODesc* d) const override;
 
-	unsigned int Footprint(std::set<const RecordVal*>* analyzed_records) const override;
-
 	[[deprecated("Remove in v5.1. MemoryAllocation() is deprecated and will be removed. See "
 	             "GHI-572.")]] unsigned int
 	MemoryAllocation() const override;
 
 protected:
+	unsigned int ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const override;
+
 	ValPtr DoClone(CloneState* state) override;
 
 	std::vector<ValPtr> vals;
@@ -968,8 +973,6 @@ public:
 	// the function in the frame allowing it to capture its closure.
 	void InitDefaultFunc(detail::Frame* f);
 
-	unsigned int Footprint(std::set<const RecordVal*>* analyzed_records) const override;
-
 	[[deprecated("Remove in v5.1. MemoryAllocation() is deprecated and will be removed. See "
 	             "GHI-572.")]] unsigned int
 	MemoryAllocation() const override;
@@ -1060,6 +1063,8 @@ protected:
 	// Sends data on to backing Broker Store
 	void SendToStore(const Val* index, const TableEntryVal* new_entry_val, OnChangeType tpe);
 
+	unsigned int ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const override;
+
 	ValPtr DoClone(CloneState* state) override;
 
 	TableTypePtr table_type;
@@ -1396,8 +1401,6 @@ public:
 		}
 	RecordValPtr CoerceTo(RecordTypePtr other, bool allow_orphaning = false);
 
-	unsigned int Footprint(std::set<const RecordVal*>* analyzed_records) const override;
-
 	[[deprecated("Remove in v5.1. MemoryAllocation() is deprecated and will be removed. See "
 	             "GHI-572.")]] unsigned int
 	MemoryAllocation() const override;
@@ -1470,6 +1473,8 @@ private:
 	// Just for template inferencing.
 	RecordVal* Get() { return this; }
 
+	unsigned int ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const override;
+
 	// Keep this handy for quick access during low-level operations.
 	RecordTypePtr rt;
 
@@ -1655,8 +1660,6 @@ public:
 	const auto& RawYieldType() const { return yield_type; }
 	const auto& RawYieldTypes() const { return yield_types; }
 
-	unsigned int Footprint(std::set<const RecordVal*>* analyzed_records) const override;
-
 protected:
 	/**
 	 * Returns the element at a given index or nullptr if it does not exist.
@@ -1670,6 +1673,9 @@ protected:
 	ValPtr At(unsigned int index) const;
 
 	void ValDescribe(ODesc* d) const override;
+
+	unsigned int ComputeFootprint(std::unordered_set<const Val*>* analyzed_vals) const override;
+
 	ValPtr DoClone(CloneState* state) override;
 
 private: