From ce3fa0d71099f030b42f7057c12527a3b2c30061 Mon Sep 17 00:00:00 2001 From: Susumu Oda Date: Thu, 24 Oct 2019 05:50:22 +0200 Subject: [PATCH 1/4] Add ATLAS_CHECK_THREAD_SAFETY to InDetTrackSelectorTool package. Remove mutable keyword from m_nHitTrt and m_nHitTrtPlusOutliers in InDetDetailedTrackSelectorTool. Pass nHitTrt and nHitTrtPlusOutliers in one of decision methods. --- .../ATLAS_CHECK_THREAD_SAFETY | 1 + .../InDetDetailedTrackSelectorTool.h | 9 +- .../src/InDetDetailedTrackSelectorTool.cxx | 83 +++++++++++-------- 3 files changed, 53 insertions(+), 40 deletions(-) create mode 100644 InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/ATLAS_CHECK_THREAD_SAFETY diff --git a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/ATLAS_CHECK_THREAD_SAFETY b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/ATLAS_CHECK_THREAD_SAFETY new file mode 100644 index 00000000000..fae68e879fa --- /dev/null +++ b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/ATLAS_CHECK_THREAD_SAFETY @@ -0,0 +1 @@ +InnerDetector/InDetRecTools/InDetTrackSelectorTool diff --git a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/InDetDetailedTrackSelectorTool.h b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/InDetDetailedTrackSelectorTool.h index 4db9ebf6ed6..f2843391c79 100644 --- a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/InDetDetailedTrackSelectorTool.h +++ b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/InDetTrackSelectorTool/InDetDetailedTrackSelectorTool.h @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ #ifndef InDetDetailedTrackSelectorTool_InDetDetailedTrackSelectorTool_H @@ -84,7 +84,8 @@ namespace InDet bool decision(const Trk::FitQuality* TrkQuality) const; bool decision(double chi2, int ndf ) const; bool decision(const Trk::TrackSummary* summary,bool useSharedHitInfo,bool useTrtHitInfo, - const Trk::Perigee* track) const; + const Trk::Perigee* track, + const int nHitTrt, const int nHitTrtPlusOutliers) const; bool preselectionBeforeExtrapolation(const Trk::Perigee & myPerigee) const; Amg::Vector3D getPosOrBeamSpot(const xAOD::Vertex*) const; @@ -111,8 +112,8 @@ namespace InDet int m_nHitPixPhysical; //!< at least n physical hits in pixel int m_nHitSiPhysical; //!< at least n physical hits in pixel+SCT - mutable int m_nHitTrt; //(track.perigeeParameters()); if (perigeeBeforeExtrapolation && m_usePreselectionCuts){ bool preselectionDecision=preselectionBeforeExtrapolation(*perigeeBeforeExtrapolation); @@ -270,13 +272,12 @@ namespace InDet if (m_useTrackSummaryInfo) { //number of hits, silicon hits, b-layer const Trk::TrackSummary* summary = 0; + bool ownSummary = false; // first ask track for summary summary = track.trackSummary(); if (m_trackSumToolAvailable && summary == 0) { - // ugly but one needs to cast the const away because the method needs to update the track (the tool is a friend of track) - Trk::Track& nonConstTrack = const_cast(track); - m_trackSumTool->updateTrack(nonConstTrack); - summary = nonConstTrack.trackSummary(); + summary = m_trackSumTool->createSummary(track); + ownSummary = true; } if (!m_trackSumToolAvailable) { ATH_MSG_WARNING( " No Track Summary Tool available. This should be the case only when running on AOD" ); @@ -287,26 +288,29 @@ namespace InDet } // get the minimum nimber of TRT hits based on eta of the track if(m_useEtaDepententMinHitTrt) { - m_nHitTrt = m_trtDCTool->minNumberDCs( (*track.trackParameters())[0] ); + nHitTrt = m_trtDCTool->minNumberDCs( (*track.trackParameters())[0] ); if(m_addToMinHitTrt!=0){ - m_nHitTrt += m_addToMinHitTrt; + nHitTrt += m_addToMinHitTrt; }else{ - m_nHitTrt = (int)((double)m_nHitTrt*m_scaleMinHitTrt); + nHitTrt = (int)((double)nHitTrt*m_scaleMinHitTrt); } } // get the minimum nimber of TRT hits + outliers based on eta of the track if(m_useEtaDepententMinHitTrtWithOutliers) { - m_nHitTrtPlusOutliers = m_trtDCTool->minNumberDCs( (*track.trackParameters())[0] ); + nHitTrtPlusOutliers = m_trtDCTool->minNumberDCs( (*track.trackParameters())[0] ); if(m_addToMinHitTrtWithOutliers!=0){ - m_nHitTrtPlusOutliers += m_addToMinHitTrtWithOutliers; + nHitTrtPlusOutliers += m_addToMinHitTrtWithOutliers; }else{ - m_nHitTrtPlusOutliers = (int)((double)m_nHitTrtPlusOutliers*m_scaleMinHitTrtWithOutliers); + nHitTrtPlusOutliers = (int)((double)nHitTrtPlusOutliers*m_scaleMinHitTrtWithOutliers); } } - if (!decision(summary,m_useSharedHitInfo,isInTrtAcceptance, perigeeBeforeExtrapolation)) { + if (!decision(summary,m_useSharedHitInfo,isInTrtAcceptance, perigeeBeforeExtrapolation, + nHitTrt, nHitTrtPlusOutliers)) { + if (ownSummary) delete summary; summary=0; return false; } + if (ownSummary) delete summary; summary=0; } return true; @@ -315,6 +319,8 @@ namespace InDet // --------------------------------------------------------------------- bool InDetDetailedTrackSelectorTool::decision(const Trk::TrackParticleBase& track,const Trk::Vertex* vertex) const{ + int nHitTrt = m_nHitTrt; + int nHitTrtPlusOutliers = m_nHitTrtPlusOutliers; const Trk::TrackParameters* definintParameters=&(track.definingParameters()); const Trk::Perigee* perigeeBeforeExtrapolation=dynamic_cast(definintParameters); if (perigeeBeforeExtrapolation && m_usePreselectionCuts) { @@ -348,22 +354,24 @@ namespace InDet return false; } if(m_useEtaDepententMinHitTrt) { - m_nHitTrt = m_trtDCTool->minNumberDCs( (track.trackParameters())[0] ); + nHitTrt = m_trtDCTool->minNumberDCs( (track.trackParameters())[0] ); if(m_addToMinHitTrt!=0){ - m_nHitTrt += m_addToMinHitTrt; + nHitTrt += m_addToMinHitTrt; }else{ - m_nHitTrt = (int)((double)m_nHitTrt*m_scaleMinHitTrt); + nHitTrt = (int)((double)nHitTrt*m_scaleMinHitTrt); } } if(m_useEtaDepententMinHitTrtWithOutliers) { - m_nHitTrtPlusOutliers = m_trtDCTool->minNumberDCs( (track.trackParameters())[0] ); + nHitTrtPlusOutliers = m_trtDCTool->minNumberDCs( (track.trackParameters())[0] ); if(m_addToMinHitTrtWithOutliers!=0){ - m_nHitTrtPlusOutliers += m_addToMinHitTrtWithOutliers; + nHitTrtPlusOutliers += m_addToMinHitTrtWithOutliers; }else{ - m_nHitTrtPlusOutliers = (int)((double)m_nHitTrtPlusOutliers*m_scaleMinHitTrtWithOutliers); + nHitTrtPlusOutliers = (int)((double)nHitTrtPlusOutliers*m_scaleMinHitTrtWithOutliers); } } - if ((!perigeeBeforeExtrapolation) or (!decision(summary, m_useSharedHitInfo, isInTrtAcceptance, perigeeBeforeExtrapolation))) { + if ((!perigeeBeforeExtrapolation) or + (!decision(summary, m_useSharedHitInfo, isInTrtAcceptance, perigeeBeforeExtrapolation, + nHitTrt, nHitTrtPlusOutliers))) { return false; } } @@ -416,7 +424,7 @@ namespace InDet } if (extrapolatedParameters) ATH_MSG_VERBOSE ("Result: " << *extrapolatedParameters); const Trk::RecVertex* recVertex = dynamic_cast(myVertex); - bool dec = decision(extrapolatedPerigee, recVertex ? &recVertex->covariancePosition() : 0 ); + bool dec = decision(extrapolatedPerigee, recVertex ? &recVertex->covariancePosition() : 0 ); // D if (myVertex!=vertex) { delete myVertex; myVertex=0; @@ -442,8 +450,10 @@ namespace InDet // --------------------------------------------------------------------- bool - InDetDetailedTrackSelectorTool::decision(const xAOD::TrackParticle& tp,const xAOD::Vertex* vertex) const + InDetDetailedTrackSelectorTool::decision(const xAOD::TrackParticle& tp,const xAOD::Vertex* vertex) const { + int nHitTrt = m_nHitTrt; + int nHitTrtPlusOutliers = m_nHitTrtPlusOutliers; const Trk::Perigee& perigee=tp.perigeeParameters(); if (m_usePreselectionCuts && !preselectionBeforeExtrapolation(perigee)) { @@ -460,19 +470,19 @@ namespace InDet //number of hits, silicon hits, b-layer if(m_useEtaDepententMinHitTrt) { - m_nHitTrt = m_trtDCTool->minNumberDCs( &perigee ); + nHitTrt = m_trtDCTool->minNumberDCs( &perigee ); if(m_addToMinHitTrt!=0){ - m_nHitTrt += m_addToMinHitTrt; + nHitTrt += m_addToMinHitTrt; }else{ - m_nHitTrt = (int)((double)m_nHitTrt*m_scaleMinHitTrt); + nHitTrt = (int)((double)nHitTrt*m_scaleMinHitTrt); } } if(m_useEtaDepententMinHitTrtWithOutliers) { - m_nHitTrtPlusOutliers = m_trtDCTool->minNumberDCs( &perigee ); + nHitTrtPlusOutliers = m_trtDCTool->minNumberDCs( &perigee ); if(m_addToMinHitTrtWithOutliers!=0){ - m_nHitTrtPlusOutliers += m_addToMinHitTrtWithOutliers; + nHitTrtPlusOutliers += m_addToMinHitTrtWithOutliers; }else{ - m_nHitTrtPlusOutliers = (int)((double)m_nHitTrtPlusOutliers*m_scaleMinHitTrtWithOutliers); + nHitTrtPlusOutliers = (int)((double)nHitTrtPlusOutliers*m_scaleMinHitTrtWithOutliers); } } int nb = getCount(tp,xAOD::numberOfInnermostPixelLayerHits ); @@ -569,14 +579,14 @@ namespace InDet if (std::fabs(tp.eta())>m_TrtMaxEtaAcceptance) { int nh = getCount(tp,xAOD::numberOfTRTHits); - if(nh < m_nHitTrt) { - ATH_MSG_DEBUG("Track rejected because of nHitTrt "<get(Trk::numberOfTRTHits); if(nh<0) nh=0; - if(nh < m_nHitTrt) { - ATH_MSG_DEBUG("Track rejected because of nHitTrt "<get( Trk::numberOfTRTHits ) + summary->get( Trk::numberOfTRTOutliers ); if (nhh<0) nhh=0; - if (nhh Date: Thu, 24 Oct 2019 05:52:24 +0200 Subject: [PATCH 2/4] Remove a debug comment --- .../src/InDetDetailedTrackSelectorTool.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetDetailedTrackSelectorTool.cxx b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetDetailedTrackSelectorTool.cxx index 765948cfd2c..ccf7cb89b9d 100644 --- a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetDetailedTrackSelectorTool.cxx +++ b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetDetailedTrackSelectorTool.cxx @@ -424,7 +424,7 @@ namespace InDet } if (extrapolatedParameters) ATH_MSG_VERBOSE ("Result: " << *extrapolatedParameters); const Trk::RecVertex* recVertex = dynamic_cast(myVertex); - bool dec = decision(extrapolatedPerigee, recVertex ? &recVertex->covariancePosition() : 0 ); // D + bool dec = decision(extrapolatedPerigee, recVertex ? &recVertex->covariancePosition() : 0 ); if (myVertex!=vertex) { delete myVertex; myVertex=0; -- GitLab From 54ca7ab43ea1f156407756807816b7727567ec93 Mon Sep 17 00:00:00 2001 From: Susumu Oda Date: Thu, 24 Oct 2019 14:27:15 +0200 Subject: [PATCH 3/4] Remove const_cast --- .../src/InDetCosmicTrackSelectorTool.cxx | 15 +++++++++------ .../src/InDetTrackSelectorTool.cxx | 15 +++++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx index a81a475934b..b6dcf8c5851 100644 --- a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx +++ b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx @@ -1,5 +1,5 @@ /* - Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration + Copyright (C) 2002-2019 CERN for the benefit of the ATLAS collaboration */ #include "InDetTrackSelectorTool/InDetCosmicTrackSelectorTool.h" @@ -90,13 +90,11 @@ namespace InDet // number of hits, silicon hits, b-layer // first ask track for summary + bool ownSummary = false; const Trk::TrackSummary * summary = track.trackSummary(); if (summary == 0 && m_trackSumToolAvailable) { - // ugly but one needs to cast the const away because the method needs to update the track (the tool is a friend of track) - Trk::Track& nonConstTrack = const_cast(track); - m_trackSumTool->updateTrack(nonConstTrack); - // now get it from the track (the track has OWNERSHIP) - summary = nonConstTrack.trackSummary(); + summary = m_trackSumTool->createSummary(track); + ownSummary = true; } if (0==summary) { @@ -111,6 +109,11 @@ namespace InDet int nSiHitsTop = getNSiHits(&track,true); int nSiHitsBottom = getNSiHits(&track,false); + if (ownSummary) { + delete summary; + summary = nullptr; + } + if(nPixHits(track); - m_trackSumTool->updateTrack(nonConstTrack); - // now get it from the track (the track has OWNERSHIP) - summary = nonConstTrack.trackSummary(); + summary = m_trackSumTool->createSummary(track); + ownSummary = true; } if (0==summary) { @@ -116,6 +114,11 @@ bool InDetTrackSelectorTool::decision(const Trk::Track & track, const Trk::Verte int nBLayerHits = summary->get(Trk::numberOfInnermostPixelLayerHits); + if (ownSummary) { + delete summary; + summary = nullptr; + } + if(nPixelHits+nPixelDead Date: Fri, 25 Oct 2019 15:56:50 +0200 Subject: [PATCH 4/4] Use std::unique_ptr --- .../src/InDetCosmicTrackSelectorTool.cxx | 11 +++-------- .../src/InDetDetailedTrackSelectorTool.cxx | 13 ++++--------- .../src/InDetTrackSelectorTool.cxx | 11 +++-------- 3 files changed, 10 insertions(+), 25 deletions(-) diff --git a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx index b6dcf8c5851..49c81b0b256 100644 --- a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx +++ b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetCosmicTrackSelectorTool.cxx @@ -90,11 +90,11 @@ namespace InDet // number of hits, silicon hits, b-layer // first ask track for summary - bool ownSummary = false; + std::unique_ptr summaryUniquePtr; const Trk::TrackSummary * summary = track.trackSummary(); if (summary == 0 && m_trackSumToolAvailable) { - summary = m_trackSumTool->createSummary(track); - ownSummary = true; + summaryUniquePtr = m_trackSumTool->summary(track); + summary = summaryUniquePtr.get(); } if (0==summary) { @@ -109,11 +109,6 @@ namespace InDet int nSiHitsTop = getNSiHits(&track,true); int nSiHitsBottom = getNSiHits(&track,false); - if (ownSummary) { - delete summary; - summary = nullptr; - } - if(nPixHits summaryUniquePtr; + const Trk::TrackSummary* summary = track.trackSummary(); if (m_trackSumToolAvailable && summary == 0) { - summary = m_trackSumTool->createSummary(track); - ownSummary = true; + summaryUniquePtr = m_trackSumTool->summary(track); + summary = summaryUniquePtr.get(); } if (!m_trackSumToolAvailable) { ATH_MSG_WARNING( " No Track Summary Tool available. This should be the case only when running on AOD" ); @@ -306,12 +305,8 @@ namespace InDet } if (!decision(summary,m_useSharedHitInfo,isInTrtAcceptance, perigeeBeforeExtrapolation, nHitTrt, nHitTrtPlusOutliers)) { - if (ownSummary) delete summary; - summary=0; return false; } - if (ownSummary) delete summary; - summary=0; } return true; } diff --git a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetTrackSelectorTool.cxx b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetTrackSelectorTool.cxx index 8ff61b05d0c..6e45d318c95 100644 --- a/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetTrackSelectorTool.cxx +++ b/InnerDetector/InDetRecTools/InDetTrackSelectorTool/src/InDetTrackSelectorTool.cxx @@ -95,11 +95,11 @@ bool InDetTrackSelectorTool::decision(const Trk::Track & track, const Trk::Verte // number of hits, silicon hits, b-layer // first ask track for summary - bool ownSummary = false; + std::unique_ptr summaryUniquePtr; const Trk::TrackSummary * summary = track.trackSummary(); if (summary == 0 && m_trackSumToolAvailable) { - summary = m_trackSumTool->createSummary(track); - ownSummary = true; + summaryUniquePtr = m_trackSumTool->summary(track); + summary = summaryUniquePtr.get(); } if (0==summary) { @@ -114,11 +114,6 @@ bool InDetTrackSelectorTool::decision(const Trk::Track & track, const Trk::Verte int nBLayerHits = summary->get(Trk::numberOfInnermostPixelLayerHits); - if (ownSummary) { - delete summary; - summary = nullptr; - } - if(nPixelHits+nPixelDead