[DF] Improve Snapshot branch-address-resetting logic #8375
Merged
eguiraud merged 7 commits intoroot-project:masterfrom Jun 10, 2021
Merged
[DF] Improve Snapshot branch-address-resetting logic #8375eguiraud merged 7 commits intoroot-project:masterfrom
eguiraud merged 7 commits intoroot-project:masterfrom
Conversation
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
Warnings:
|
This was referenced Jun 8, 2021
|
Build failed on mac1014/python3. Errors:
Warnings:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
Warnings:
|
|
Build failed on mac11.0/cxx17. Failing tests: |
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
Warnings:
|
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
|
Build failed on mac1014/python3. Failing tests: |
Axel-Naumann
approved these changes
Jun 9, 2021
|
Build failed on mac11.0/cxx17. Failing tests: |
|
Starting build on |
1 similar comment
|
Starting build on |
pcanal
reviewed
Jun 9, 2021
pcanal
approved these changes
Jun 9, 2021
These are callbacks that are run at the beginning of a new "data block": in the case of a TChain, a new data block starts when the TChain switches TTrees or when a new multi-thread task starts. In other cases a new data block starts with each new task. In the future RDataSources might be able to provide their own definition of a data block. For now, only actions are wired to the data-block callback logic. In the future we could also offer a special kind of Defines that only update their values at the beginning of a data block, which would provide a `DefinePerSample` feature.
Instead of TTree::AddClone + TTree::CopyAddresses, use the data-block callback mechanism to reset addresses when needed. We cannot trust TTree::CopyAddresses to do the right thing because the output branch might be a different kind of branch than the input branch (e.g. TBranch vs TBranchElement), which leads to the problem described at root-project#8295 . Note that TBranch::SetAddress (which we now call directly to keep the address of the output branches synced with the location of the input variables) needs the address of a _pointer that points to the object associated to the branch_ instead of just the address of that object unless the output branch is really a simple TBranch. We reuse fBranchAddresses as storage for these pointers, which until now was only used in case the column was of RVec type. This fixes root-project#8295. As a side-effect of moving away from TTree::AddClone + TTree::CopyAddresses, these changes also fix root-project#7727.
Co-authored-by: Axel Naumann <[email protected]>
Co-authored-by: Axel Naumann <[email protected]>
eguiraud
force-pushed
the
df_fix_snapshot
branch
from
a61be0e to
7ac34a1
Compare
|
Starting build on |
Contributor
Author
|
Rebased just to clean the commit history a bit, the set of changes is exactly the same. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of TTree::AddClone + TTree::CopyAddresses, use the data-block
callback mechanism to reset addresses when needed.
We cannot trust TTree::CopyAddresses to do the right thing because
the output branch might be a different kind of branch than the input
branch (e.g. TBranch vs TBranchElement), which leads to the problem
described at #8295 .
This fixes #8295 for what regards Snapshot.
This PR also adds support for "data-block" callbacks, wired to
TChain'sTNotifymechanism, to be able to react to changes of the underlying TTree (e.g. by resetting the input address of Snapshot's output branches). This is a pre-requisite for the fix, but it also makes it relatively simple to implementDefinePerSampleas requested in #6745 .As a side-effect, since we now don't rely on
TTree::CopyAddressesinSnapshotanymore, this PR should also fix #7727 .