I use TChainIndex for some time, but I would like today to fully understand how it is meant to be used. After digging the code, and based on the fact that TChain do not redefine TTree::BuildIndex, I got the feeling that TChainIndex was meant to be explicitly constructed and used :
...
TChainIndex tci(myChain,major,minor) ;
...
Unhapilly when doing this, I usually got a program crahs when the execution goes through TChainIndex destructor. I suspect some sort of double delete of objects shared between my TChainIndex and the associated TChain.
My current ugly fix is to create the TChainIndex with new and never delete it.
Actually, am i wrong to create the TChainIndex myself ? What is the official way to create such an index ?
After some more investigation it seems that TTree::BuildIndex delegates to TTreePlayer, which recognize that the current object is an instance of TChain, and consequently constructs a TChainIndex. The kind of behavior hardly guessable…
I assume doing a call TTree::BuildIndex will avoid my crash.
Then I move to another issue : a memory leak noticed by Tracy Usher when opening/closing many chains. Apparently, TChainIndex contains a collection of TChainIndexEntry by value, but there is no explicity destructor for TChainIndexEntry, and I suspect the enclosed fTreeIndex is never deleted.
Thanks for reporting this memory leak. It has been fixed in revision 26765.
Cheers,
Philippe.
PS. Rather than having TChainIndexEntry delete the index, it is done in TChainIndex::DeleteIndices … which need to make sure to not double delete the index (because it’s ownership is ‘shared’ with the TTree object).
Could this fix be applied to a patch of ROOT v5.20.00? We are in the process of upgrading to this version and it would be helpful to have this fix in place. Otherwise, I could look into applying the modification by hand - if that is deemed feasible.
If I look to the TChainIndex constructor code, I notice that some TTreeIndex are constructed only for trees which do not have already an index. And those indexes made on the fly are detached from the associated trees. So, one should be able to delete them whenever he wants. And it should never happen that those indexes stored in TChainIndexElement are the same as the index of the currently loaded tree.
Looking back to the original bug (savannah.cern.ch/bugs/?22722), which made you comment the delete instructions, I have another hypothesis about the source of the problem :
When one to TTree->SetTreeIndex(0), the TTree does not point any more to the index, BUT THE INDEX STILL POINT TO THE TREE. In the context of a chain, when the chain load another tree, the index keep a dangling pointer.
The TTreeIndex class has been designed to born and die together with its TTree, but in the context of TChainIndex, they must live independantly from the corresponding TTree, which is created and destructed frequently when the chain load and unload them.
A naive solution : within TTree::SetTreeIndex(…) method, there should be a call oldindex->SetTree(0). But I have not checked all TTreeIndex methods so to check if they can work with fTree set to 0.
[quote]Looking back to the original bug (savannah.cern.ch/bugs/?22722), which made you comment the delete instructions, I have another hypothesis about the source of the problem[/quote]In the case of this bug report, the problem really is a double delete of the TTreeIndex: once by the TTree and once by the TChainIndex. So my fix is coherent in the sense that
Before the fix for 22722, TChainIndex was always deleting the TTreeIndex
After the first fix for 22722, TChainIndex was never deleting the TTreeIndex even it explicitly detached it from a TTree (hence nobody was deleting them … ever).
With today fix, TChainIndex deletes the TTreeIndex whenever it explicitly detaches the TTreeIndex from a TTree.
On the other it is very possible that there is still some serious leaks … I am taking another look.
[quote=“pcanal”][quote]Looking back to the original bug (savannah.cern.ch/bugs/?22722), which made you comment the delete instructions, I have another hypothesis about the source of the problem[/quote]In the case of this bug report, the problem really is a double delete of the TTreeIndex: once by the TTree and once by the TChainIndex. So my fix is coherent in the sense that
Before the fix for 22722, TChainIndex was always deleting the TTreeIndex[/quote]
Yes, it was. And the TTreeIndex was detached from the TTree, but always having the TTree address, which explains why the TChainIndex destructor was crashing, because it was deleting TTreeIndexes whose fTree pointer was dangling.
Yes, and this imply the memory leak.
That’s where I disagree.
[quote]void TChainIndex::DeleteIndices()
{
// Delete all the indices which were built by this object
for (unsigned int i = 0; i < fEntries.size(); i++) {
if (fEntries[i].fTreeIndex) {
if (fTree->GetTree() && fTree->GetTree()->GetTreeIndex() == fEntries[i].fTreeIndex) {
fTree->GetTree()->SetTreeIndex(0);
SafeDelete(fEntries[i].fTreeIndex);
} else {
// Since we did not explicitly detach this index from a TTree, it
// still belongs to that TTree object and the TTree will delete the index.
}
}
[/quote]
I think the test "if (fTree->GetTree() && Tree->GetTree()->GetTreeIndex() == fEntries[i].fTreeIndex) " is never true, because fEntries[i].fTreeIndex has been detached from the corresponding tree, long ago, in the constructor of TChainIndex. Worse, it contains a TTree pointer, fTree, which point to a tree which has been deleted long ago also, and if you delete it you crash the program.
.