Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HistFactory] Disable global histogram registration for HistFactory. #18182

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hageboeck
Copy link
Member

The HistFactory histograms are owned by the HistFactory models, and they are explicitly written to files. Therefore, they should not participate in directory registration.

Fix #18172

@hageboeck hageboeck self-assigned this Mar 28, 2025
@hageboeck hageboeck force-pushed the histFactoryCleanups branch 2 times, most recently from 7b34874 to 7b0297d Compare March 28, 2025 12:15
@hageboeck hageboeck changed the title [HistFactory] Disable TH1::SetDirectory for HistFactory. [HistFactory] Disable global histogram registration for HistFactory. Mar 28, 2025
Copy link

github-actions bot commented Mar 28, 2025

Test Results

    19 files      19 suites   4d 11h 37m 54s ⏱️
 2 717 tests  2 716 ✅ 0 💤 1 ❌
49 832 runs  49 831 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit ec75ccc.

♻️ This comment has been updated with latest results.

Comment on lines 86 to 88
TDirectory::TContext dirContext;
std::unique_ptr<TFile> outFile;
gDirectory = nullptr; // Disable global registration of histograms this file. HistFactory places them explicitly later.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TDirectory::TContext dirContext;
std::unique_ptr<TFile> outFile;
gDirectory = nullptr; // Disable global registration of histograms this file. HistFactory places them explicitly later.
TDirectory::TContext dirContext{nullptr} // Disable global registration of histograms this file. HistFactory places them explicitly later.
std::unique_ptr<TFile> outFile;

Since the next line does not allocated a TFile and thus does not change gDirectory

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However line 138:

outFile = std::make_unique<TFile>(outputFileName.c_str(), "recreate");

should probably be:

outFile = std::make_unique<TFile>(outputFileName.c_str(), "RECREATE_WITHOUT_GLOBALREGISTRATION") // note: casing does not matter.
gDirectory = nullptr;  // Make sure the files does not become the current directory.  We do not need TContext here since we have one on the outer scope.

Side note the _WITHOUT_GLOBALREGISTRATION is probably the only thing strictly necessary for the goal of this PR (i.e. since it is not registered, it is not on the list of Cleanups and thus will not be traversed on delete of the histograms).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I had no idea this option existed! That's really good to know!

@TomasDado
Copy link
Contributor

TomasDado commented Mar 31, 2025

Hello, the execution is slightly faster from ~260s to about 230s but it seems most of the time is still spent in the destructors

tomas@tomas-IdeaPad-5-Pro-14IAP7:~/TRExFitter$ root --version
ROOT Version: 6.35.01
Built for linuxx8664gcc on Mar 31 2025, 07:22:16
From remotes/hageboeck/histFactoryCleanups@7b0297d5e9

Screenshot From 2025-03-31 09-47-28

@hageboeck hageboeck force-pushed the histFactoryCleanups branch from 7b0297d to 18a27e3 Compare March 31, 2025 11:06
@hageboeck
Copy link
Member Author

Hello, the execution is slightly faster from ~260s to about 230s but it seems most of the time is still spent in the destructors

Hello @TomasDado, my attempt at changing it was mostly without effect as pointed out by Philippe. I force-pushed a new version of the commit that should have larger impact. Could you try again, please?

@TomasDado
Copy link
Contributor

TomasDado commented Mar 31, 2025

Hello, the execution is slightly faster from ~260s to about 230s but it seems most of the time is still spent in the destructors

Hello @TomasDado, my attempt at changing it was mostly without effect as pointed out by Philippe. I force-pushed a new version of the commit that should have larger impact. Could you try again, please?

Unfortunately, it still looks very similar, running time of about 230s:

tomas@tomas-IdeaPad-5-Pro-14IAP7:~/TRExFitter$ root --version
ROOT Version: 6.35.01
Built for linuxx8664gcc on Mar 31 2025, 11:12:47
From remotes/hageboeck/histFactoryCleanups@18a27e3303

Screenshot From 2025-03-31 13-22-39

The HistFactory histograms are owned by the HistFactory models, and they
are explicitly written to files. Therefore, they should not participate
in global directory registration.
@hageboeck hageboeck force-pushed the histFactoryCleanups branch from 18a27e3 to eeba948 Compare April 4, 2025 13:15
HistFactory models can suffer from slowdowns of minutes during
destruction because the list of cleanups is visited over and over again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RF] Inefficient un-registering of histograms in HistFactory
3 participants