Merge pull request #1441 from wanmeihuali/hotfix/stack_overflow

Fixing stack overflow issue for large tree

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>

gtsam/inference/BayesTree.h

@@ -113,6 +113,9 @@ namespace gtsam {
 
     /// @}
 
+    /** Destructor */
+    ~BayesTree(); 
+
     /** Assignment operator */
     This& operator=(const This& other);
 

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) {

gtsam/inference/ClusterTree.h

@@ -166,6 +166,8 @@ class ClusterTree {
 
   /// @}
 
+  ~ClusterTree();
+
  protected:
   /// @name Details
   /// @{

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> >

gtsam/inference/EliminationTree.h

@@ -118,6 +118,7 @@ namespace gtsam {
     /// @}
 
   public:
+    ~EliminationTree(); 
     /// @name Standard Interface
     /// @{