Skip to content

Commit

Permalink
Merge pull request #1347 from wmtan/RemoveMutablesFromMuonDetIdAssoci…
Browse files Browse the repository at this point in the history 
…ator

Multithreading fixes -- Fix thread safety issues by removing mutable data members of MuonDetIdAssociator
  • Loading branch information
@ktf
ktf committed Nov 7, 2013
2 parents d1d1d9a + 3dfc4e3 commit b18d59a
Show file tree
Hide file tree
Showing 10 changed files with 69 additions and 65 deletions.
4 changes: 2 additions & 2 deletions TrackingTools/TrackAssociator/interface/DetIdAssociator.h
View file
Expand Up @@ -129,8 +129,8 @@ class DetIdAssociator{

virtual GlobalPoint getPosition(const DetId&) const = 0;
virtual const unsigned int getNumberOfSubdetectors() const { return 1;}
virtual const std::vector<DetId>& getValidDetIds(unsigned int subDetectorIndex) const = 0;
virtual std::pair<const_iterator, const_iterator> getDetIdPoints(const DetId&) const = 0;
virtual void getValidDetIds(unsigned int subDetectorIndex, std::vector<DetId>&) const = 0;
virtual std::pair<const_iterator, const_iterator> getDetIdPoints(const DetId&, std::vector<GlobalPoint>&) const = 0;

virtual bool insideElement(const GlobalPoint&, const DetId&) const = 0;
virtual bool crossedElement(const GlobalPoint&,
Expand Down
9 changes: 5 additions & 4 deletions TrackingTools/TrackAssociator/plugins/CaloDetIdAssociator.cc
View file
Expand Up @@ -7,7 +7,8 @@ bool CaloDetIdAssociator::crossedElement(const GlobalPoint& point1,
const SteppingHelixStateInfo* initialState
) const
{
const std::pair<const_iterator,const_iterator>& points = getDetIdPoints(id);
std::vector<GlobalPoint> pointBuffer;
const std::pair<const_iterator,const_iterator>& points = getDetIdPoints(id, pointBuffer);
// fast check
bool xLess(false), xIn(false), xMore(false);
bool yLess(false), yIn(false), yMore(false);
Expand Down Expand Up @@ -200,14 +201,14 @@ GlobalPoint CaloDetIdAssociator::getPosition(const DetId& id) const {
return geometry_->getSubdetectorGeometry(id)->getGeometry(id)->getPosition();
}

const std::vector<DetId>& CaloDetIdAssociator::getValidDetIds(unsigned int subDectorIndex) const
void CaloDetIdAssociator::getValidDetIds(unsigned int subDectorIndex, std::vector<DetId>& detIds) const
{
if ( subDectorIndex!=0 ) cms::Exception("FatalError") << "Calo sub-dectors are all handle as one sub-system, but subDetectorIndex is not zero.\n";
return geometry_->getValidDetIds(DetId::Calo, 1);
detIds = geometry_->getValidDetIds(DetId::Calo, 1);
}

std::pair<DetIdAssociator::const_iterator, DetIdAssociator::const_iterator>
CaloDetIdAssociator::getDetIdPoints(const DetId& id) const
CaloDetIdAssociator::getDetIdPoints(const DetId& id, std::vector<GlobalPoint>& points) const
{
const CaloSubdetectorGeometry* subDetGeom = geometry_->getSubdetectorGeometry(id);
if(! subDetGeom){
Expand Down
20 changes: 10 additions & 10 deletions TrackingTools/TrackAssociator/plugins/CaloDetIdAssociator.h
View file
Expand Up @@ -38,32 +38,32 @@ class CaloDetIdAssociator: public DetIdAssociator{
CaloDetIdAssociator(const edm::ParameterSet& pSet)
:DetIdAssociator(pSet.getParameter<int>("nPhi"),pSet.getParameter<int>("nEta"),pSet.getParameter<double>("etaBinSize")),geometry_(0){};

virtual void setGeometry(const CaloGeometry* ptr){ geometry_ = ptr; };
virtual void setGeometry(const CaloGeometry* ptr) { geometry_ = ptr; };

virtual void setGeometry(const DetIdAssociatorRecord& iRecord);
virtual void setGeometry(const DetIdAssociatorRecord& iRecord) override;

virtual const GeomDet* getGeomDet(const DetId& id) const { return 0; };
virtual const GeomDet* getGeomDet(const DetId& id) const override { return 0; };

virtual const char* name() const { return "CaloTowers"; }
virtual const char* name() const override { return "CaloTowers"; }

protected:
virtual void check_setup() const;
virtual void check_setup() const override;

virtual GlobalPoint getPosition(const DetId& id) const;
virtual GlobalPoint getPosition(const DetId& id) const override;

virtual const std::vector<DetId>& getValidDetIds( unsigned int subDetectorIndex ) const;
virtual void getValidDetIds( unsigned int subDetectorIndex, std::vector<DetId>& ) const override;

virtual std::pair<const_iterator, const_iterator> getDetIdPoints(const DetId& id) const;
virtual std::pair<const_iterator, const_iterator> getDetIdPoints(const DetId& id, std::vector<GlobalPoint>& points) const override;

virtual bool insideElement(const GlobalPoint& point, const DetId& id) const {
virtual bool insideElement(const GlobalPoint& point, const DetId& id) const override{
return geometry_->getSubdetectorGeometry(id)->getGeometry(id)->inside(point);
};

virtual bool crossedElement(const GlobalPoint&,
const GlobalPoint&,
const DetId& id,
const double tolerance = -1,
const SteppingHelixStateInfo* = 0 ) const;
const SteppingHelixStateInfo* = 0 ) const override;
const CaloGeometry* geometry_;
std::vector<GlobalPoint> dummy_;
};
Expand Down
10 changes: 5 additions & 5 deletions TrackingTools/TrackAssociator/plugins/EcalDetIdAssociator.h
View file
Expand Up @@ -26,16 +26,16 @@ class EcalDetIdAssociator: public CaloDetIdAssociator{

EcalDetIdAssociator(const edm::ParameterSet& pSet):CaloDetIdAssociator(pSet){};

virtual const char* name() const { return "ECAL"; }
virtual const char* name() const override { return "ECAL"; }

protected:

virtual const unsigned int getNumberOfSubdetectors() const { return 2;}
virtual const std::vector<DetId>& getValidDetIds(unsigned int subDetectorIndex) const {
virtual const unsigned int getNumberOfSubdetectors() const override { return 2;}
virtual void getValidDetIds(unsigned int subDetectorIndex, std::vector<DetId>& validIds) const {
if ( subDetectorIndex == 0 )
return geometry_->getValidDetIds(DetId::Ecal, EcalBarrel);//EB
validIds = geometry_->getValidDetIds(DetId::Ecal, EcalBarrel);//EB
else
return geometry_->getValidDetIds(DetId::Ecal, EcalEndcap);//EE
validIds = geometry_->getValidDetIds(DetId::Ecal, EcalEndcap);//EE
};

};
Expand Down
6 changes: 3 additions & 3 deletions TrackingTools/TrackAssociator/plugins/HODetIdAssociator.h
View file
Expand Up @@ -26,15 +26,15 @@ class HODetIdAssociator: public CaloDetIdAssociator{

HODetIdAssociator(const edm::ParameterSet& pSet):CaloDetIdAssociator(pSet){};

virtual const char* name() const { return "HO"; }
virtual const char* name() const override { return "HO"; }

protected:

const std::vector<DetId>& getValidDetIds(unsigned int subDectorIndex) const
void getValidDetIds(unsigned int subDectorIndex, std::vector<DetId>& validIds) const override
{
if ( subDectorIndex!=0 ) cms::Exception("FatalError") <<
"HO sub-dectors are all handle as one sub-system, but subDetectorIndex is not zero.\n";
return geometry_->getValidDetIds(DetId::Hcal, HcalOuter);
validIds = geometry_->getValidDetIds(DetId::Hcal, HcalOuter);
}
};
#endif
10 changes: 5 additions & 5 deletions TrackingTools/TrackAssociator/plugins/HcalDetIdAssociator.h
View file
Expand Up @@ -26,16 +26,16 @@ class HcalDetIdAssociator: public CaloDetIdAssociator{

HcalDetIdAssociator(const edm::ParameterSet& pSet):CaloDetIdAssociator(pSet){};

virtual const char* name() const { return "HCAL"; }
virtual const char* name() const override { return "HCAL"; }

protected:

virtual const unsigned int getNumberOfSubdetectors() const { return 2;}
virtual const std::vector<DetId>& getValidDetIds(unsigned int subDetectorIndex) const {
virtual const unsigned int getNumberOfSubdetectors() const override { return 2;}
void getValidDetIds(unsigned int subDetectorIndex, std::vector<DetId>& validIds) const {
if ( subDetectorIndex == 0 )
return geometry_->getValidDetIds(DetId::Hcal, HcalBarrel);//HB
validIds = geometry_->getValidDetIds(DetId::Hcal, HcalBarrel);//HB
else
return geometry_->getValidDetIds(DetId::Hcal, HcalEndcap);//HE
validIds = geometry_->getValidDetIds(DetId::Hcal, HcalEndcap);//HE
};
};
#endif
34 changes: 17 additions & 17 deletions TrackingTools/TrackAssociator/plugins/MuonDetIdAssociator.cc
View file
Expand Up @@ -47,8 +47,8 @@ GlobalPoint MuonDetIdAssociator::getPosition(const DetId& id) const {
return GlobalPoint(point.x(),point.y(),point.z());
}

const std::vector<DetId>& MuonDetIdAssociator::getValidDetIds(unsigned int subDectorIndex) const {
validIds_.clear();
void MuonDetIdAssociator::getValidDetIds(unsigned int subDectorIndex, std::vector<DetId>& validIds) const {
validIds.clear();
if (geometry_==0) throw cms::Exception("ConfigurationProblem") << "GlobalTrackingGeomtry is not set\n";
if (subDectorIndex!=0) throw cms::Exception("FatalError") <<
"Muon sub-dectors are all handle as one sub-system, but subDetectorIndex is not zero.\n";
Expand All @@ -59,14 +59,14 @@ const std::vector<DetId>& MuonDetIdAssociator::getValidDetIds(unsigned int subDe
for(std::vector<GeomDet*>::const_iterator it = geomDetsCSC.begin(); it != geomDetsCSC.end(); ++it)
if (CSCChamber* csc = dynamic_cast< CSCChamber*>(*it)) {
if ((! includeBadChambers_) && (cscbadchambers_->isInBadChamber(CSCDetId(csc->id())))) continue;
validIds_.push_back(csc->id());
validIds.push_back(csc->id());
}

// DT
if (! geometry_->slaveGeometry(DTChamberId()) ) throw cms::Exception("FatalError") << "Cannnot DTGeometry\n";
const std::vector<GeomDet*>& geomDetsDT = geometry_->slaveGeometry(DTChamberId())->dets();
for(std::vector<GeomDet*>::const_iterator it = geomDetsDT.begin(); it != geomDetsDT.end(); ++it)
if (DTChamber* dt = dynamic_cast< DTChamber*>(*it)) validIds_.push_back(dt->id());
if (DTChamber* dt = dynamic_cast< DTChamber*>(*it)) validIds.push_back(dt->id());

// RPC
if (! geometry_->slaveGeometry(RPCDetId()) ) throw cms::Exception("FatalError") << "Cannnot RPCGeometry\n";
Expand All @@ -75,10 +75,9 @@ const std::vector<DetId>& MuonDetIdAssociator::getValidDetIds(unsigned int subDe
if (RPCChamber* rpc = dynamic_cast< RPCChamber*>(*it)) {
std::vector< const RPCRoll*> rolls = (rpc->rolls());
for(std::vector<const RPCRoll*>::iterator r = rolls.begin(); r != rolls.end(); ++r)
validIds_.push_back((*r)->id().rawId());
validIds.push_back((*r)->id().rawId());
}

return validIds_;
}

bool MuonDetIdAssociator::insideElement(const GlobalPoint& point, const DetId& id) const {
Expand All @@ -88,9 +87,10 @@ bool MuonDetIdAssociator::insideElement(const GlobalPoint& point, const DetId& i
}

std::pair<DetIdAssociator::const_iterator,DetIdAssociator::const_iterator>
MuonDetIdAssociator::getDetIdPoints(const DetId& id) const
MuonDetIdAssociator::getDetIdPoints(const DetId& id, std::vector<GlobalPoint>& points) const
{
points_.clear();
points.clear();
points.reserve(8);
const GeomDet* geomDet = getGeomDet( id );

// the coners of muon detector elements are not stored and can be only calculated
Expand All @@ -105,16 +105,16 @@ MuonDetIdAssociator::getDetIdPoints(const DetId& id) const
// calculation from the edge, we will use exact geometry to get it right.

const Bounds* bounds = &(geometry_->idToDet(id)->surface().bounds());
points_.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,+bounds->length()/2,+bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,+bounds->length()/2,+bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,-bounds->length()/2,+bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,-bounds->length()/2,+bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,+bounds->length()/2,-bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,+bounds->length()/2,-bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,-bounds->length()/2,-bounds->thickness()/2)));
points_.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,-bounds->length()/2,-bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,+bounds->length()/2,+bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,+bounds->length()/2,+bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,-bounds->length()/2,+bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,-bounds->length()/2,+bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,+bounds->length()/2,-bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,+bounds->length()/2,-bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(+bounds->width()/2,-bounds->length()/2,-bounds->thickness()/2)));
points.push_back(geomDet->toGlobal(LocalPoint(-bounds->width()/2,-bounds->length()/2,-bounds->thickness()/2)));

return std::pair<const_iterator,const_iterator>(points_.begin(),points_.end());
return std::pair<const_iterator,const_iterator>(points.begin(),points.end());
}

void MuonDetIdAssociator::setGeometry(const DetIdAssociatorRecord& iRecord)
Expand Down
24 changes: 11 additions & 13 deletions TrackingTools/TrackAssociator/plugins/MuonDetIdAssociator.h
View file
Expand Up @@ -38,40 +38,38 @@ class MuonDetIdAssociator: public DetIdAssociator{
MuonDetIdAssociator(const edm::ParameterSet& pSet)
:DetIdAssociator(pSet.getParameter<int>("nPhi"),pSet.getParameter<int>("nEta"),pSet.getParameter<double>("etaBinSize")),geometry_(0),cscbadchambers_(0),includeBadChambers_(pSet.getParameter<bool>("includeBadChambers")){};

virtual void setGeometry(const GlobalTrackingGeometry* ptr){ geometry_ = ptr; }
virtual void setGeometry(const GlobalTrackingGeometry* ptr) { geometry_ = ptr; }

virtual void setGeometry(const DetIdAssociatorRecord& iRecord);
virtual void setGeometry(const DetIdAssociatorRecord& iRecord) override;

virtual void setCSCBadChambers(const CSCBadChambers* ptr){ cscbadchambers_ = ptr; }
virtual void setCSCBadChambers(const CSCBadChambers* ptr) { cscbadchambers_ = ptr; }

virtual void setConditions(const DetIdAssociatorRecord& iRecord){
virtual void setConditions(const DetIdAssociatorRecord& iRecord) override{
edm::ESHandle<CSCBadChambers> cscbadchambersH;
iRecord.getRecord<CSCBadChambersRcd>().get(cscbadchambersH);
setCSCBadChambers(cscbadchambersH.product());
};

virtual const GeomDet* getGeomDet( const DetId& id ) const;
virtual const GeomDet* getGeomDet( const DetId& id ) const override;

virtual const char* name() const { return "AllMuonDetectors"; }
virtual const char* name() const override { return "AllMuonDetectors"; }

protected:

virtual void check_setup() const;
virtual void check_setup() const override;

virtual GlobalPoint getPosition(const DetId& id) const;
virtual GlobalPoint getPosition(const DetId& id) const override;

virtual const std::vector<DetId>& getValidDetIds(unsigned int) const;
virtual void getValidDetIds(unsigned int, std::vector<DetId>&) const override;

virtual std::pair<const_iterator,const_iterator> getDetIdPoints(const DetId& id) const;
virtual std::pair<const_iterator,const_iterator> getDetIdPoints(const DetId& id, std::vector<GlobalPoint>& points) const override;

virtual bool insideElement(const GlobalPoint& point, const DetId& id) const;
virtual bool insideElement(const GlobalPoint& point, const DetId& id) const override;

const GlobalTrackingGeometry* geometry_;

const CSCBadChambers* cscbadchambers_;
bool includeBadChambers_;
mutable std::vector<GlobalPoint> points_;
mutable std::vector<DetId> validIds_;

};
#endif
6 changes: 3 additions & 3 deletions TrackingTools/TrackAssociator/plugins/PreshowerDetIdAssociator.h
View file
Expand Up @@ -26,12 +26,12 @@ class PreshowerDetIdAssociator: public CaloDetIdAssociator{

PreshowerDetIdAssociator(const edm::ParameterSet& pSet):CaloDetIdAssociator(pSet){};

virtual const char* name() const { return "Preshower"; }
virtual const char* name() const override { return "Preshower"; }
protected:

virtual const std::vector<DetId>& getValidDetIds(unsigned int subDetectorIndex) const {
virtual void getValidDetIds(unsigned int subDetectorIndex, std::vector<DetId>& validIds) const override {
if ( subDetectorIndex != 0 ) throw cms::Exception("FatalError") << "Preshower has only one sub-detector for geometry. Abort.";
return geometry_->getValidDetIds(DetId::Ecal, EcalPreshower);
validIds = geometry_->getValidDetIds(DetId::Ecal, EcalPreshower);
};

};
Expand Down
11 changes: 8 additions & 3 deletions TrackingTools/TrackAssociator/src/DetIdAssociator.cc
View file
Expand Up @@ -148,17 +148,21 @@ void DetIdAssociator::buildMap()
LogTrace("TrackAssociator")<<"building map for " << name() << "\n";
// clear the map
if (nEta_ <= 0 || nPhi_ <= 0) throw cms::Exception("FatalError") << "incorrect look-up map size. Cannot build such a map.";
std::vector<GlobalPoint> pointBuffer;
std::vector<DetId> detIdBuffer;
unsigned int numberOfSubDetectors = getNumberOfSubdetectors();
unsigned totalNumberOfElementsInTheContainer(0);
for ( unsigned int step = 0; step < 2; ++step){
unsigned int numberOfDetIdsOutsideEtaRange = 0;
unsigned int numberOfDetIdsActive = 0;
LogTrace("TrackAssociator")<< "Step: " << step;
for ( unsigned int subDetectorIndex = 0; subDetectorIndex < numberOfSubDetectors; ++subDetectorIndex ){
const std::vector<DetId>& validIds = getValidDetIds(subDetectorIndex);
getValidDetIds(subDetectorIndex, detIdBuffer);
// This std::move prevents modification
std::vector<DetId> const& validIds = std::move(detIdBuffer);
LogTrace("TrackAssociator")<< "Number of valid DetIds for subdetector: " << subDetectorIndex << " is " << validIds.size();
for (std::vector<DetId>::const_iterator id_itr = validIds.begin(); id_itr!=validIds.end(); id_itr++) {
std::pair<const_iterator,const_iterator> points = getDetIdPoints(*id_itr);
std::pair<const_iterator,const_iterator> points = getDetIdPoints(*id_itr, pointBuffer);
LogTrace("TrackAssociatorVerbose")<< "Found " << points.second-points.first << " global points to describe geometry of DetId: "
<< id_itr->rawId();
int etaMax(-1);
Expand Down Expand Up @@ -325,13 +329,14 @@ void DetIdAssociator::dumpMapContent(int ieta, int iphi) const
return;
}

std::vector<GlobalPoint> pointBuffer;
std::set<DetId> set;
fillSet(set,ieta,iphi%nPhi_);
LogTrace("TrackAssociator") << "Map content for cell (ieta,iphi): " << ieta << ", " << iphi%nPhi_;
for(std::set<DetId>::const_iterator itr = set.begin(); itr!=set.end(); itr++)
{
LogTrace("TrackAssociator") << "\tDetId " << itr->rawId() << ", geometry (x,y,z,rho,eta,phi):";
std::pair<const_iterator,const_iterator> points = getDetIdPoints(*itr);
std::pair<const_iterator,const_iterator> points = getDetIdPoints(*itr, pointBuffer);
for(std::vector<GlobalPoint>::const_iterator point = points.first; point != points.second; point++)
LogTrace("TrackAssociator") << "\t\t" << point->x() << ", " << point->y() << ", " << point->z() << ", "
<< point->perp() << ", " << point->eta() << ", " << point->phi();
Expand Down

0 comments on commit b18d59a

Please sign in to comment.