-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement compression and splitting option in TRestProcessRunner #201
Conversation
…ard c++ type name(long long)
I splited the EventSelectionProcess pipeline in two parts, with different rml and validation files. |
return fChain ? fChain->Draw(varexp, selection, option, nentries, firstentry) | ||
: TTree::Draw(varexp, selection, option, nentries, firstentry); | ||
} | ||
Long64_t Draw(const char* varexp, const char* selection, Option_t* option = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an override of TTree
functions? What is the reason of these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It is used to read splitted data files. In case multiple files are created to store analysis tree data, we make TRestAnalysisTree
running internally with TChain
.
if (tree->GetEntry(0) > 0) { | ||
if (tree->GetRunOrigin() == this->GetRunOrigin()) { | ||
// this is a valid tree | ||
delete tree; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think is a good practise to delete
an object which has not been created with new
, you can fall in an unexpected behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is what returned by ROOT. We can only trust that TFile::Get() returns the pointer which is deleteable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure you can delete the pointer, however root
has his own memory management, which means that it can try to access de tree
that you just delete, which can end up in a seg fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. In which case will ROOT access such object? Or there is any "safe" method provided by ROOT to delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tree
in fact belongs to the TFile
, when the TFile
is destroyed it migth try to delete
the tree, so you can fall in a double free with an unexpected behaviour.
https://stackoverflow.com/questions/9169774/what-happens-in-a-double-delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a test and there is no such double free fault if I delete the tree manually. There is also no memory leak if I don't delete the tree. So I guess ROOT has some mechanism to prevent object from file being deleted. Either delete or not is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I still need to delete the tree. Otherwise the initializaion of the TChain will be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juanangp Can you approve this PR? I believe those problems are trivial
for (i = fThreadNumber; i > 1; i--) { | ||
// delete (*fThreads.end()); | ||
fThreads.erase(fThreads.end() - 1); | ||
fThreadNumber--; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't quite understand this is this bucle for? Note that you have to delete
the pointer content, if not you have a potential memory leak.
for (i = fThreadNumber; i > 1; i--) { | |
// delete (*fThreads.end()); | |
fThreads.erase(fThreads.end() - 1); | |
fThreadNumber--; | |
} | |
for (auto it = fThread.begin(); it != fThread.end(); ) { | |
if( it !=fThread.begin() ) { | |
delete * it; | |
it = fThread.erase(it); | |
fThreadNumber--; | |
} else { | |
++it; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why but it always causes problems when I delete the TRestThread object. This code block is for a rare case that, when adding processes, one process turns out to be must running under single thread. So we remove other threads. I guess this memory leak is acceptable and so I directly comment the delete operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it was deleted in the previous implementation...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous implementation this delete operation sometimes causes seg. fault which I don't know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I check more thoroughly, looks like you have a double bucle for fThreads
, which can be the origin of the issue.
for (int i = 0; i < fThreadNumber; i++) {
....
for (i = fThreadNumber; i > 1; i--)
I would suggest to use std iterators instead:
for (auto it = fThread.begin(); it != fThread.end(); ) {
Co-authored-by: Juan Antonio García <80903717+juanangp@users.noreply.github.com>
Co-authored-by: Juan Antonio García <80903717+juanangp@users.noreply.github.com>
Related to #190.
This PR implements compression and splitting option in TRestProcessRunner. The details of these feature can be found in the issue. It also introduces other changes:
test job in ci is moved after build and install job. This avoids warnings of PCM not find(solved in another PR)NotifyAnalysisTreeReset()
. If the file is split, this method will be called.restG4 pipeline for branch
nkx111-patch-3
: