memory leak in NeighborSearch #298

Closed
rcurtin opened this Issue Dec 29, 2014 · 3 comments

Projects

None yet

1 participant

@rcurtin
Member
rcurtin commented Dec 29, 2014

Reported by bianjiang on 19 Apr 43883161 15:53 UTC
See attached file...

Looks like NeighborSearch.Search() is leaking memory.

@rcurtin rcurtin self-assigned this Dec 29, 2014
@rcurtin rcurtin added this to the mlpack 1.0.8 milestone Dec 29, 2014
@rcurtin rcurtin closed this Dec 29, 2014
@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by bianjiang on 27 Dec 43883163 12:49 UTC
gcc-4.8; mac os x;

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by bianjiang on 16 Jan 43883223 04:19 UTC
Apparently, if you pass in a referenceTree, rather than just use the constructor, it doesn't leak... The following code if fine.

                AllkNN* allknn = NULL;

        std::vector<size_t> oldFromNewRefs;


        BinarySpaceTree<bound::HRectBound<2>,
            NeighborSearchStat<NearestNeighborSort> >
            refTree(data, oldFromNewRefs, leafSize);

            allknn = new AllkNN(&refTree, data, false);

Trees created from the following constructor not being deleted?

// Construct the object.
template<typename SortPolicy, typename MetricType, typename TreeType>
NeighborSearch<SortPolicy, MetricType, TreeType>::
NeighborSearch(const typename TreeType::Mat& referenceSet,
               const typename TreeType::Mat& querySet,
               const bool naive,
               const bool singleMode,
               const size_t leafSize,
               const MetricType metric) :
    referenceCopy(referenceSet),
    queryCopy(querySet),
    referenceSet(referenceCopy),
    querySet(queryCopy),
    referenceTree(NULL),
    queryTree(NULL),
    treeOwner(true), // False if a tree was passed.
    hasQuerySet(true),
    naive(naive),
    singleMode(!naive && singleMode), // No single mode if naive.
    metric(metric),
    numberOfPrunes(0)
{
  // C++11 will allow us to call out to other constructors so we can avoid this
  // copypasta problem.

  // We'll time tree building, but only if we are building trees.
  Timer::Start("tree_building");

  // Construct as a naive object if we need to.
  referenceTree = new TreeType(referenceCopy, oldFromNewReferences,
      (naive ? referenceCopy.n_cols : leafSize));

  if (!singleMode)
    queryTree = new TreeType(queryCopy, oldFromNewQueries,
        (naive ? querySet.n_cols : leafSize));

  // Stop the timer we started above (if we need to).
  Timer::Stop("tree_building");
}
@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by rcurtin on 30 Nov 43884642 23:29 UTC
Hello there,

Thank you for pointing this out. I believe I have fixed the issue in r16041, although I was unable to run the test you gave (it never terminated; that could be setup-specific though). Anyway, valgrind reports no errors for the AllkNN tests as of r16041, whereas it did before. The issue was that some matrices used to remap points to their original indices were not being deleted.

If the problem isn't fixed, feel free to reopen the bug, and if you have further problems, feel free to open other bugs.

Thanks again!

Ryan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment