From 36e9d4aaed05619c780bb2c575f23d16823f397d Mon Sep 17 00:00:00 2001
From: wanmeihuali <ksun6@hotmail.com>
Date: Tue, 7 Feb 2023 17:51:08 -0800
Subject: [PATCH] move the destructor implementation from .h to -inst.h

---
 gtsam/inference/BayesTree-inst.h       | 40 ++++++++++++++++++++++++++
 gtsam/inference/BayesTree.h            | 26 ++---------------
 gtsam/inference/ClusterTree-inst.h     | 36 +++++++++++++++++++++++
 gtsam/inference/ClusterTree.h          | 22 ++------------
 gtsam/inference/EliminationTree-inst.h | 38 ++++++++++++++++++++++++
 gtsam/inference/EliminationTree.h      | 20 +------------
 6 files changed, 120 insertions(+), 62 deletions(-)

diff --git a/gtsam/inference/BayesTree-inst.h b/gtsam/inference/BayesTree-inst.h
index 2d8f917dc..95d475a02 100644
--- a/gtsam/inference/BayesTree-inst.h
+++ b/gtsam/inference/BayesTree-inst.h
@@ -26,6 +26,7 @@
 #include <gtsam/base/timing.h>
 
 #include <fstream>
+#include <queue>
 
 namespace gtsam {
 
@@ -178,6 +179,45 @@ namespace gtsam {
     *this = other;
   }
 
+  /* ************************************************************************* */
+
+  /** Destructor 
+   * Using default destructor causes stack overflow for large trees due to recursive destruction of nodes;
+   * so we manually decrease the reference count of each node in the tree through a BFS, and the nodes with
+   * reference count 0 will be deleted. Please see [PR-1441](https://github.com/borglab/gtsam/pull/1441) for more details.
+   */
+  template<class CLIQUE>
+  BayesTree<CLIQUE>::~BayesTree() {
+    /* Because tree nodes are hold by both root_ and nodes_, we need to clear nodes_ manually first and
+     * reduce the reference count of each node by 1. Otherwise, the nodes will not be properly deleted
+     * during the BFS process.
+     */
+    nodes_.clear();
+    for (auto&& root: roots_) {
+      std::queue<sharedClique> bfs_queue;
+      
+      // first, move the root to the queue
+      bfs_queue.push(root);
+      root = nullptr; // now the root node is owned by the queue
+
+      // do a BFS on the tree, for each node, add its children to the queue, and then delete it from the queue
+      // So if the reference count of the node is 1, it will be deleted, and because its children are in the queue,
+      // the deletion of the node will not trigger a recursive deletion of the children.
+      while (!bfs_queue.empty()) {
+        // move the ownership of the front node from the queue to the current variable
+        auto current = bfs_queue.front();
+        bfs_queue.pop();
+
+        // add the children of the current node to the queue, so that the queue will also own the children nodes.
+        for (auto child: current->children) {
+          bfs_queue.push(child);
+        } // leaving the scope of current will decrease the reference count of the current node by 1, and if the reference count is 0,
+          // the node will be deleted. Because the children are in the queue, the deletion of the node will not trigger a recursive
+          // deletion of the children.
+      }
+    }
+  }
+
   /* ************************************************************************* */
   namespace {
     template<typename NODE>
diff --git a/gtsam/inference/BayesTree.h b/gtsam/inference/BayesTree.h
index ebca5de25..d0ebd916e 100644
--- a/gtsam/inference/BayesTree.h
+++ b/gtsam/inference/BayesTree.h
@@ -27,7 +27,6 @@
 #include <gtsam/base/FastVector.h>
 
 #include <string>
-#include <queue>
 
 namespace gtsam {
 
@@ -112,30 +111,11 @@ namespace gtsam {
     /** Copy constructor */
     BayesTree(const This& other);
 
-    ~BayesTree() {
-      nodes_.clear();  // clear the map manually, as we cannot control the order of destruction in the map
-      for (auto&& root: roots_) {
-        std::queue<sharedClique> bfs_queue;
-        
-        // first, move the root to the queue
-        bfs_queue.push(root);
-        root = nullptr;
-
-        // do a BFS on the tree, for each node, add its children to the queue, and then delete it from the queue
-        // So if the reference count of the node is 1, it will be deleted, and because its children are in the queue,
-        // the deletion of the node will not trigger a recursive deletion of the children.
-        while (!bfs_queue.empty()) {
-          auto current = bfs_queue.front();
-          bfs_queue.pop();
-          for (auto child: current->children) {
-            bfs_queue.push(child);
-          }
-        }
-      }
-    }
-
     /// @}
 
+    /** Destructor */
+    ~BayesTree(); 
+
     /** Assignment operator */
     This& operator=(const This& other);
 
diff --git a/gtsam/inference/ClusterTree-inst.h b/gtsam/inference/ClusterTree-inst.h
index a6175f76b..318624608 100644
--- a/gtsam/inference/ClusterTree-inst.h
+++ b/gtsam/inference/ClusterTree-inst.h
@@ -18,6 +18,7 @@
 #ifdef GTSAM_USE_TBB
 #include <mutex>
 #endif
+#include <queue>
 
 namespace gtsam {
 
@@ -99,6 +100,41 @@ void ClusterTree<GRAPH>::print(const std::string& s, const KeyFormatter& keyForm
   treeTraversal::PrintForest(*this, s, keyFormatter);
 }
 
+/* ************************************************************************* */
+
+/* Destructor.
+ * Using default destructor causes stack overflow for large trees due to recursive destruction of nodes;
+ * so we manually decrease the reference count of each node in the tree through a BFS, and the nodes with
+ * reference count 0 will be deleted. Please see [PR-1441](https://github.com/borglab/gtsam/pull/1441) for more details.
+ */
+template <class GRAPH>
+ClusterTree<GRAPH>::~ClusterTree() {
+  // For each tree, we first move the root into a queue; then we do a BFS on the tree with the queue;
+  
+  for (auto&& root : roots_) {
+    std::queue<sharedNode> bfs_queue;
+    // first, move the root to the queue
+    bfs_queue.push(root);
+    root = nullptr; // now the root node is owned by the queue
+
+    // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
+    // so that the recursive deletion will not happen.
+    while (!bfs_queue.empty()) {
+      // move the ownership of the front node from the queue to the current variable
+      auto node = bfs_queue.front();
+      bfs_queue.pop();
+
+      // add the children of the current node to the queue, so that the queue will also own the children nodes.
+      for (auto child : node->children) {
+        bfs_queue.push(child);
+      } // leaving the scope of current will decrease the reference count of the current node by 1, and if the reference count is 0,
+        // the node will be deleted. Because the children are in the queue, the deletion of the node will not trigger a recursive
+        // deletion of the children.
+    }
+  }
+
+}
+
 /* ************************************************************************* */
 template <class GRAPH>
 ClusterTree<GRAPH>& ClusterTree<GRAPH>::operator=(const This& other) {
diff --git a/gtsam/inference/ClusterTree.h b/gtsam/inference/ClusterTree.h
index c724adf5e..db80cee3b 100644
--- a/gtsam/inference/ClusterTree.h
+++ b/gtsam/inference/ClusterTree.h
@@ -9,7 +9,6 @@
 
 #pragma once
 
-#include <queue>
 #include <gtsam/base/Testable.h>
 #include <gtsam/base/FastVector.h>
 #include <gtsam/inference/Ordering.h>
@@ -165,27 +164,10 @@ class ClusterTree {
     return *(roots_[i]);
   }
 
-  virtual ~ClusterTree() {
-    // use default destructor which recursively deletes all nodes with shared_ptr causes stack overflow.
-    // so for each tree, we first move the root into a queue; then we do a BFS on the tree with the queue;
-    // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
-    // so that the recursive deletion will not happen.
-    for (auto&& root : roots_) {
-      std::queue<sharedNode> bfs_queue;
-      bfs_queue.push(root);
-      root = nullptr;
-      while (!bfs_queue.empty()) {
-        auto node = bfs_queue.front();
-        bfs_queue.pop();
-        for (auto child : node->children) {
-          bfs_queue.push(child);
-        }
-      }
-    }
-  }
-
   /// @}
 
+  ~ClusterTree();
+
  protected:
   /// @name Details
   /// @{
diff --git a/gtsam/inference/EliminationTree-inst.h b/gtsam/inference/EliminationTree-inst.h
index 63cbe222b..e9acc12e1 100644
--- a/gtsam/inference/EliminationTree-inst.h
+++ b/gtsam/inference/EliminationTree-inst.h
@@ -18,6 +18,7 @@
 #pragma once
 
 #include <stack>
+#include <queue>
 
 #include <gtsam/base/timing.h>
 #include <gtsam/base/treeTraversal-inst.h>
@@ -180,6 +181,43 @@ namespace gtsam {
     return *this;
   }
 
+  /* ************************************************************************* */
+  
+  /** Destructor
+   * Using default destructor causes stack overflow for large trees due to recursive destruction of nodes;
+   * so we manually decrease the reference count of each node in the tree through a BFS, and the nodes with
+   * reference count 0 will be deleted. Please see [PR-1441](https://github.com/borglab/gtsam/pull/1441) for more details.
+   */
+  template<class BAYESNET, class GRAPH>
+  EliminationTree<BAYESNET,GRAPH>::~EliminationTree()
+  {
+    // For each tree, we first move the root into a queue; then we do a BFS on the tree with the queue;
+
+    for (auto&& root : roots_) {
+      std::queue<sharedNode> bfs_queue;
+
+      // first, move the root to the queue
+      bfs_queue.push(root);
+      root = nullptr; // now the root node is owned by the queue
+
+      // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
+      // so that the recursive deletion will not happen.
+      while (!bfs_queue.empty()) {
+        // move the ownership of the front node from the queue to the current variable
+        auto node = bfs_queue.front();
+        bfs_queue.pop();
+
+        // add the children of the current node to the queue, so that the queue will also own the children nodes.
+        for (auto&& child : node->children) {
+          bfs_queue.push(child);
+        } // leaving the scope of current will decrease the reference count of the current node by 1, and if the reference count is 0,
+          // the node will be deleted. Because the children are in the queue, the deletion of the node will not trigger a recursive
+          // deletion of the children.
+      }
+    }
+  }
+
+
   /* ************************************************************************* */
   template<class BAYESNET, class GRAPH>
   std::pair<std::shared_ptr<BAYESNET>, std::shared_ptr<GRAPH> >
diff --git a/gtsam/inference/EliminationTree.h b/gtsam/inference/EliminationTree.h
index fd3eb16f5..724c68608 100644
--- a/gtsam/inference/EliminationTree.h
+++ b/gtsam/inference/EliminationTree.h
@@ -19,7 +19,6 @@
 
 #include <utility>
 #include <memory>
-#include <queue>
 
 #include <gtsam/base/Testable.h>
 #include <gtsam/base/FastVector.h>
@@ -119,24 +118,7 @@ namespace gtsam {
     /// @}
 
   public:
-    ~EliminationTree() {
-      // use default destructor which recursively deletes all nodes with shared_ptr causes stack overflow.
-      // so for each tree, we first move the root into a queue; then we do a BFS on the tree with the queue;
-      // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
-      // so that the recursive deletion will not happen.
-      for (auto&& root : roots_) {
-        std::queue<sharedNode> bfs_queue;
-        bfs_queue.push(root);
-        root = nullptr;
-        while (!bfs_queue.empty()) {
-          auto node = bfs_queue.front();
-          bfs_queue.pop();
-          for (auto&& child : node->children) {
-            bfs_queue.push(child);
-          }
-        }
-      }
-    }
+    ~EliminationTree(); 
     /// @name Standard Interface
     /// @{