[math] Replace TRef with array indices in TFoamCell
#12061
Conversation
|
Starting build on |
|
Starting build on |
|
Build failed on mac12/noimt. Failing tests: |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests: |
|
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
|
Build failed on mac11/cxx14. Failing tests: |
|
Starting build on |
guitargeek
changed the title
TFoamTFoamCell
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thank you Jonas for the fix and still maintaining the I/O capabilities of TFoam to keep backward compatibility.
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. |
|
Starting build on |
|
Starting build on |
|
Build failed on mac12/noimt. Errors:
|
|
Starting build on |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Errors:
|
|
Build failed on mac11/cxx14. Errors:
|
|
Could add a few words to the commit log explaining why the use the |
|
Good question, but unfortunately I have to admit I don't really know why. I didn't have that much time to investigate but needed a fix for 6.28 for which I'm already late. The TFoam class is quite fundamental to RooFit, and the memory increase has affected many users. All I had was this hint by valgrind when checked the reproducer in the commit message: This made me think that maybe TRef is the problem, and indeed replacind the TRef with simple indices solved the problem. Is there something that you can make of that valgrind output maybe? |
yes. It is expected, does not indicate a leak and thus is unrelated to your issue :( (Were you using |
|
Oh yes you are right, I keep forgetting to use that file! I don't use valgrind that often Now I was re-running the reproducer with the suppression file, and actually there are still lines in the log related to the Are these expected too? I mean there must be a problem somewhere, otherwise this PR would not fix the problem with the memory increase when instantiating lots of |
No. Those appears (likely) to be within a function that is inlined directly in the script. |
|
After compiling the code as an executable and using massif, I manage to clarify that this is not a memory leak but rather memory use to keep track of the unique ids associated with the TRef: vs |
pcanal
left a comment
pcanal
left a comment
There was a problem hiding this comment.
So indeed, TRef was an overkill to implement those references (since they are solely within the object and the object is not expected to be split). The memory use is due to the fact that the TRef counter is never reset and thus the collection of references keeps growing.
There ought to be a way to write a schema evolution rule to update the index from the TRef (previous attempts may have been missing some 'sources'). However the current solution is adequate (albeit wasting a bit of memory and disk space), so the fix can wait for another PR.
This fixes a memory leak related to the usage of `TRef` in `TFoamCell`
that can be reproduced with the following code:
```C++
class MyTFoamBinding : public TFoamIntegrand {
public:
double Density(Int_t ndim, double *) override { return 1.0; }
};
void reproTFoam()
{
ProcInfo_t procinfo;
const float toMB = 1.f / 1024.f;
std::size_t nToys = 5000;
TRandom3 rand{};
for (std::size_t index_toy = 1; index_toy <= nToys; index_toy++) {
if (index_toy % 500 == 0) {
gSystem->GetProcInfo(&procinfo);
std::cout << index_toy << ": " << procinfo.fMemResident * toMB
<< " " << procinfo.fMemVirtual * toMB << std::endl;
}
MyTFoamBinding binding{};
TFoam tfoam{"TFOAM"};
tfoam.SetkDim(1);
tfoam.SetRho(&binding);
tfoam.SetPseRan(&rand);
tfoam.SetnCells(30);
tfoam.SetnSampl(200);
tfoam.SetPseRan(&rand);
tfoam.SetChat(0);
tfoam.Initialize();
}
}
```
Before this commit, you will see a steady memory increase that is gone
after this commit.
This fixes the fundamental memory increase in RooFit toy studies,
reported for example in root-project#8984.
The docs said explicitly to not use the copy constructor and the copy assignment, so these operators should be deleted, as well as moving.
|
Starting build on |
|
Thank you for your assessment, @pcanal! I have updated the PR a last time to squash some commits, and I'll merge it then. |
4 participants
This fixes a memory leak related to the usage of
TRefinTFoamCellthat can be reproduced with the following code:
Before this commit, you will see a steady memory increase that is gone
after this commit.
This fixes the fundamental memory increase in RooFit toy studies,
reported for example in #8984.