[BUG] bugreport memory

[hroest]

We have been worried a bit about the potential of memory leaks in the IsoSpec algorithms, so I did some benchmarking and what I found was that in a tight loop there seems to be leaks of memory. I hope that I am doing this correctly but in my understanding, this should clean up all memory when run in a loop:

      Iso* iso = new Iso(formula.c_str());
      IsoThresholdGenerator* generator = new IsoThresholdGenerator(std::move(*iso), threshold, absolute, tabSize, hashSize); 
      Tabulator<IsoThresholdGenerator>* tabulator = new Tabulator<IsoThresholdGenerator>(generator, true, true, false, true); 
      delete generator;
      delete tabulator;

to reproduce, run

$ make memleak && /usr/bin/time ./memleak-clang-opt 
88.28user 0.18system 1:28.47elapsed 99%CPU (0avgtext+0avgdata 316356maxresident)k
0inputs+0outputs (0major+78337minor)pagefaults 0swaps

as documented, what is worrying is that memory keeps constantly increasing the longer the loop is run which is concerning.

[hroest]

ok, sorry, I may have mis-interpreted ownership in the code, I think the correct code should be

      Iso* iso = new Iso(formula.c_str());
      IsoThresholdGenerator* generator = new IsoThresholdGenerator(std::move(*iso), threshold, absolute, tabSize, hashSize); 
      Tabulator<IsoThresholdGenerator>* tabulator = new Tabulator<IsoThresholdGenerator>(generator, true, true, false, true); 
      delete generator;
      delete tabulator;
      delete iso; // still need to destroy the struct

is that correct? I was under the impression that the std::move will take ownership of iso however upon reflection that is probably not the case as it will only take ownership of the data owned by iso and not of iso itself. Is that correct?

[michalsta]

yes, that's absolutely true. By the way, you can avoid the (explicit) creation of those Iso objects like this:

IsoThresholdGenerator* generator = new IsoThresholdGenerator(formula.c_str(), threshold, absolute, tabSize, hashSize);

The compiler will automatically create an Iso object from the string for the constructor. And also, it will do the right thing with ownership, etc...