From 53a1c76818dea6d55c510c79b29fd01b746142cd Mon Sep 17 00:00:00 2001 From: "Kuangyuan.Sun" Date: Thu, 2 Feb 2023 20:04:27 +0000 Subject: [PATCH] a better solution for ISUEE-1433 --- gtsam/inference/BayesTree.h | 24 +++++++++++++----------- gtsam/inference/BayesTreeCliqueBase.h | 1 - gtsam/inference/ClusterTree.h | 22 +++++++++------------- gtsam/inference/EliminationTree.h | 22 +++++++++------------- 4 files changed, 31 insertions(+), 38 deletions(-) diff --git a/gtsam/inference/BayesTree.h b/gtsam/inference/BayesTree.h index 6637370cc..ebca5de25 100644 --- a/gtsam/inference/BayesTree.h +++ b/gtsam/inference/BayesTree.h @@ -28,7 +28,6 @@ #include #include -#include namespace gtsam { @@ -114,21 +113,24 @@ namespace gtsam { 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 bfs_queue; - std::deque topological_order; - bfs_queue.push(root.get()); + std::queue 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()) { - Clique* current = bfs_queue.front(); + auto current = bfs_queue.front(); bfs_queue.pop(); - topological_order.push_front(current); - for (auto&& child: current->children) { - bfs_queue.push(child.get()); + for (auto child: current->children) { + bfs_queue.push(child); } } - for (auto&& clique: topological_order) { - clique->children.clear(); - } } } diff --git a/gtsam/inference/BayesTreeCliqueBase.h b/gtsam/inference/BayesTreeCliqueBase.h index 6e50182c4..976d1a7f2 100644 --- a/gtsam/inference/BayesTreeCliqueBase.h +++ b/gtsam/inference/BayesTreeCliqueBase.h @@ -26,7 +26,6 @@ #include #include - namespace gtsam { // Forward declarations diff --git a/gtsam/inference/ClusterTree.h b/gtsam/inference/ClusterTree.h index 63a1b5a96..c724adf5e 100644 --- a/gtsam/inference/ClusterTree.h +++ b/gtsam/inference/ClusterTree.h @@ -9,7 +9,6 @@ #pragma once -#include #include #include #include @@ -48,8 +47,7 @@ class ClusterTree { Cluster() : problemSize_(0) {} - virtual ~Cluster() { - } + virtual ~Cluster() {} const Cluster& operator[](size_t i) const { return *(children[i]); @@ -169,22 +167,20 @@ class ClusterTree { virtual ~ClusterTree() { // use default destructor which recursively deletes all nodes with shared_ptr causes stack overflow. - // so for each tree, we do a BFS to get sequence of nodes to delete, and clear their children first. + // 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 bfs_queue; - std::deque topological_order; - bfs_queue.push(root.get()); + std::queue bfs_queue; + bfs_queue.push(root); + root = nullptr; while (!bfs_queue.empty()) { auto node = bfs_queue.front(); - topological_order.push_front(node); bfs_queue.pop(); - for (auto&& child : node->children) { - bfs_queue.push(child.get()); + for (auto child : node->children) { + bfs_queue.push(child); } } - for (auto&& node : topological_order) { - node->children.clear(); - } } } diff --git a/gtsam/inference/EliminationTree.h b/gtsam/inference/EliminationTree.h index dcd561fd2..404f4f38f 100644 --- a/gtsam/inference/EliminationTree.h +++ b/gtsam/inference/EliminationTree.h @@ -121,24 +121,21 @@ namespace gtsam { public: ~EliminationTree() { - // default destructor will cause stack overflow for deletion of large trees - // so we delete the tree explicitly by first BFSing the tree to determine the topological order - // and then clearing the children of each node in topological order + // 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 bfs_queue; - std::deque topological_order; - bfs_queue.push(root.get()); + std::queue bfs_queue; + bfs_queue.push(root); + root = nullptr; while (!bfs_queue.empty()) { - Node* node = bfs_queue.front(); + auto node = bfs_queue.front(); bfs_queue.pop(); - topological_order.push_front(node); for (auto&& child : node->children) { - bfs_queue.push(child.get()); + bfs_queue.push(child); } } - for (auto&& node : topological_order) { - node->children.clear(); - } } } /// @name Standard Interface @@ -179,7 +176,6 @@ namespace gtsam { /** Swap the data of this tree with another one, this operation is very fast. */ void swap(This& other); - protected: /// Protected default constructor EliminationTree() {}