From c465f45200698b3c81d632fa5cfdfc1107507332 Mon Sep 17 00:00:00 2001
From: Yonas Habteab <yonas.habteab@icinga.com>
Date: Fri, 31 Jan 2025 16:30:13 +0100
Subject: [PATCH] Rewrite `Checkable::GetAllChildrenInternal()` method

The previous wasn't per-se wrong, but it was way too inefficient. With
this commit each and every Checkable is going to be visited only once,
and we won't traverse the same Checkable's children multiple times
somewhere in the dependency chain.
---
 lib/icinga/checkable-dependency.cpp | 26 +++++++++-----------------
 lib/icinga/checkable.hpp            |  2 +-
 2 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/lib/icinga/checkable-dependency.cpp b/lib/icinga/checkable-dependency.cpp
index d10d6f3ce..40d66f59f 100644
--- a/lib/icinga/checkable-dependency.cpp
+++ b/lib/icinga/checkable-dependency.cpp
@@ -8,7 +8,7 @@
 using namespace icinga;
 
 /**
- * The maximum number of dependency recursion levels allowed.
+ * The maximum number of allowed dependency recursion levels.
  *
  * This is a subjective limit how deep the dependency tree should be allowed to go, as anything beyond this level
  * is just madness and will likely result in a stack overflow or other undefined behavior.
@@ -197,7 +197,7 @@ size_t Checkable::GetAllChildrenCount() const
 
 std::set<Checkable::Ptr> Checkable::GetAllChildren() const
 {
-	std::set<Checkable::Ptr> children = GetChildren();
+	std::set<Checkable::Ptr> children;
 
 	GetAllChildrenInternal(children, 0);
 
@@ -210,29 +210,21 @@ std::set<Checkable::Ptr> Checkable::GetAllChildren() const
  * Note, this function performs a recursive call chain traversing all the children of the current Checkable
  * up to a certain limit (256). When that limit is reached, it will log a warning message and abort the operation.
  *
- * @param children - The set of children to be filled with all the children of the current Checkable.
+ * @param seenChildren - A container to store all the traversed children into.
  * @param level - The current level of recursion.
  */
-void Checkable::GetAllChildrenInternal(std::set<Checkable::Ptr>& children, int level) const
+void Checkable::GetAllChildrenInternal(std::set<Checkable::Ptr>& seenChildren, int level) const
 {
 	if (level > l_MaxDependencyRecursionLevel) {
 		Log(LogWarning, "Checkable")
 			<< "Too many nested dependencies (>" << l_MaxDependencyRecursionLevel << ") for checkable '" << GetName() << "': aborting traversal.";
-		return ;
+		return;
 	}
 
-	std::set<Checkable::Ptr> localChildren;
-
-	for (const Checkable::Ptr& checkable : children) {
-		if (auto cChildren(checkable->GetChildren()); !cChildren.empty()) {
-			GetAllChildrenInternal(cChildren, level + 1);
-			localChildren.insert(cChildren.begin(), cChildren.end());
-		}
-
-		if (level != 0) { // Recursion level 0 is the initiator, so checkable is already in the set.
-			localChildren.insert(checkable);
+	for (const Checkable::Ptr& checkable : GetChildren()) {
+		if (auto [_, inserted] = seenChildren.insert(checkable); inserted) {
+			seenChildren.emplace(checkable);
+			checkable->GetAllChildrenInternal(seenChildren, level + 1);
 		}
 	}
-
-	children.insert(localChildren.begin(), localChildren.end());
 }
diff --git a/lib/icinga/checkable.hpp b/lib/icinga/checkable.hpp
index c6413fa1b..12e620ed5 100644
--- a/lib/icinga/checkable.hpp
+++ b/lib/icinga/checkable.hpp
@@ -249,7 +249,7 @@ private:
 	std::set<intrusive_ptr<Dependency> > m_Dependencies;
 	std::set<intrusive_ptr<Dependency> > m_ReverseDependencies;
 
-	void GetAllChildrenInternal(std::set<Checkable::Ptr>& children, int level = 0) const;
+	void GetAllChildrenInternal(std::set<Checkable::Ptr>& seenChildren, int level = 0) const;
 
 	/* Flapping */
 	static const std::map<String, int> m_FlappingStateFilterMap;