[Root7] A path to reducing global memory management. #18083
Draft
+302
−121
Conversation
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
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
89bd358 to
4d29c60
Compare
pcanal
reviewed
Mar 20, 2025
hist/hist/src/TH1.cxx
Outdated
| Bool_t TH1::AddDirectoryStatus() | ||
| { | ||
| return fgAddDirectory; | ||
| return !ROOT::ROOT7MemoryManagement() && fgAddDirectory; |
Member
There was a problem hiding this comment.
Note that this flag is primarily about file constructions (and secondarily about shared ownership). Beside the (intentional) memory leak it introduces in existing scripts, it leads most of them to not longer write the histograms to the file (because they are no longer attached to the File). It is not clear whether this kind of approach is worth it compared to a more user active change (ie. Switching explicitly from TFile to RFile).
Member
Author
There was a problem hiding this comment.
Thanks for the information!
Details will be discussed on the ROOT retreat next week.
Test Results 21 files 21 suites 3d 14h 35m 26s ⏱️ For more details on these failures, see this check. Results for commit 9c5ce87. ♻️ This comment has been updated with latest results. |
silverweed
reviewed
Mar 21, 2025
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
4d29c60 to
1224a4b
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
b894456 to
167c0cc
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
342a3cb to
0ca4f8a
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
0ca4f8a to
68de9c4
Compare
1 task
When calling TDirectory::Append(obj, replace=true), and obj is already in the directory, don't issue a warning. Proceed to clear the list of any duplicates, issuing a warning for each. This will enable workflows such as: auto h = new TH1D(...); directory->Append(h, true); After this change, the above lines work both with implicit object ownership off and on.
This makes TEfficiency consistent with other histogram-like classes, in the sense that it only adds itself to a directory if the corresponding flags are on.
- Explain that implicit assignment to directories is likely off in R7. - Make the test insensitive to any defaults by always adding the histogram to the file.
Slightly adapt ownership test to be independent of whether ROOT's implicit ownership settings are on or off.
When histograms get added to a THStack, they are likely in multiple TLists. Therefore, the kMustCleanup bit must be set. In the past, the bit usually got set automatically when histograms added themselves to directories, but when this is disabled, the cleanup bit remained unset.
Also add an assertion to stop the test when executed in the wrong directory. Previously, the test would crash.
The function documentation explicitly states that the histograms created by these operations are added to the current directory, so this is done explicitly here.
Explicitly assign histograms to directories. In this way, the tests proceed irrespective of whether or not implicit object ownership is in use.
The test was relying on implicit ownership to transfer a histogram from python to C++, so now the histogram is appended explicitly.
When implicit histogram ownership is off, the co-ownership of a function both by the global list of functions and by a histogram lead to a double delete.
The tutorial was running code that didn't directly reference the histogram, so it failed when implicit ownership is off.
- Add a silentOff mode for running tests that check the program output.
…ived. - Couple TH1::AddDirectoryStatus to ROOT::DisableImplicitObjectOwnership. - Add a manual exception for TTree::Draw(): Histograms and profiles will still register themselves to gDirectory because the registration is an essential part of the feature.
When running the test via ssh, it might issue the warning about the DISPLAY variable not being set, failing the test.
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
68de9c4 to
9c5ce87
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This is a longer-term effort to update ROOT to work both with implicit and explicit object ownership (i.e. whether THx and similar are owned by gDirectory). The branch started by switching off this ownership and fixing all failing tests. These fixes will be done in a way that the code works both with and without implicit ownership, and will be split off into other PRs to keep the review load manageable. Once that's done, the final names and implementations of the ownership-related functions will be decided here.
Note: No need to review until the following PRs are merged: