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.

Author: yhabteab

lib/icinga/checkable-dependency.cpp

@@ -7,7 +7,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.
@@ -315,7 +315,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);
 
@@ -328,31 +328,23 @@ 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 (seenChildren.find(checkable) == seenChildren.end()) {
+			seenChildren.emplace(checkable);
+			checkable->GetAllChildrenInternal(seenChildren, level + 1);
 		}
 	}
-
-	children.insert(localChildren.begin(), localChildren.end());
 }
 
 /**

lib/icinga/checkable.hpp

@@ -264,7 +264,7 @@ class Checkable : public ObjectImpl<Checkable>
 	std::unordered_set<intrusive_ptr<DependencyGroup>, HashDependencyGroup, EqualDependencyGroups> m_DependencyGroups;
 	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;