[Root7] A path to reducing global memory management.#18083
Draft
hageboeck wants to merge 27 commits intoroot-project:masterfrom
Draft
[Root7] A path to reducing global memory management.#18083hageboeck wants to merge 27 commits intoroot-project:masterfrom
hageboeck wants to merge 27 commits intoroot-project:masterfrom
Conversation
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 15 files 15 suites 2d 8h 20m 43s ⏱️ Results for commit 8d9511a. ♻️ 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
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
68de9c4 to
9c5ce87
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
9c5ce87 to
b79032e
Compare
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.
- Don't rely on implicit registration to directories. - Make the test insensitive to any defaults by always adding the histogram to the file.
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.
The function documentation explicitly states that the histograms created by these operations are added to the current directory, so this is done explicitly here. This makes the code independent of whether implicit ownership is on or off.
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 DisableImplicitObjectOwnership() and EnableImplicitObjectOwnership(). - Add a function to test if ownership is on or off. - Add a silentOff mode for running tests that check the program output. - Allow for setting defaults using - The environment variable ROOT_IMPLICIT_OWNERSHIP - The rootrc variable Root.ImplicitOwnership
…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.
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
b79032e to
74a6279
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
74a6279 to
8d9511a
Compare
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
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.
TODOs:
Note: No need to review until the following PRs are merged:
gDirectory, use TContext instead. #21287