Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Thread safety -- Fix CastorObjects #890

Merged
merged 3 commits into from Sep 23, 2013
Merged

Thread safety -- Fix CastorObjects #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac6423c
Thread safety fix -- Make CastorObjects threadsafe.
ktf Sep 16, 2013
5a666fa
Merged refs/pull/847/head from repository cms-sw
apfeiffer1 Sep 19, 2013
abc26b8
added missing initializers (to nullprtr) from Valentin
apfeiffer1 Sep 19, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 20 additions & 4 deletions CondFormats/CastorObjects/interface/CastorElectronicsMap.h
View file
Open in desktop
Expand Up @@ -14,6 +14,9 @@ Modified for CASTOR by L. Mundim
#include <vector>
#include <algorithm>
#include <boost/cstdint.hpp>
#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__)
#include <atomic>
#endif

#include "DataFormats/DetId/interface/DetId.h"
#include "DataFormats/HcalDetId/interface/HcalDetId.h"
Expand All @@ -27,6 +30,16 @@ class CastorElectronicsMap {
CastorElectronicsMap();
~CastorElectronicsMap();

// swap function
void swap(CastorElectronicsMap& other);
// copy-ctor
CastorElectronicsMap(const CastorElectronicsMap& src); // copy assignment operator
CastorElectronicsMap& operator=(const CastorElectronicsMap& rhs);
// move constructor
#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__)
CastorElectronicsMap(CastorElectronicsMap&& other);
#endif

/// lookup the logical detid associated with the given electronics id
//return Null item if no such mapping
const DetId lookup(CastorElectronicsId fId) const;
Expand Down Expand Up @@ -86,10 +99,13 @@ class CastorElectronicsMap {

std::vector<PrecisionItem> mPItems;
std::vector<TriggerItem> mTItems;
mutable std::vector<const PrecisionItem*> mPItemsById;
mutable bool sortedByPId;
mutable std::vector<const TriggerItem*> mTItemsByTrigId;
mutable bool sortedByTId;
#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__)
mutable std::atomic<std::vector<const PrecisionItem*>*> mPItemsById;
mutable std::atomic<std::vector<const TriggerItem*>*> mTItemsByTrigId;
#else
mutable std::vector<const PrecisionItem*>* mPItemsById;
mutable std::vector<const TriggerItem*>* mTItemsByTrigId;
#endif
};

#endif
92 changes: 65 additions & 27 deletions CondFormats/CastorObjects/src/CastorElectronicsMap.cc
View file
Open in desktop
Expand Up @@ -17,25 +17,56 @@ Adapted for CASTOR by L. Mundim
CastorElectronicsMap::CastorElectronicsMap() :
mPItems(CastorElectronicsId::maxLinearIndex+1),
mTItems(CastorElectronicsId::maxLinearIndex+1),
sortedByPId(false),
sortedByTId(false)
mPItemsById(nullptr), mTItemsByTrigId(nullptr)
{}

namespace castor_impl {
class LessById {public: bool operator () (const CastorElectronicsMap::PrecisionItem* a, const CastorElectronicsMap::PrecisionItem* b) {return a->mId < b->mId;}};
class LessByTrigId {public: bool operator () (const CastorElectronicsMap::TriggerItem* a, const CastorElectronicsMap::TriggerItem* b) {return a->mTrigId < b->mTrigId;}};
}

CastorElectronicsMap::~CastorElectronicsMap(){}
CastorElectronicsMap::~CastorElectronicsMap() {
if (mPItemsById) {
delete mPItemsById;
mPItemsById = nullptr;
}
if (mTItemsByTrigId) {
delete mTItemsByTrigId;
mTItemsByTrigId = nullptr;
}
}
// copy-ctor
CastorElectronicsMap::CastorElectronicsMap(const CastorElectronicsMap& src)
: mPItems(src.mPItems), mTItems(src.mTItems),
mPItemsById(nullptr), mTItemsByTrigId(nullptr) {}
// copy assignment operator
CastorElectronicsMap&
CastorElectronicsMap::operator=(const CastorElectronicsMap& rhs) {
CastorElectronicsMap temp(rhs);
temp.swap(*this);
return *this;
}
// public swap function
void CastorElectronicsMap::swap(CastorElectronicsMap& other) {
std::swap(mPItems, other.mPItems);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this with @apfeiffer1 yesterday. Are you sure the two vectors are not related? How do you guarantee that both updates happen atomically?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are following the C++11 standard's concept of thead safety which is it is safe to have multiple threads call membe function of an object only if all those functions are const. Swap is not const and therefore we are not expecting it to be safe to call concurrently.

std::swap(mTItems, other.mTItems);
other.mTItemsByTrigId.exchange(mTItemsByTrigId.exchange(other.mTItemsByTrigId));
other.mPItemsById.exchange(mPItemsById.exchange(other.mPItemsById));
}
// move constructor
CastorElectronicsMap::CastorElectronicsMap(CastorElectronicsMap&& other)
: CastorElectronicsMap() {
other.swap(*this);
}

const CastorElectronicsMap::PrecisionItem* CastorElectronicsMap::findById (unsigned long fId) const {
PrecisionItem target (fId, 0);
std::vector<const CastorElectronicsMap::PrecisionItem*>::const_iterator item;

if (!sortedByPId) sortById();
sortById();

item = std::lower_bound (mPItemsById.begin(), mPItemsById.end(), &target, castor_impl::LessById());
if (item == mPItemsById.end() || (*item)->mId != fId)
item = std::lower_bound ((*mPItemsById).begin(), (*mPItemsById).end(), &target, castor_impl::LessById());
if (item == (*mPItemsById).end() || (*item)->mId != fId)
// throw cms::Exception ("Conditions not found") << "Unavailable Electronics map for cell " << fId;
return 0;
return *item;
Expand All @@ -62,10 +93,10 @@ const CastorElectronicsMap::TriggerItem* CastorElectronicsMap::findByTrigId (uns
TriggerItem target (fTrigId,0);
std::vector<const CastorElectronicsMap::TriggerItem*>::const_iterator item;

if (!sortedByTId) sortByTriggerId();
sortByTriggerId();

item = std::lower_bound (mTItemsByTrigId.begin(), mTItemsByTrigId.end(), &target, castor_impl::LessByTrigId());
if (item == mTItemsByTrigId.end() || (*item)->mTrigId != fTrigId)
item = std::lower_bound ((*mTItemsByTrigId).begin(), (*mTItemsByTrigId).end(), &target, castor_impl::LessByTrigId());
if (item == (*mTItemsByTrigId).end() || (*item)->mTrigId != fTrigId)
// throw cms::Exception ("Conditions not found") << "Unavailable Electronics map for cell " << fId;
return 0;
return *item;
Expand Down Expand Up @@ -158,7 +189,6 @@ std::vector <HcalTrigTowerDetId> CastorElectronicsMap::allTriggerId () const {

bool CastorElectronicsMap::mapEId2tId (CastorElectronicsId fElectronicsId, HcalTrigTowerDetId fTriggerId) {
TriggerItem& item = mTItems[fElectronicsId.linearIndex()];
sortedByTId=false;
if (item.mElId==0) item.mElId=fElectronicsId.rawId();
if (item.mTrigId == 0) {
item.mTrigId = fTriggerId.rawId (); // just cast avoiding long machinery
Expand All @@ -174,7 +204,6 @@ bool CastorElectronicsMap::mapEId2tId (CastorElectronicsId fElectronicsId, HcalT
bool CastorElectronicsMap::mapEId2chId (CastorElectronicsId fElectronicsId, DetId fId) {
PrecisionItem& item = mPItems[fElectronicsId.linearIndex()];

sortedByPId=false;
if (item.mElId==0) item.mElId=fElectronicsId.rawId();
if (item.mId == 0) {
item.mId = fId.rawId ();
Expand All @@ -188,25 +217,34 @@ bool CastorElectronicsMap::mapEId2chId (CastorElectronicsId fElectronicsId, DetI
}

void CastorElectronicsMap::sortById () const {
if (!sortedByPId) {
mPItemsById.clear();
for (std::vector<PrecisionItem>::const_iterator i=mPItems.begin(); i!=mPItems.end(); ++i) {
if (i->mElId) mPItemsById.push_back(&(*i));
}

std::sort (mPItemsById.begin(), mPItemsById.end(), castor_impl::LessById ());
sortedByPId=true;
if (!mPItemsById) {
auto ptr = new std::vector<const PrecisionItem*>;
for (auto i=mPItems.begin(); i!=mPItems.end(); ++i) {
if (i->mElId) (*ptr).push_back(&(*i));
}
std::sort ((*ptr).begin(), (*ptr).end(), castor_impl::LessById ());
//atomically try to swap this to become mPItemsById
std::vector<const PrecisionItem*>* expect = nullptr;
bool exchanged = mPItemsById.compare_exchange_strong(expect, ptr);
if(!exchanged) {
delete ptr;
}
}
}

void CastorElectronicsMap::sortByTriggerId () const {
if (!sortedByTId) {
mTItemsByTrigId.clear();
for (std::vector<TriggerItem>::const_iterator i=mTItems.begin(); i!=mTItems.end(); ++i) {
if (i->mElId) mTItemsByTrigId.push_back(&(*i));
}

std::sort (mTItemsByTrigId.begin(), mTItemsByTrigId.end(), castor_impl::LessByTrigId ());
sortedByTId=true;
if (!mTItemsByTrigId) {
auto ptr = new std::vector<const TriggerItem*>;
for (auto i=mTItems.begin(); i!=mTItems.end(); ++i) {
if (i->mElId) (*ptr).push_back(&(*i));
}

std::sort ((*ptr).begin(), (*ptr).end(), castor_impl::LessByTrigId ());
//atomically try to swap this to become mTItemsByTrigId
std::vector<const TriggerItem*>* expect = nullptr;
bool exchanged = mTItemsByTrigId.compare_exchange_strong(expect, ptr);
if(!exchanged) {
delete ptr;
}
}
}
2 changes: 0 additions & 2 deletions CondFormats/CastorObjects/src/classes_def.xml
View file
Open in desktop
Expand Up @@ -25,9 +25,7 @@

<class name="CastorElectronicsMap">
<field name="mPItemsById" transient="true"/>
<field name="sortedByPId" transient="true"/>
<field name="mTItemsByTrigId" transient="true"/>
<field name="sortedByTId" transient="true"/>
</class>
<class name="CastorElectronicsMap::PrecisionItem"/>
<class name="CastorElectronicsMap::TriggerItem"/>
Expand Down