Rework TChain::GetEntries to avoid side-effects #19873
Conversation
Test Results 20 files 20 suites 3d 15h 15m 49s ⏱️ For more details on these failures, see this check. Results for commit 0608255. ♻️ This comment has been updated with latest results. |
|
Turns out there are even more side-effects caused by |
Instead of relying on TChain::LoadTree to compute the total number of entries, which has guaranteed side-effects, switch to a simpler accumulation algorithm by opening just the files of the chain. Use the READ_WITHOUT_GLOBAL_REGISTRATION option to avoid possible side-effects in this case as well.
Co-authored-by: ferdymercury <[email protected]>
vepadulano
force-pushed
the
gh-19814-fix
branch
from
59905d9 to
9f3dda8
Compare
|
@pcanal I implemented the changes we discussed in the last commit. Let me know what you think and let's see if the CI will agree with us. |
There was a problem hiding this comment.
LGTM, just a minor comment on the corner case.
Also, does this change with the cache maybe impact https://its.cern.ch/jira/browse/ROOT-7973 ?
| @@ -30,7 +30,7 @@ TChainElement::TChainElement() : TNamed(),fBaddress(nullptr),fBaddressType(0), | |||
| { | |||
| fNPackets = 0; | |||
| fPackets = nullptr; | |||
| fEntries = 0; | |||
| fEntries = TTree::kMaxEntries; | |||
There was a problem hiding this comment.
Should this maybe be kMaxEntries+1 ?
Since otherwise kMaxEntries can not be safely reached?
Fixes #19814 and also the recent CI failures such as https://github.com/root-project/root/actions/runs/17610706130/job/50031825233?pr=19860#step:6:4762
The second commit contains the main change: reworking the
TChain::GetEntriesmethod. The issue with it was that it was causing one side-effect too many: the last opened file was not being properly closed. This in turn triggered the if condition inTTreeReader::Restartand the segfaults due to bad memory accesses. This is an alternative solution to #19866 . The advantage IMHO is that it requires changing fewer places and better conveys the intent of the method: a for loop opening the files as opposed to calling LoadTree until all files have been opened. On top of course of having far less side-effects (AFAICT none).