We have been experienced some memory leaks when fitting in our script multiple times. Indeed, we are simply doing a “fitTo” in a for loop, and memory usage increase after each iterations. It’s problematic, because the script get killed after some times because it used too much memory.
The memory usage remains stable if we comment the fitTo call. In order to find where the leak come from, we have replace the “fitTo” call by a call to RooMinimizer, by copying the code from the fitTo function. We have observed that a call to RooMinimizer::optimizeConst(1) trigger the memory leak. For example, try :
for (int i = 0; i < 500; i++) {
RooAbsReal* nll = simPdf.createNLL(*RedData, Optimize(0));
RooMinimizer m(*nll);
m.setMinimizerType("Minuit");
m.optimizeConst(1);
delete nll;
}
You’ll see that memory usage increases a lot. However, setting “optimizeConst” to zero stop the leak.
That’d be great if we can backport that to fitTo(), but, unfortunately, using “RooFit::Optimize(0)” does not seems to be working (the output is full of RooAbsOptTestStatistic::ctor(electron) optimizing internal clone of p.d.f for likelihood evaluation).
Using “RooTrace::active(1)” shows unfreed objects after the call to “optimizeConst(1)” or “fitTo()”.
You’ll find a sample showing the memory leak attached. Don’t pay too much attention at the pdf declaration. The script needs to be compiled before running. Tested with RooFit 3.50.
Any help fixing this issue would be greatly appreciated!
I am sorry, I misunderstood 3.50 with 5.32.
Yes using RooFit 3.51 (ROOT trunk) might contain some further fixes. If it is still leaking, please submit a bug in Savannah.
If you are always using the dame pdf, a workaround, which is much faster, could be what we are doing in RooStats when evaluating the ProfileLikelihoodTestStatistics. Isstead of creating the nil all the time, which takes a lot of time, we re–use the NLL object and set only the data using the setData() method.
See roofit/roostats/ProfileLikelihoodTestStatistics.cxx
In this case I have not observed a memory leak,
Please let me know if you still see a leak with the current SVN trunk.
Concerning the setData() solution - I strongly recommend this in any case
as it is also much faster for cases where the fit is comparatively fast
w.r.t the overhead. Here is a code example for you
for (int i = 0; i < 500; i++) {
// make new dataset
nll.setData(newDataSet) ;
// use likelihood
}
delete nll;
Note that the same level of optimization is automatically applied again when you introduce a new dataset.
As of ROOT 5.32 it is also explicitly OK if you delete the currently used dataset before you set the new
one (as long as you don’t use nll in the mean time) - this is procedurally often simpler.
Unfortunately, there’s still a leak in the current svn trunk. I’ve also noted a leak in previous root versions, but a smaller one (not really annoying). with root 5.32, the memory jumps by around 100 mb each iteration.
For the latter proposal, I’m afraid it won’t work in our case. The dataset is the same each fit, we only fix a parameter and change its value for each iteration. I think the NLL needs to be recreated each time.
Anyway, set “optimizeConst” to 1 should not cause any leak, especially if nothing is done after!
Here’s the thing I observe (from ‘htop’ results):
for (int i = 0; i < 1000; i++) {
RooAbsReal* nll = simPdf.createNLL(*RedData, RooFit::CloneData(kFALSE));
RooMinimizer m(*nll);
m.setMinimizerType("Minuit");
m.optimizeConst(0);
delete nll;
}
Memory usage: 0.4%
for (int i = 0; i < 1000; i++) {
RooAbsReal* nll = simPdf.createNLL(*RedData, RooFit::CloneData(kFALSE));
RooMinimizer m(*nll);
m.setMinimizerType("Minuit");
m.optimizeConst(1);
delete nll;
}
(notice the change in optimizeConst)
killed after iteration 263 (RAM usage greater than 2 Gb)
Memory usage: > 12.5%
I’ll look into the memory leak issue. It didn’t show up in the standard regression tests so
I am not sure if I can reproduce it. I’ll give it another try today and let you know.
Meanwhile Would it be easy for your to try the SVN trunk version of ROOT? Various things have
already been fixed there w.r.t. 5.32
On the other hand, if your only actions is to fix a parameter and/or change a fixed parameter,
there is no reason to recreate the NLL. Such modifications are automatically detected and
processed (e.g. recalculation of constant terms - redefinition of constant terms) if they occur,
so you should be able to do everything with a single NLL instance.
I’ll look into the memory leak issue. It didn’t show up in the standard regression tests so
I am not sure if I can reproduce it. I’ll give it another try today and let you know.
Meanwhile Would it be easy for your to try the SVN trunk version of ROOT? Various things have
already been fixed there w.r.t. 5.32
[/quote]
Results above was obtained with the SVN trunk, I should have made it more clear, sorry. You can try with the script I posted, you should see the leak immediately. It has everything included inside the zip archive.
[quote=“Wouter Verkerke”]
On the other hand, if your only actions is to fix a parameter and/or change a fixed parameter,
there is no reason to recreate the NLL. Such modifications are automatically detected and
processed (e.g. recalculation of constant terms - redefinition of constant terms) if they occur,
so you should be able to do everything with a single NLL instance.
Wouter[/quote]
That’s a good news, it will probably make the whole analysis a bit faster
I found the problem. It turns out that TList::Delete() no longer deletes my component datasets since I overloaded the operator new() for RooDataSet as TList only deletes objects ‘on the heap’ and it doesn’t see these as on the heap anymore (apparently).
I now delete the component RooDataSets for a simultaneous pdf by hand inside RooAbsTestStatistic and the leak is gone. The code will go into trunk and 532-patches today.
