From 31c29505df5f797e800dde073dafbf8cd9759d0d Mon Sep 17 00:00:00 2001 From: Ryan Curtin Date: Sun, 22 Mar 2020 15:42:25 -0400 Subject: [PATCH] Fix various memory handling issues. Delete things that were allocated, be clear about ownership. --- .../core/tree/hollow_ball_bound_impl.hpp | 3 ++ src/mlpack/methods/ann/brnn.hpp | 6 ++-- src/mlpack/methods/ann/brnn_impl.hpp | 36 ++++++++++++++----- .../hoeffding_trees/hoeffding_tree.hpp | 5 ++- .../hoeffding_trees/hoeffding_tree_impl.hpp | 28 +++++++++++---- .../hoeffding_trees/hoeffding_tree_model.cpp | 6 ++++ .../hoeffding_trees/hoeffding_tree_model.hpp | 12 ------- 7 files changed, 66 insertions(+), 30 deletions(-) diff --git a/src/mlpack/core/tree/hollow_ball_bound_impl.hpp b/src/mlpack/core/tree/hollow_ball_bound_impl.hpp index e6d9737ee6..88e3e082ea 100644 --- a/src/mlpack/core/tree/hollow_ball_bound_impl.hpp +++ b/src/mlpack/core/tree/hollow_ball_bound_impl.hpp @@ -80,6 +80,9 @@ template HollowBallBound& HollowBallBound:: operator=(const HollowBallBound& other) { + if (ownsMetric) + delete metric; + radii = other.radii; center = other.center; hollowCenter = other.hollowCenter; diff --git a/src/mlpack/methods/ann/brnn.hpp b/src/mlpack/methods/ann/brnn.hpp index 57789336a9..7e042de6fd 100644 --- a/src/mlpack/methods/ann/brnn.hpp +++ b/src/mlpack/methods/ann/brnn.hpp @@ -73,10 +73,12 @@ class BRNN BRNN(const size_t rho, const bool single = false, OutputLayerType outputLayer = OutputLayerType(), - MergeLayerType mergeLayer = MergeLayerType(), - MergeOutputType mergeOutput = MergeOutputType(), + MergeLayerType* mergeLayer = new MergeLayerType(), + MergeOutputType* mergeOutput = new MergeOutputType(), InitializationRuleType initializeRule = InitializationRuleType()); + ~BRNN(); + /** * Check if the optimizer has MaxIterations() parameter, if it does * then check if it's value is less than the number of datapoints diff --git a/src/mlpack/methods/ann/brnn_impl.hpp b/src/mlpack/methods/ann/brnn_impl.hpp index 697bad7dfa..69e9020768 100644 --- a/src/mlpack/methods/ann/brnn_impl.hpp +++ b/src/mlpack/methods/ann/brnn_impl.hpp @@ -39,13 +39,13 @@ BRNN +BRNN::~BRNN() +{ + // Remove mergeLayer from the forward and backward RNNs so it doesn't get + // deleted. This assumes that mergeLayer is the last layer! + forwardRNN.network.pop_back(); + backwardRNN.network.pop_back(); + + // Clean up layers that we allocated. + boost::apply_visitor(DeleteVisitor(), mergeLayer); + boost::apply_visitor(DeleteVisitor(), mergeOutput); +} + template @@ -230,7 +246,7 @@ void BRNN( forwardRNN.network.back()), mergeLayer); boost::apply_visitor(AddVisitor( @@ -707,6 +725,8 @@ void BRNN& numericSplitIn = NumericSplitType(0), std::unordered_map>* - dimensionMappings = NULL); + dimensionMappings = NULL, + const bool copyDatasetInfo = true); /** * Construct a Hoeffding tree with no data and no information. Be sure to diff --git a/src/mlpack/methods/hoeffding_trees/hoeffding_tree_impl.hpp b/src/mlpack/methods/hoeffding_trees/hoeffding_tree_impl.hpp index c0f647fe4c..e1769e55b1 100644 --- a/src/mlpack/methods/hoeffding_trees/hoeffding_tree_impl.hpp +++ b/src/mlpack/methods/hoeffding_trees/hoeffding_tree_impl.hpp @@ -96,7 +96,8 @@ HoeffdingTree< categoricalSplitIn, const NumericSplitType& numericSplitIn, std::unordered_map>* - dimensionMappingsIn) : + dimensionMappingsIn, + const bool copyDatasetInfo) : dimensionMappings((dimensionMappingsIn != NULL) ? dimensionMappingsIn : new std::unordered_map>()), ownsMappings(dimensionMappingsIn == NULL), @@ -105,8 +106,9 @@ HoeffdingTree< maxSamples((maxSamples == 0) ? size_t(-1) : maxSamples), checkInterval(checkInterval), minSamples(minSamples), - datasetInfo(new data::DatasetInfo(datasetInfo)), - ownsInfo(true), + datasetInfo(copyDatasetInfo ? new data::DatasetInfo(datasetInfo) : + &datasetInfo), + ownsInfo(copyDatasetInfo), successProbability(successProbability), splitDimension(size_t(-1)), majorityClass(0), @@ -208,7 +210,18 @@ HoeffdingTree:: { // Copy each of the children. for (size_t i = 0; i < other.children.size(); ++i) + { children.push_back(new HoeffdingTree(*other.children[i])); + + // Delete copied datasetInfo and dimension mappings. + delete children[i]->datasetInfo; + children[i]->datasetInfo = this->datasetInfo; + children[i]->ownsInfo = false; + + delete children[i]->dimensionMappings; + children[i]->dimensionMappings = this->dimensionMappings; + children[i]->ownsMappings = false; + } } template(0, numClasses), - numericSplits[0], dimensionMappings)); + numericSplits[0], dimensionMappings, false)); } else if (numericSplits.size() == 0) { @@ -752,14 +765,14 @@ void HoeffdingTree< children.push_back(new HoeffdingTree(*datasetInfo, numClasses, successProbability, maxSamples, checkInterval, minSamples, categoricalSplits[0], NumericSplitType(numClasses), - dimensionMappings)); + dimensionMappings, false)); } else { // Pass both splits that we already have. children.push_back(new HoeffdingTree(*datasetInfo, numClasses, successProbability, maxSamples, checkInterval, minSamples, - categoricalSplits[0], numericSplits[0], dimensionMappings)); + categoricalSplits[0], numericSplits[0], dimensionMappings, false)); } children[i]->MajorityClass() = childMajorities[i]; @@ -874,7 +887,8 @@ void HoeffdingTree< { // The child doesn't actually own its own DatasetInfo. We do. The same // applies for the dimension mappings. - children[i]->ownsInfo = false; + if (children[i]->datasetInfo == datasetInfo) + children[i]->ownsInfo = false; children[i]->ownsMappings = false; } diff --git a/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.cpp b/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.cpp index 5a27cffa3b..55635f50f1 100644 --- a/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.cpp +++ b/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.cpp @@ -135,6 +135,12 @@ void HoeffdingTreeModel::BuildModel( const size_t bins, const size_t observationsBeforeBinning) { + // Clean memory, if needed. + delete giniHoeffdingTree; + delete giniBinaryTree; + delete infoHoeffdingTree; + delete infoBinaryTree; + // Depending on the type, create the tree. switch (type) { diff --git a/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.hpp b/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.hpp index 7ab8754f3b..bdf12e3d6b 100644 --- a/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.hpp +++ b/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.hpp @@ -188,30 +188,18 @@ class HoeffdingTreeModel data::DatasetInfo info; if (type == GINI_HOEFFDING) { - // Create fake tree to load into if needed. - if (Archive::is_loading::value) - giniHoeffdingTree = new GiniHoeffdingTreeType(info, 1, 1); ar & BOOST_SERIALIZATION_NVP(giniHoeffdingTree); } else if (type == GINI_BINARY) { - // Create fake tree to load into if needed. - if (Archive::is_loading::value) - giniBinaryTree = new GiniBinaryTreeType(info, 1, 1); ar & BOOST_SERIALIZATION_NVP(giniBinaryTree); } else if (type == INFO_HOEFFDING) { - // Create fake tree to load into if needed. - if (Archive::is_loading::value) - infoHoeffdingTree = new InfoHoeffdingTreeType(info, 1, 1); ar & BOOST_SERIALIZATION_NVP(infoHoeffdingTree); } else if (type == INFO_BINARY) { - // Create fake tree to load into if needed. - if (Archive::is_loading::value) - infoBinaryTree = new InfoBinaryTreeType(info, 1, 1); ar & BOOST_SERIALIZATION_NVP(infoBinaryTree); } }