[RF] Inefficient un-registering of histograms in HistFactory

[TomasDado]

Explain what you would like to see improved and how.

While doing some profiling check with VTune with @guitargeek we noticed that in some extreme cases the bottleneck can come from the un-registering of histograms during the creation of the WS in the HistFactory MakeModelAndMeasurementFast() method.

Please, see the attached result of VTune.

ROOT version

ROOT Version: 6.35.01
Built for linuxx8664gcc on Mar 27 2025, 14:27:43
From heads/master@1c94b446

Installation method

build from source with debug symbols on

Operating system

Ubuntu 24.10

Additional context

No response

[guitargeek]

Thank you very much for opening this issue! Even though you marked it as a RooFit issue, I wanted to point out that we're talking about the TH1D destructor here, which seems to become expensive once you have many histograms registered to the global list of histograms in gROOT. Not attaching the histograms to a TDirectory in the case of the HistFactory::Measurement class might be a solution, but if someone sees a way to make deleting registered histograms faster that would we even better! Then we don't have to change the ownership models of histograms for HistFactory.

[dpiparo]

@pcanal

[hageboeck]

Thank you very much for opening this issue! Even though you marked it as a RooFit issue, I wanted to point out that we're talking about the TH1D destructor here, which seems to become expensive once you have many histograms registered to the global list of histograms in gROOT. Not attaching the histograms to a TDirectory in the case of the HistFactory::Measurement class might be a solution, but if someone sees a way to make deleting registered histograms faster that would we even better! Then we don't have to change the ownership models of histograms for HistFactory.

Hello,
I would say that your suggestion of not attaching to the current directory is actually the right one. The Measurement owns the pointer, and it also actively takes care of writing the histograms (in the few functions I checked), so there should be no need to have automatic registration and de-registration, which causes the slowdown.
I made a PR that disables registration for the time that MakeModelAndMeasurementFast runs. @TomasDado, would you be able to test it out?
It's unlikely, but in case the HistFactory authors forgot the writing of a histogram, disabling the registration would prevent it from showing up in the output file. So testing that it's 1) faster and 2) still correct would help tremendously. The CI is of course also testing HistFactory already, which should tell us if it's very wrong.

[TomasDado]

Thanks, yes I can test the changes on Monday, I hope that is okay. One thing I should have mentioned that this is a bottleneck in the WS I tested only after the changes from #18174 are included. So I would need to test the changes on top of the improvements in RooRealIntegral.

[hageboeck]

Thanks, yes I can test the changes on Monday, I hope that is okay. One thing I should have mentioned that this is a bottleneck in the WS I tested only after the changes from #18174 are included. So I would need to test the changes on top of the improvements in RooRealIntegral.

OK, no problem! And I just rebased it, so the work is on top of the RooRealIntegral improvement now.

[TomasDado]

Thanks for the updates. Unfortunately, none of these changes have any imapct on the running time. Likely because I realised we already sisable the registering globally with TH1::AddDirectory(kFALSE)