[Root7] A path to reducing global memory management. #18083
Draft
+342
−161
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 20 files 20 suites 3d 14h 45m 40s ⏱️ For more details on these failures, see this check. Results for commit 0ca4f8a. ♻️ 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
Although fallbacks to the cpu backend have been implemented, trying to test RooFit without clad leads to a gtest failure, since the test suite will be instantiated three times with the same backend.
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.
Diffs of the outputs are fragile, so the ownership of histograms is now tested programmatically.
If implicit object ownership gets in TDirectories gets switched off, the example histograms need to be added to directories or written explicitly.
- 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 test broke when implicit object ownership was switched off in ROOT. However, this test can also break in other circumstances, because del histogram generally does not destroy the C++ object immediately. It used to work because only a single reference to the object was alive (which changed when trying to prepare the test for implicit object ownership off), but one cannot base a test on undocumented details of a Python implementation. For this reason, it does not make sense to test if gDirectory (C++) contains an entry for the object that was just "del"-ed.
The function documentation explicitly states that the histograms created by these operations are added to the current directory, so this is done explicitly here.
When calling TDirectory::Append(obj, replace=true), and obj is already in the directory, return without doing anything. If obj has the same name as an existing object, but is physically different, the usual warning is raised. 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 pattern will allow writing robust programs with consistent behaviour.
Explicitly assign histograms to directories. In this way, the tests proceed irrespective of whether or not implicit object ownership is in use.
nbconvert was creating an invalid path when creating an output notebook.
This seems to result from a behaviour change that doesn't anymore trim a
redundant path from a notebook name ("a/b/notebook" was transformed to
"notebook" if the input notebook is in a/b).
In nbconvert 7.16.6, this became "a/b/a/b/notebook".
Since nbconvert 5.0, the notebook is anyway placed next to the input
notebook instead of the current working directory, so the output path
can be removed from the command.
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.
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
342a3cb to
0ca4f8a
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.
Details to be updated.
Needs the following PRs to be merged first: