cms-sw
ROOT's TH1::AddDirectory is not thread safe
TH1::AddDirectory allows one to turn off automatically adding histograms to the most recently opened TFile. By default, cmsRun has calls TH1::AddDirectory(false) before any plugins are loaded. Unfortunately, there are several modules which reset the value to true (even just temporarily).
A new Issue was created by @Dr15Jones.
@Dr15Jones, @ftenchini, @makortel, @mandrenguyen, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.
cms-bot commands are listed here
$ git grep --name-only "TH1::AddDirectory("
Alignment/CommonAlignmentProducer/plugins/GlobalTrackerMuonAlignment.cc
Alignment/OfflineValidation/bin/haddws.C
Alignment/OfflineValidation/macros/FitPVResiduals.C
Alignment/OfflineValidation/macros/MultiRunAndPlotPVValidation.C
Alignment/OfflineValidation/scripts/merge_TrackerOfflineValidation.C
Alignment/OfflineValidation/src/CompareAlignments.cc
Alignment/OfflineValidation/src/PreparePVTrends.cc
CalibTracker/SiStripChannelGain/plugins/SiStripGainFromData.cc
CalibTracker/SiStripChannelGain/test/CodeExample/merge.C
CaloOnlineTools/EcalTools/plugins/EcalPulseShapeGrapher.cc
CommonTools/Utils/src/TH1AddDirectorySentry.cc
CondTools/SiStrip/macros/noise_analysis.C
DQMServices/Components/plugins/DQMGenericClient.cc
FWCore/ROOTTests/test/tprofile_threaded_t.cc
FWCore/Services/plugins/InitRootHandlers.cc
Fireworks/Calo/src/FWCaloDataHistProxyBuilder.cc
Fireworks/Core/src/Context.cc
Fireworks/Core/src/FWLegoViewBase.cc
L1Trigger/TrackFindingTracklet/test/INTERNAL/HistImp.cc
MuonAnalysis/MomentumScaleCalibration/test/Macros/ResolDraw.cc
PhysicsTools/FWLite/src/EventContainer.cc
RecoLocalMuon/DTSegment/test/DTAnalyzerDetailed.cc
RecoLocalMuon/DTSegment/test/DTClusAnalyzer.cc
RecoLocalMuon/DTSegment/test/DTEffAnalyzer.cc
RecoLocalMuon/DTSegment/test/DTSegAnalyzer.cc
RecoTracker/DebugTools/plugins/TestOutliers.cc
SimG4CMS/FP420/plugins/FP420Test.cc
SimG4Core/GFlash/plugins/GflashG4Watcher.cc
SimGeneral/GFlash/src/GflashHistogram.cc
Validation/RecoMuon/test/multiPlotter.cpp
Validation/RecoMuon/test/remakeComposites.C
Validation/RecoParticleFlow/src/Comparator.cc
Validation/RecoParticleFlow/src/TH2Analyzer.cc
Validation/RecoTau/plugins/DQMFileLoader.cc
Validation/RecoTau/plugins/DQMHistPlotter.cc
$ git grep --name-only "TH1AddDirectorySentry"
CalibMuon/DTCalibration/plugins/DTResidualCalibration.cc
CommonTools/Utils/interface/TFileDirectory.h
CommonTools/Utils/interface/TH1AddDirectorySentry.h
CommonTools/Utils/src/TFileDirectory.cc
CommonTools/Utils/src/TH1AddDirectorySentry.cc
assign Alignment/CommonAlignmentProducer,Alignment/OfflineValidation,CalibTracker/SiStripChannelGain,CaloOnlineTools/EcalTools,CommonTools/Utils,CondTools/SiStrip,DQMServices/Components,FWCore/ROOTTests,FWCore/Services,Fireworks/Calo,Fireworks/Core,L1Trigger/TrackFindingTracklet,MuonAnalysis/MomentumScaleCalibration,PhysicsTools/FWLite,RecoLocalMuon/DTSegment,RecoTracker/DebugTools,SimG4CMS/FP420,SimG4Core/GFlash,SimGeneral/GFlash,Validation/RecoMuon,Validation/RecoParticleFlow,Validation/RecoTau
New categories assigned: alca,reconstruction,db,dqm,core,visualization,l1,analysis,simulation
@Alejandro1400,@alja,@arunhep,@atpathak,@BenjaminRS,@civanch,@ctarricone,@Dr15Jones,@francescobrivio,@gabrielmscampos,@JanChyczynski,@jfernan2,@kpedro88,@makortel,@mandrenguyen,@mdhildreth,@nothingface0,@perrotta,@quinnanm,@rseidita,@smuzaffar,@srimanob,@tvami you have been requested to review this Pull request/Issue and eventually sign? Thanks
FWCore/ROOTTests/test/tprofile_threaded_t.cc FWCore/Services/plugins/InitRootHandlers.cc
The calls in these files are in single-threaded sections.
Please excuse my ignorance, for my understanding: in most of the cases the change implies to simply remove lines like:
https://github.com/cms-sw/cmssw/blob/master/RecoTracker/DebugTools/plugins/TestOutliers.cc#L943
where TH1::AddDirectory(true); -> remove
But for those cases in:
CalibMuon/DTCalibration/plugins/DTResidualCalibration.cc CommonTools/Utils/interface/TFileDirectory.h CommonTools/Utils/interface/TH1AddDirectorySentry.h CommonTools/Utils/src/TFileDirectory.cc CommonTools/Utils/src/TH1AddDirectorySentry.cc
the value is not reset, apparently.
Any guidance? Thank you in advance.
the value is not reset, apparently.
https://github.com/cms-sw/cmssw/blob/61a1113117cf3a7aafee41dee39a175dbda615f4/CommonTools/Utils/src/TH1AddDirectorySentry.cc#L31-L33
TH1AddDirectorySentry resets the value back to what it was before the constructor call of TH1AddDirectorySentry. Presumably, it was set to false before. However, even this change isn't safe as other threads could be reading/writing the value.
for my understanding: in most of the cases the change implies to simply remove lines like:
https://github.com/cms-sw/cmssw/blob/61a1113117cf3a7aafee41dee39a175dbda615f4/RecoTracker/DebugTools/plugins/TestOutliers.cc#L944-L946
So for the case you point out, the best change would be to do
file = new TFile(out.c_str(), "recreate");
histoPtOut = new TH1F("histoPtOut", "histoPtOut", 100, -10, 10);
histoPtOut->SetDirectory(file);
histoPtOld = new TH1F("histoPtOld", "histoPtOld", 100, -10, 10);
histoPtOld->SetDirectory(file);
...
As that will explicitly add the histogram to the file.
If one does not call SetDirectory then the created histogram will not be bound to any TFile with TH1::AddDirectory(false) being set.
Thank you @Dr15Jones I have implemented those belonging to RECO in https://github.com/cms-sw/cmssw/pull/49548
Alignment adressed in https://github.com/cms-sw/cmssw/pull/49567 For CondTools/SiStrip/macros/noise_analysis.C seems that no change is required.
@Dr15Jones Fireworks creates a histogram in memory for calorimeter visualization. The TH1::AddDirectory(kFALSE) is called in all 3 places. Here is the example:
https://github.com/cms-sw/cmssw/blob/master/Fireworks/Core/src/Context.cc#L139-L146
The TH1::AddDirectory(kFALSE) is already called.
Do I need to do anything extra to ensure thread safety for the TH1::AddDirectory call?
