TH1::Fit thread safety clang analyzer warning

[smuzaffar]

Clang static analyzer warnings about

  src/Validation/RecoParticleFlow/plugins/PFJetDQMPostProcessor.cc:406:3: warning: Known thread unsafe function TH1::Fit(const char *, const char *, const char *, double, double) is called in function PFJetDQMPostProcessor::fitResponse(TH1F *, TH1F *, int, int, double, double &, double &, double &, double &) [threadsafety.ThrUnsafeFCallChecker]

This is because we have TH1:Fit(..) explicitly marked as thread unsafe . ROOT 6.06 doc says

Improve thread safety of TH1::Fit by making static member of TVirtualFitter and TMinuitMinimizer thread local.  This fixes [ROOT-7791].

So do we still see it as thread unsafe?

[smuzaffar]

assign core

[cmsbuild]

New categories assigned: core

@Dr15Jones,@makortel,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @smuzaffar.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

ROOT 6.06 doc says

Improve thread safety of TH1::Fit by making static member of TVirtualFitter and TMinuitMinimizer thread local.  This fixes [ROOT-7791].

So do we still see it as thread unsafe?

I wouldn't give this note much value, since I see we've made fixes in e.g. https://github.com/cms-sw/cmssw/pull/22227 that in a release cycle that used ROOT 6.10.

[makortel]

Chatting with @Dr15Jones there are likely many thread-unsafe features still in TH1::Fit(), like having to pass SERIAL fit option or avoiding addition of functions in the global list.

@dan131riley Do you have anything to add?

[makortel]

FYI @pcanal

[pcanal]

like having to pass SERIAL fit option or avoiding addition of functions in the global list.

Apriori this has already been made thread safe albeit disabling it when not needed will improve scaling.

[dan131riley]

We added SERIAL in part because TH1::Fit() used to default to the old TMinuit, which had (and probably still does have) some subtle threading issues. I think (hope?) ROOT doesn't default to TMinuit.

I've never been completely confident that the threading issues with the global function cache have been entirely resolved, it also had some subtle issues.