From 41ef2c2d0fcd73965a27424951d60822a1e4af14 Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Sat, 7 Nov 2015 20:01:01 +0100 Subject: [PATCH 01/10] thread safe DetSet --- .../Common/interface/DetSetVectorNew.h | 261 ++++++++++++++++-- DataFormats/Common/src/DetSetVectorNew.cc | 7 + DataFormats/Common/src/classes_def.xml | 3 +- DataFormats/Common/test/BuildFile.xml | 3 + DataFormats/Common/test/DetSetNew_t.cpp | 91 +++++- 5 files changed, 332 insertions(+), 33 deletions(-) diff --git a/DataFormats/Common/interface/DetSetVectorNew.h b/DataFormats/Common/interface/DetSetVectorNew.h index 42176b4d0ba47..2b9ad5a5afe55 100644 --- a/DataFormats/Common/interface/DetSetVectorNew.h +++ b/DataFormats/Common/interface/DetSetVectorNew.h @@ -6,6 +6,7 @@ #include "DataFormats/Common/interface/DetSetNew.h" #include "DataFormats/Common/interface/traits.h" + #include #include #include @@ -14,6 +15,16 @@ #include "FWCore/Utilities/interface/Exception.h" #include "FWCore/Utilities/interface/GCC11Compatibility.h" +#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__) +#define USE_ATOMIC +// #warning using atomic +#endif + +#ifdef USE_ATOMIC +#include +#include +#include +#endif #include #include @@ -30,20 +41,41 @@ namespace edmNew { namespace dslv { template< typename T> class LazyGetter; + } /* transient component of DetSetVector - * for pure conviniency of dictioanary declaration + * for pure conviniency of dictionary declaration */ namespace dstvdetails { + + void errorFilling(); + void notSafe(); + void errorIdExists(det_id_type iid); + void throw_range(det_id_type iid); + struct DetSetVectorTrans { DetSetVectorTrans(): filling(false){} - bool filling; +#ifndef USE_ATOMIC + mutable bool filling; + private: + DetSetVectorTrans& operator=(const DetSetVectorTrans&){return *this;} + DetSetVectorTrans(const DetSetVectorTrans&){} + public: +#else + DetSetVectorTrans& operator=(const DetSetVectorTrans&) = delete; + DetSetVectorTrans(const DetSetVectorTrans&) = delete; + DetSetVectorTrans(DetSetVectorTrans&&) = default; + DetSetVectorTrans& operator=(DetSetVectorTrans&&) = default; + mutable std::atomic filling; +#endif boost::any getter; void swap(DetSetVectorTrans& rh) { - std::swap(filling,rh.filling); + // better no one is filling... + assert(filling==false); assert(rh.filling==false); + // std::swap(filling,rh.filling); std::swap(getter,rh.getter); } @@ -52,22 +84,45 @@ namespace edmNew { struct Item { Item(id_type i=0, int io=-1, size_type is=0) : id(i), offset(io), size(is){} - bool isValid() const { return offset>=0;} id_type id; +#ifdef USE_ATOMIC + // Item(Item&&)=default; Item & operator=(Item&& rh)=default; + Item(Item const & rh) noexcept : + id(rh.id),offset(rh.offset.load(std::memory_order_acquire)),size(rh.size) { + } + Item & operator=(Item const & rh) noexcept { + id=rh.id;offset=rh.offset.load(std::memory_order_acquire);size=rh.size; return *this; + } + Item(Item&& rh) noexcept : + id(std::move(rh.id)),offset(rh.offset.load(std::memory_order_acquire)),size(std::move(rh.size)) { + } + Item & operator=(Item&& rh) noexcept { + id=std::move(rh.id);offset=rh.offset.load(std::memory_order_acquire);size=std::move(rh.size); return *this; + } + std::atomic offset; +#else int offset; +#endif size_type size; + + bool isValid() const { return offset>=0;} bool operator<(Item const &rh) const { return id::id_type id_type; typedef typename DetSetVector::size_type size_type; +#ifdef USE_ATOMIC + static DetSetVector::Item & dummy() { + static DetSetVector::Item d; return d; + } FastFiller(DetSetVector & iv, id_type id, bool isaveEmpty=false) : - v(iv), item(v.push_back(id)), saveEmpty(isaveEmpty) { - if (v.filling) dstvdetails::errorFilling(); - v.filling=true; + v(iv), item(v.ready()? v.push_back(id): dummy()),saveEmpty(isaveEmpty) { + if (v.onDemand()) dstvdetails::notSafe(); } + FastFiller(DetSetVector & iv, typename DetSetVector::Item & it, bool isaveEmpty=false) : v(iv), item(it), saveEmpty(isaveEmpty) { - if (v.filling) dstvdetails::errorFilling(); - v.filling=true; + if (v.onDemand()) dstvdetails::notSafe(); + if(v.ready()) item.offset = int(v.m_data.size()); + } ~FastFiller() { if (!saveEmpty && item.size==0) { v.pop_back(item.id); } - v.filling=false; + assert(v.filling==true); + v.filling.store(false,std::memory_order_release); + } +#endif + void abort() { v.pop_back(item.id); saveEmpty=true; // avoid mess in destructor @@ -187,7 +251,7 @@ namespace edmNew { return v.m_data[item.offset+i]; } DataIter begin() { return v.m_data.begin()+ item.offset;} - DataIter end() { return v.m_data.end();} + DataIter end() { return begin()+size();} void push_back(data_type const & d) { checkCapacityExausted(); @@ -212,13 +276,106 @@ namespace edmNew { typename DetSetVector::Item & item; bool saveEmpty; }; + + /* fill on demand a given DetSet + */ + class TSFastFiller { + public: + typedef typename DetSetVector::data_type value_type; + typedef typename DetSetVector::id_type key_type; + typedef typename DetSetVector::id_type id_type; + typedef typename DetSetVector::size_type size_type; + +#ifdef USE_ATOMIC + static DetSetVector::Item & dummy() { + static DetSetVector::Item d; return d; + } + TSFastFiller(DetSetVector & iv, id_type id) : + v(iv), item(v.ready()? v.push_back(id): dummy()) { assert(v.filling==true); v.filling = false;} + + TSFastFiller(DetSetVector & iv, typename DetSetVector::Item & it) : + v(iv), item(it) { + + } + ~TSFastFiller() { + bool expected=false; + while (!v.filling.compare_exchange_weak(expected,true,std::memory_order_acq_rel)) { expected=false; nanosleep(0,0);} + int offset = v.m_data.size(); + if (v.m_data.capacity() lv; + DetSetVector & v; + typename DetSetVector::Item & item; + }; + + + friend class FastFiller; + friend class TSFastFiller; + friend class edmNew::DetSet; class FindForDetSetVector : public std::binary_function&, unsigned int, const T*> { public: typedef FindForDetSetVector self; typename self::result_type operator()(typename self::first_argument_type iContainer, typename self::second_argument_type iIndex) { - return &(iContainer.m_data[iIndex]); +#ifdef USE_ATOMIC + bool expected=false; + while (!iContainer.filling.compare_exchange_weak(expected,true,std::memory_order_acq_rel)) { expected=false; nanosleep(0,0);} +#else + iContainer.filling = true; +#endif + typename self::result_type item = &(iContainer.m_data[iIndex]); + assert(iContainer.filling==true); + iContainer.filling = false; + return item; } }; friend class FindForDetSetVector; @@ -233,10 +390,24 @@ namespace edmNew { ~DetSetVector() { // delete content if T is pointer... } - + +#ifdef USE_ATOMIC + // default or delete is the same... + DetSetVector& operator=(const DetSetVector&) = delete; + DetSetVector(const DetSetVector&) = delete; + DetSetVector(DetSetVector&&) = default; + DetSetVector& operator=(DetSetVector&&) = default; +#else + private: + DetSetVector& operator=(const DetSetVector&){return *this;} + DetSetVector(const DetSetVector&){} + public: +#endif bool onDemand() const { return !getter.empty();} + + void swap(DetSetVector & rh) { DetSetVectorTrans::swap(rh); std::swap(m_subdetId,rh.m_subdetId); @@ -256,6 +427,7 @@ namespace edmNew { void shrink_to_fit() { #ifndef CMS_NOCXX11 + clean(); m_ids.shrink_to_fit(); m_data.shrink_to_fit(); #endif @@ -265,6 +437,12 @@ namespace edmNew { m_ids.resize(isize); m_data.resize(dsize); } + + void clean() { +#ifndef CMS_NOCXX11 + m_ids.erase(std::remove_if(m_ids.begin(),m_ids.end(),[](Item const& m){ return 0==m.size;}),m_ids.end()); +#endif + } // FIXME not sure what the best way to add one cell to cont DetSet insert(id_type iid, data_type const * idata, size_type isize) { @@ -304,7 +482,11 @@ namespace edmNew { m_ids.end(), it); if (p!=m_ids.end() && !(it<*p)) dstvdetails::errorIdExists(iid); +#ifndef CMS_NOCXX11 + return *m_ids.insert(p,std::move(it)); +#else return *m_ids.insert(p,it); +#endif } @@ -431,7 +613,6 @@ namespace edmNew { // subdetector id (as returned by DetId::subdetId()) int m_subdetId; - // Workaround for ROOT 6 bug. // ROOT6 has a problem with this IdContainer typedef //IdContainer m_ids; @@ -445,11 +626,10 @@ namespace edmNew { class LazyGetter { public: virtual ~LazyGetter() {} - virtual void fill(typename DetSetVector::FastFiller&) = 0; + virtual void fill(typename DetSetVector::TSFastFiller&) = 0; }; } - template inline DetSetVector::DetSetVector(std::shared_ptr iGetter, @@ -470,27 +650,46 @@ namespace edmNew { } template - inline void DetSetVector::updateImpl(Item & item) { + inline void DetSetVector::updateImpl(Item & item) { // no getter or already updated +/* if (getter.empty()) assert(item.offset>=0); if (item.offset!=-1 || getter.empty() ) return; item.offset = int(m_data.size()); FastFiller ff(*this,item,true); (*boost::any_cast >(&getter))->fill(ff); +*/ + if (getter.empty()) { assert(item.offset>=0); return;} +#ifdef USE_ATOMIC + int expected = -1; + if (item.offset.compare_exchange_strong(expected,-2,std::memory_order_acq_rel)) { + assert(item.offset==-2); + { + TSFastFiller ff(*this,item); + (*boost::any_cast >(&getter))->fill(ff); + } + assert(item.offset>=0); + } +#endif } - - template inline void DetSet::set(DetSetVector const & icont, typename Container::Item const & item, bool update) { - if (update) { - icont.update(item); - assert(item.offset>=0); - } - m_id=item.id; +#ifdef USE_ATOMIC + // if an item is being updated we wait + if (update)icont.update(item); + while(item.offset.load(std::memory_order_acquire)<-1) nanosleep(0,0); + + bool expected=false; + while (!icont.filling.compare_exchange_weak(expected,true,std::memory_order_acq_rel)) { expected=false; nanosleep(0,0);} +#endif m_data=&icont.data(); +#ifdef USE_ATOMIC + icont.filling.store(false,std::memory_order_release); +#endif + m_id=item.id; m_offset = item.offset; m_size=item.size; } @@ -559,5 +758,9 @@ namespace edm { }; } +#ifdef USE_ATOMIC +#undef USE_ATOMIC +#endif + #endif diff --git a/DataFormats/Common/src/DetSetVectorNew.cc b/DataFormats/Common/src/DetSetVectorNew.cc index cd7cf127c3180..99730d22a9318 100644 --- a/DataFormats/Common/src/DetSetVectorNew.cc +++ b/DataFormats/Common/src/DetSetVectorNew.cc @@ -7,6 +7,13 @@ namespace edmNew { throw edm::Exception(edm::errors::LogicError,"Instantiating a second DetSetVector::FastFiller") << "only one DetSetVector::FastFiller can be active at a given time!"; } + + void notSafe() { + throw edm::Exception(edm::errors::LogicError,"Instantiating a DetSetVector::FastFiller for OnDemand") + << "use DetSetVector::TSFastFiller"; + } + + void errorIdExists(det_id_type iid) { throw edm::Exception(edm::errors::InvalidReference) << "DetSetVector::inserv called with index already in collection;\n" diff --git a/DataFormats/Common/src/classes_def.xml b/DataFormats/Common/src/classes_def.xml index 8231b43e76ae8..a0b3fbd62e9c9 100644 --- a/DataFormats/Common/src/classes_def.xml +++ b/DataFormats/Common/src/classes_def.xml @@ -96,7 +96,8 @@ - + + diff --git a/DataFormats/Common/test/BuildFile.xml b/DataFormats/Common/test/BuildFile.xml index 4352b97bdad68..7196a58721860 100644 --- a/DataFormats/Common/test/BuildFile.xml +++ b/DataFormats/Common/test/BuildFile.xml @@ -25,6 +25,9 @@ + + + diff --git a/DataFormats/Common/test/DetSetNew_t.cpp b/DataFormats/Common/test/DetSetNew_t.cpp index 7fc5d3d4335a9..eb1a1a0524a28 100644 --- a/DataFormats/Common/test/DetSetNew_t.cpp +++ b/DataFormats/Common/test/DetSetNew_t.cpp @@ -37,7 +37,7 @@ typedef edmNew::DetSetVector DSTV; typedef edmNew::DetSet DST; typedef edmNew::det_id_type det_id_type; typedef DSTV::FastFiller FF; - +typedef DSTV::TSFastFiller TSFF; class TestDetSet: public CppUnit::TestFixture { @@ -45,6 +45,7 @@ class TestDetSet: public CppUnit::TestFixture CPPUNIT_TEST(default_ctor); CPPUNIT_TEST(inserting); CPPUNIT_TEST(filling); + CPPUNIT_TEST(fillingTS); CPPUNIT_TEST(iterator); CPPUNIT_TEST(algorithm); CPPUNIT_TEST(onDemand); @@ -61,6 +62,7 @@ class TestDetSet: public CppUnit::TestFixture void default_ctor(); void inserting(); void filling(); + void fillingTS(); void iterator(); void algorithm(); void onDemand(); @@ -79,6 +81,10 @@ TestDetSet::TestDetSet() : sv(10){ } void TestDetSet::default_ctor() { +#ifdef CMS_NOCXX11 + CPPUNIT_ASSERT(false && "CMS_NOCXX11"); +#endif + DSTV detsets(2); CPPUNIT_ASSERT(!detsets.onDemand()); @@ -148,6 +154,7 @@ void TestDetSet::inserting() { } } + void TestDetSet::filling() { DSTV detsets(2); @@ -217,6 +224,12 @@ void TestDetSet::filling() { } CPPUNIT_ASSERT(detsets.size()==5); CPPUNIT_ASSERT(!detsets.exists(31)); + { FF ff1(detsets, 32,true); } + CPPUNIT_ASSERT(detsets.size()==6); + detsets.clean(); + CPPUNIT_ASSERT(detsets.size()==4); + CPPUNIT_ASSERT(!detsets.exists(30)); + CPPUNIT_ASSERT(!detsets.exists(32)); // test error conditions try { @@ -236,6 +249,78 @@ void TestDetSet::filling() { } } + +void TestDetSet::fillingTS() { + + DSTV detsets(2); + unsigned int ntot=0; + for (unsigned int n=1;n<5;++n) { + unsigned int id=20+n; + { + TSFF ff(detsets, id); + CPPUNIT_ASSERT(detsets.size()==n); + CPPUNIT_ASSERT(detsets.dataSize()==ntot); + CPPUNIT_ASSERT(ff.item.size==0); + CPPUNIT_ASSERT(ff.id()==id); + ntot+=1; + ff.push_back(3.14); + CPPUNIT_ASSERT(detsets.dataSize()==ntot-1); + CPPUNIT_ASSERT(ff.item.size==0); + CPPUNIT_ASSERT(ff.size()==1); + ntot+=n-1; + ff.resize(n); + CPPUNIT_ASSERT(ff.size()==n); + std::copy(sv.begin(),sv.begin()+n,ff.begin()); + } + CPPUNIT_ASSERT(detsets.size()==n); + CPPUNIT_ASSERT(detsets.dataSize()==ntot); + + std::vector v1(n); + std::vector v2(n); + std::copy(detsets.m_data.begin()+ntot-n,detsets.m_data.begin()+ntot,v2.begin()); + std::copy(sv.begin(),sv.begin()+n,v1.begin()); + CPPUNIT_ASSERT(v1==v2); + } + + // test abort and empty + { TSFF ff1(detsets, 30); CPPUNIT_ASSERT(detsets.exists(30));} + CPPUNIT_ASSERT(detsets.size()==5); + CPPUNIT_ASSERT(detsets.exists(30)); + detsets.clean(); + CPPUNIT_ASSERT(detsets.size()==4); + CPPUNIT_ASSERT(!detsets.exists(30)); + + unsigned int cs = detsets.dataSize(); + { + TSFF ff1(detsets, 31); + ff1.resize(4); + ff1.abort(); + } + CPPUNIT_ASSERT(detsets.size()==5); + CPPUNIT_ASSERT(detsets.dataSize()==cs); + CPPUNIT_ASSERT(detsets.exists(31)); + detsets.clean(); + CPPUNIT_ASSERT(detsets.size()==4); + CPPUNIT_ASSERT(!detsets.exists(31)); + + // test error conditions + try { + TSFF ff1(detsets, 22); + CPPUNIT_ASSERT(" fast filler did not throw"==0); + } + catch (edm::Exception const & err) { + CPPUNIT_ASSERT(err.categoryCode()==edm::errors::InvalidReference); + } + + try { + TSFF ff1(detsets, 44); + TSFF ff2(detsets, 45); + CPPUNIT_ASSERT(" fast filler did not throw"==0); + } catch (edm::Exception const &err) { + CPPUNIT_ASSERT(err.categoryCode()==edm::errors::LogicError); + } +} + namespace { struct VerifyIter{ VerifyIter(TestDetSet * itest, unsigned int in=1, int iincr=1): @@ -260,9 +345,9 @@ namespace { struct Getter final : public DSTV::Getter { Getter(TestDetSet * itest):ntot(0), test(*itest){} - void fill(FF& ff) override { + void fill(TSFF& ff) override { try { - int n=ff.id()-20; + int n=ff.id()-20; CPPUNIT_ASSERT(n>0); ff.resize(n); std::copy(test.sv.begin(),test.sv.begin()+n,ff.begin()); From a0ec998ab26ebc2f3bf351f5a1208840235568a4 Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Sun, 8 Nov 2015 13:59:54 +0100 Subject: [PATCH 02/10] fix copies by value... --- .../src/SiPixelTrackResidualSource.cc | 8 ++++---- RecoJets/JetAnalyzers/src/myFilter.cc | 4 ++-- RecoJets/JetAnalyzers/src/myJetAna.cc | 4 ++-- .../interface/OldThreeThresholdAlgorithm.h | 6 +++--- .../interface/StripClusterizerAlgorithm.h | 12 ++++++------ .../interface/ThreeThresholdAlgorithm.h | 12 ++++++------ .../plugins/ClustersFromRawProducer.cc | 6 +++--- .../src/OldThreeThresholdAlgorithm.cc | 6 +++--- .../src/ThreeThresholdAlgorithm.cc | 6 +++--- .../test/ClusterRefinerTagMCmerged.cc | 2 +- .../SiStripClusterizer/test/ClusterizerUnitTester.cc | 4 ++-- .../test/StripByStripTestDriver.cc | 2 +- .../src/ClusterChargeMasker.cc | 2 +- .../src/HLTTrackClusterRemoverNew.cc | 2 +- 14 files changed, 38 insertions(+), 38 deletions(-) diff --git a/DQM/SiPixelMonitorTrack/src/SiPixelTrackResidualSource.cc b/DQM/SiPixelMonitorTrack/src/SiPixelTrackResidualSource.cc index 864687343a65d..d0120265c2e28 100644 --- a/DQM/SiPixelMonitorTrack/src/SiPixelTrackResidualSource.cc +++ b/DQM/SiPixelMonitorTrack/src/SiPixelTrackResidualSource.cc @@ -863,26 +863,26 @@ void SiPixelTrackResidualSource::analyze(const edm::Event& iEvent, const edm::Ev edm::Handle > trajCollectionHandle; //iEvent.getByLabel(tracksrc_,trajCollectionHandle); iEvent.getByToken ( tracksrcToken_, trajCollectionHandle ); - const std::vector trajColl = *(trajCollectionHandle.product()); + auto const & trajColl = *(trajCollectionHandle.product()); //get tracks edm::Handle > trackCollectionHandle; //iEvent.getByLabel(tracksrc_,trackCollectionHandle); iEvent.getByToken( trackToken_, trackCollectionHandle ); - const std::vector trackColl = *(trackCollectionHandle.product()); + auto const & trackColl = *(trackCollectionHandle.product()); //get the map edm::Handle match; //iEvent.getByLabel(tracksrc_,match); iEvent.getByToken( trackAssociationToken_, match); - const TrajTrackAssociationCollection ttac = *(match.product()); + auto const & ttac = *(match.product()); // get clusters edm::Handle< edmNew::DetSetVector > clusterColl; //iEvent.getByLabel( clustersrc_, clusterColl ); iEvent.getByToken( clustersrcToken_, clusterColl ); - const edmNew::DetSetVector clustColl = *(clusterColl.product()); + auto const & clustColl = *(clusterColl.product()); if(debug_){ std::cout << "Trajectories\t : " << trajColl.size() << std::endl; diff --git a/RecoJets/JetAnalyzers/src/myFilter.cc b/RecoJets/JetAnalyzers/src/myFilter.cc index d0492193a4c0a..4fc81c856b693 100644 --- a/RecoJets/JetAnalyzers/src/myFilter.cc +++ b/RecoJets/JetAnalyzers/src/myFilter.cc @@ -678,7 +678,7 @@ myFilter::filter(edm::Event& evt, edm::EventSetup const& es) { edm::Handle< edmNew::DetSetVector > hClusterColl; evt.getByLabel("siPixelClusters", hClusterColl); - const edmNew::DetSetVector clustColl = *(hClusterColl.product()); + auto const & clustColl = *(hClusterColl.product()); // nCl = clustColl.size(); /*** @@ -696,7 +696,7 @@ myFilter::filter(edm::Event& evt, edm::EventSetup const& es) { // evt.getByLabel("ctfWithMaterialTracks", trackCollection); evt.getByLabel("generalTracks", trackCollection); - const reco::TrackCollection tC = *(trackCollection.product()); + auto const & tC = *(trackCollection.product()); // std::cout << "FIL: Reconstructed "<< tC.size() << " tracks" << std::endl ; if (tC.size() > 3) filter_NTrks = true; diff --git a/RecoJets/JetAnalyzers/src/myJetAna.cc b/RecoJets/JetAnalyzers/src/myJetAna.cc index 51135ba84ada3..c36312cfb8346 100644 --- a/RecoJets/JetAnalyzers/src/myJetAna.cc +++ b/RecoJets/JetAnalyzers/src/myJetAna.cc @@ -1048,11 +1048,11 @@ void myJetAna::analyze( const edm::Event& evt, const edm::EventSetup& es ) { // ******************************** edm::Handle< edmNew::DetSetVector > hClusterColl; evt.getByLabel("siPixelClusters", hClusterColl); - const edmNew::DetSetVector clustColl = *(hClusterColl.product()); + auto const & clustColl = *(hClusterColl.product()); edm::Handle trackCollection; evt.getByLabel("generalTracks", trackCollection); - const reco::TrackCollection tC = *(trackCollection.product()); + auto const & tC = *(trackCollection.product()); // ************************** diff --git a/RecoLocalTracker/SiStripClusterizer/interface/OldThreeThresholdAlgorithm.h b/RecoLocalTracker/SiStripClusterizer/interface/OldThreeThresholdAlgorithm.h index 77b718687afb6..f74e3029fd2b1 100644 --- a/RecoLocalTracker/SiStripClusterizer/interface/OldThreeThresholdAlgorithm.h +++ b/RecoLocalTracker/SiStripClusterizer/interface/OldThreeThresholdAlgorithm.h @@ -32,8 +32,8 @@ class OldThreeThresholdAlgorithm final : public StripClusterizerAlgorithm { public: // void setSiStripNoiseService( SiStripNoiseService* in ){ SiStripNoiseService_=in;} - void clusterizeDetUnit(const edm::DetSet & digis, edmNew::DetSetVector::FastFiller & output); - void clusterizeDetUnit(const edmNew::DetSet & digis, edmNew::DetSetVector::FastFiller & output); + void clusterizeDetUnit(const edm::DetSet & digis, edmNew::DetSetVector::TSFastFiller & output); + void clusterizeDetUnit(const edmNew::DetSet & digis, edmNew::DetSetVector::TSFastFiller & output); void initialize(const edm::EventSetup&); @@ -55,7 +55,7 @@ class OldThreeThresholdAlgorithm final : public StripClusterizerAlgorithm { // SiStripNoiseService* SiStripNoiseService_; template - void clusterizeDetUnit_(const InputDetSet & digis, edmNew::DetSetVector::FastFiller & output); + void clusterizeDetUnit_(const InputDetSet & digis, edmNew::DetSetVector::TSFastFiller & output); float theChannelThreshold; float theSeedThreshold; diff --git a/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h b/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h index 3c99480119a60..4db2a45666961 100644 --- a/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h +++ b/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h @@ -28,8 +28,8 @@ class StripClusterizerAlgorithm { typedef edmNew::DetSetVector output_t; void clusterize(const edm::DetSetVector &, output_t &); void clusterize(const edmNew::DetSetVector &, output_t &); - virtual void clusterizeDetUnit(const edm::DetSet &, output_t::FastFiller &) = 0; - virtual void clusterizeDetUnit(const edmNew::DetSet &, output_t::FastFiller &) = 0; + virtual void clusterizeDetUnit(const edm::DetSet &, output_t::TSFastFiller &) = 0; + virtual void clusterizeDetUnit(const edmNew::DetSet &, output_t::TSFastFiller &) = 0; //HLT stripByStrip interface virtual bool stripByStripBegin(uint32_t id) = 0; @@ -38,9 +38,9 @@ class StripClusterizerAlgorithm { virtual void stripByStripAdd(uint16_t strip, uint8_t adc, std::vector& out) {} virtual void stripByStripEnd(std::vector& out) {} - virtual void addFed(sistrip::FEDZSChannelUnpacker & unpacker, uint16_t ipair, output_t::FastFiller & out) {} - virtual void stripByStripAdd(uint16_t strip, uint8_t adc, output_t::FastFiller & out) {} - virtual void stripByStripEnd(output_t::FastFiller & out) {} + virtual void addFed(sistrip::FEDZSChannelUnpacker & unpacker, uint16_t ipair, output_t::TSFastFiller & out) {} + virtual void stripByStripAdd(uint16_t strip, uint8_t adc, output_t::TSFastFiller & out) {} + virtual void stripByStripEnd(output_t::TSFastFiller & out) {} virtual void cleanState(){} @@ -71,7 +71,7 @@ class StripClusterizerAlgorithm { template void clusterize_(const T& input, output_t& output) { for(typename T::const_iterator it = input.begin(); it!=input.end(); it++) { - output_t::FastFiller ff(output, it->detId()); + output_t::TSFastFiller ff(output, it->detId()); clusterizeDetUnit(*it, ff); if(ff.empty()) ff.abort(); } diff --git a/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h b/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h index 5865a8bf6a866..f2f79af990abe 100644 --- a/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h +++ b/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h @@ -9,8 +9,8 @@ class ThreeThresholdAlgorithm final : public StripClusterizerAlgorithm { public: - void clusterizeDetUnit(const edm::DetSet &, output_t::FastFiller &); - void clusterizeDetUnit(const edmNew::DetSet &, output_t::FastFiller &); + void clusterizeDetUnit(const edm::DetSet &, output_t::TSFastFiller &); + void clusterizeDetUnit(const edmNew::DetSet &, output_t::TSFastFiller &); bool stripByStripBegin(uint32_t id); @@ -25,26 +25,26 @@ class ThreeThresholdAlgorithm final : public StripClusterizerAlgorithm { } // detset interface - void addFed(sistrip::FEDZSChannelUnpacker & unpacker, uint16_t ipair, output_t::FastFiller & out) override { + void addFed(sistrip::FEDZSChannelUnpacker & unpacker, uint16_t ipair, output_t::TSFastFiller & out) override { while (unpacker.hasData()) { stripByStripAdd(unpacker.sampleNumber()+ipair*256,unpacker.adc(),out); unpacker++; } } - void stripByStripAdd(uint16_t strip, uint8_t adc, output_t::FastFiller & out) override { + void stripByStripAdd(uint16_t strip, uint8_t adc, output_t::TSFastFiller & out) override { if(candidateEnded(strip)) endCandidate(out); addToCandidate(strip,adc); } - void stripByStripEnd(output_t::FastFiller & out) override { endCandidate(out);} + void stripByStripEnd(output_t::TSFastFiller & out) override { endCandidate(out);} void cleanState() override {clearCandidate();} private: - template void clusterizeDetUnit_(const T&, output_t::FastFiller&); + template void clusterizeDetUnit_(const T&, output_t::TSFastFiller&); ThreeThresholdAlgorithm(float, float, float, unsigned, unsigned, unsigned, std::string qualityLabel, bool setDetId, bool removeApvShots, float minGoodCharge); diff --git a/RecoLocalTracker/SiStripClusterizer/plugins/ClustersFromRawProducer.cc b/RecoLocalTracker/SiStripClusterizer/plugins/ClustersFromRawProducer.cc index bdb2ab5a93b71..549006907d7b3 100644 --- a/RecoLocalTracker/SiStripClusterizer/plugins/ClustersFromRawProducer.cc +++ b/RecoLocalTracker/SiStripClusterizer/plugins/ClustersFromRawProducer.cc @@ -117,7 +117,7 @@ namespace { ~ClusterFiller() { printStat();} - void fill(StripClusterizerAlgorithm::output_t::FastFiller & record) override; + void fill(StripClusterizerAlgorithm::output_t::TSFastFiller & record) override; private: @@ -276,7 +276,7 @@ void SiStripClusterizerFromRaw::run(const FEDRawDataCollection& rawColl, // loop over good det in cabling for ( auto idet : clusterizer_->allDetIds()) { - StripClusterizerAlgorithm::output_t::FastFiller record(output, idet); + StripClusterizerAlgorithm::output_t::TSFastFiller record(output, idet); filler.fill(record); @@ -285,7 +285,7 @@ void SiStripClusterizerFromRaw::run(const FEDRawDataCollection& rawColl, } // end loop over dets } -void ClusterFiller::fill(StripClusterizerAlgorithm::output_t::FastFiller & record) { +void ClusterFiller::fill(StripClusterizerAlgorithm::output_t::TSFastFiller & record) { try { incReady(); diff --git a/RecoLocalTracker/SiStripClusterizer/src/OldThreeThresholdAlgorithm.cc b/RecoLocalTracker/SiStripClusterizer/src/OldThreeThresholdAlgorithm.cc index feb79aa6b4a71..b603e777ff91e 100644 --- a/RecoLocalTracker/SiStripClusterizer/src/OldThreeThresholdAlgorithm.cc +++ b/RecoLocalTracker/SiStripClusterizer/src/OldThreeThresholdAlgorithm.cc @@ -11,16 +11,16 @@ void OldThreeThresholdAlgorithm::initialize(const edm::EventSetup& es) { es.get().get(qualityLabel_,qualityHandle_); } -void OldThreeThresholdAlgorithm::clusterizeDetUnit(const edm::DetSet & digis, edmNew::DetSetVector::FastFiller & output) { +void OldThreeThresholdAlgorithm::clusterizeDetUnit(const edm::DetSet & digis, edmNew::DetSetVector::TSFastFiller & output) { clusterizeDetUnit_(digis,output); } -void OldThreeThresholdAlgorithm::clusterizeDetUnit(const edmNew::DetSet & digis, edmNew::DetSetVector::FastFiller & output) { +void OldThreeThresholdAlgorithm::clusterizeDetUnit(const edmNew::DetSet & digis, edmNew::DetSetVector::TSFastFiller & output) { clusterizeDetUnit_(digis,output); } template void OldThreeThresholdAlgorithm::clusterizeDetUnit_(const InputDetSet& input, - edmNew::DetSetVector::FastFiller& output) { + edmNew::DetSetVector::TSFastFiller& output) { #ifdef PATCH_FOR_DIGIS_DUPLICATION bool printPatchError=false; diff --git a/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc b/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc index 80e9714d6a79f..b02b977f4175f 100644 --- a/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc +++ b/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc @@ -24,7 +24,7 @@ ThreeThresholdAlgorithm(float chan, float seed, float cluster, unsigned holes, u template inline void ThreeThresholdAlgorithm:: -clusterizeDetUnit_(const digiDetSet& digis, output_t::FastFiller& output) { +clusterizeDetUnit_(const digiDetSet& digis, output_t::TSFastFiller& output) { if(isModuleBad(digis.detId())) return; if (!setDetId( digis.detId() )) return; @@ -125,8 +125,8 @@ appendBadNeighbors() { } -void ThreeThresholdAlgorithm::clusterizeDetUnit(const edm::DetSet& digis, output_t::FastFiller& output) {clusterizeDetUnit_(digis,output);} -void ThreeThresholdAlgorithm::clusterizeDetUnit(const edmNew::DetSet& digis, output_t::FastFiller& output) {clusterizeDetUnit_(digis,output);} +void ThreeThresholdAlgorithm::clusterizeDetUnit(const edm::DetSet& digis, output_t::TSFastFiller& output) {clusterizeDetUnit_(digis,output);} +void ThreeThresholdAlgorithm::clusterizeDetUnit(const edmNew::DetSet& digis, output_t::TSFastFiller& output) {clusterizeDetUnit_(digis,output);} inline bool ThreeThresholdAlgorithm:: diff --git a/RecoLocalTracker/SiStripClusterizer/test/ClusterRefinerTagMCmerged.cc b/RecoLocalTracker/SiStripClusterizer/test/ClusterRefinerTagMCmerged.cc index 07f763f9d3883..364fff5dadd02 100644 --- a/RecoLocalTracker/SiStripClusterizer/test/ClusterRefinerTagMCmerged.cc +++ b/RecoLocalTracker/SiStripClusterizer/test/ClusterRefinerTagMCmerged.cc @@ -36,7 +36,7 @@ refineCluster(const edm::Handle< edmNew::DetSetVector >& input, for (edmNew::DetSetVector::const_iterator det=input->begin(); det!=input->end(); det++) { // DetSetVector filler to receive the clusters we produce - edmNew::DetSetVector::FastFiller outFill(*output, det->id()); + edmNew::DetSetVector::TSFastFiller outFill(*output, det->id()); uint32_t detId = det->id(); int ntk = 0; int NtkAll = 0; diff --git a/RecoLocalTracker/SiStripClusterizer/test/ClusterizerUnitTester.cc b/RecoLocalTracker/SiStripClusterizer/test/ClusterizerUnitTester.cc index fe15e43a01387..698248d4294d2 100644 --- a/RecoLocalTracker/SiStripClusterizer/test/ClusterizerUnitTester.cc +++ b/RecoLocalTracker/SiStripClusterizer/test/ClusterizerUnitTester.cc @@ -62,7 +62,7 @@ runTheTest(const PSet& test) { void ClusterizerUnitTester:: constructDigis(const VPSet& stripset, edmNew::DetSetVector& digis) { - edmNew::DetSetVector::FastFiller digisFF(digis, detId); + edmNew::DetSetVector::TSFastFiller digisFF(digis, detId); for(iter_t strip = stripset.begin(); strip < stripset.end(); strip++) { digisFF.push_back( SiStripDigi(strip->getParameter("Strip"), strip->getParameter("ADC") )); @@ -72,7 +72,7 @@ constructDigis(const VPSet& stripset, edmNew::DetSetVector& digis) void ClusterizerUnitTester:: constructClusters(const VPSet& clusterset, output_t& clusters) { - output_t::FastFiller clustersFF(clusters, detId); + output_t::TSFastFiller clustersFF(clusters, detId); for(iter_t c = clusterset.begin(); cgetParameter("FirstStrip"); std::vector amplitudes = c->getParameter >("Amplitudes"); diff --git a/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc b/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc index 370bd0776818c..39dd37b0dbd56 100644 --- a/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc +++ b/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc @@ -48,7 +48,7 @@ produce(edm::Event& event, const edm::EventSetup& es) { } if(!clusters.empty()) { - output_t::FastFiller filler(*output, inputDetSet->detId()); + output_t::TSFastFiller filler(*output, inputDetSet->detId()); for( unsigned i=0; i detid: " << detid << " --> subdet: " << subdet << std::endl; - for (auto i = item.offset; i Date: Mon, 9 Nov 2015 17:07:15 +0100 Subject: [PATCH 03/10] hide atomic from root --- DataFormats/Common/interface/DetSetVectorNew.h | 5 ++--- DataFormats/Common/src/classes_def.xml | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/DataFormats/Common/interface/DetSetVectorNew.h b/DataFormats/Common/interface/DetSetVectorNew.h index 2b9ad5a5afe55..1e5abd30746f6 100644 --- a/DataFormats/Common/interface/DetSetVectorNew.h +++ b/DataFormats/Common/interface/DetSetVectorNew.h @@ -2,7 +2,6 @@ #define DataFormats_Common_DetSetVectorNew_h #include "DataFormats/Common/interface/CMS_CLASS_VERSION.h" -// #include "DataFormats/Common/interface/DetSet.h" // to get det_id_type #include "DataFormats/Common/interface/DetSetNew.h" #include "DataFormats/Common/interface/traits.h" @@ -15,7 +14,7 @@ #include "FWCore/Utilities/interface/Exception.h" #include "FWCore/Utilities/interface/GCC11Compatibility.h" -#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__) +#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__) && !defined(__ROOTCLING__) #define USE_ATOMIC // #warning using atomic #endif @@ -763,4 +762,4 @@ namespace edm { #endif #endif - + diff --git a/DataFormats/Common/src/classes_def.xml b/DataFormats/Common/src/classes_def.xml index a0b3fbd62e9c9..8231b43e76ae8 100644 --- a/DataFormats/Common/src/classes_def.xml +++ b/DataFormats/Common/src/classes_def.xml @@ -96,8 +96,7 @@ - - + From 32c6490029d26cbd67a1c81b4a17e5da9592442b Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Mon, 9 Nov 2015 18:03:12 +0100 Subject: [PATCH 04/10] print what is --- DataFormats/Common/test/DetSetNewTS_t.cpp | 292 ++++++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 DataFormats/Common/test/DetSetNewTS_t.cpp diff --git a/DataFormats/Common/test/DetSetNewTS_t.cpp b/DataFormats/Common/test/DetSetNewTS_t.cpp new file mode 100644 index 0000000000000..3429b59a5820e --- /dev/null +++ b/DataFormats/Common/test/DetSetNewTS_t.cpp @@ -0,0 +1,292 @@ +#include "Utilities/Testing/interface/CppUnit_testdriver.icpp" +#include "cppunit/extensions/HelperMacros.h" + +#include "DataFormats/Common/interface/DetSetNew.h" +#include "DataFormats/Common/interface/DetSetVectorNew.h" +#include "DataFormats/Common/interface/DetSetAlgorithm.h" +#include "DataFormats/Common/interface/DetSet2RangeMap.h" + +#include "FWCore/Utilities/interface/EDMException.h" + +#include +#include + +#include +#include +typedef std::mutex Mutex; +// typedef std::lock_guard Lock; +typedef std::unique_lock Lock; + +namespace global { + // control cout.... + Mutex coutLock; +} + +#include +#include +#include + + +template +inline void spinlock(std::atomic const & lock, T val) { + while (lock.load(std::memory_order_acquire)!=val){} +} + + + +template +inline void spinlockSleep(std::atomic const & lock, T val) { + while (lock.load(std::memory_order_acquire)!=val){nanosleep(0,0);} +} + +// syncronize all threads in a parallel section (for testing purposes) +void sync(std::atomic & all) { + auto sum = omp_get_num_threads(); sum = sum*(sum+1)/2; + all.fetch_add(omp_get_thread_num()+1,std::memory_order_acq_rel); + spinlock(all,sum); + +} + + + +struct B{ + virtual ~B(){} + virtual B * clone() const=0; +}; + +struct T : public B { + T(int iv=0) : v(iv){} + int v; + bool operator==(T t) const { return v==t.v;} + + virtual T * clone() const { return new T(*this); } +}; + +bool operator==(T const& t, B const& b) { + T const * p = dynamic_cast(&b); + return p && p->v==t.v; +} + +bool operator==(B const& b, T const& t) { + return t==b; +} + +typedef edmNew::DetSetVector DSTV; +typedef edmNew::DetSet DST; +typedef edmNew::det_id_type det_id_type; +typedef DSTV::FastFiller FF; +typedef DSTV::TSFastFiller TSFF; + + +class TestDetSet: public CppUnit::TestFixture +{ + CPPUNIT_TEST_SUITE(TestDetSet); + CPPUNIT_TEST(infrastructure); + CPPUNIT_TEST(fillSeq); + CPPUNIT_TEST(fillPar); + + CPPUNIT_TEST_SUITE_END(); + +public: + TestDetSet(); + ~TestDetSet() {} + void setUp() {} + void tearDown() {} + + void infrastructure(); + void fillSeq(); + void fillPar(); + +public: + std::vector sv; + +}; + +CPPUNIT_TEST_SUITE_REGISTRATION(TestDetSet); + +TestDetSet::TestDetSet() : sv(10){ + DSTV::data_type v[10] = {0,1,2,3,4,5,6,7,8,9}; + std::copy(v,v+10,sv.begin()); +} + +void read(DSTV const & detsets, bool all=false) { + int i=0; + for (auto di = detsets.begin(false); di!=detsets.end(false); ++di) { + auto ds = *di; + auto id = ds.id(); + std::cout << id <<' '; + // if (all) CPPUNIT_ASSERT(int(id)==20+i); + if (ds.isValid()) + { + CPPUNIT_ASSERT(ds[0]==100*(id-20)+3); + CPPUNIT_ASSERT(ds[1]==-(100*(id-20)+3)); + } + ++i; + } + std::cout << std::endl; +} + + +void TestDetSet::infrastructure() { + std::cout << std::endl; + for (int i=0; i<10; i++) { + int a=0; + std::atomic b(0); + std::atomic lock(0); + std::atomic nt(0); +#pragma omp parallel + { + nt = omp_get_num_threads(); + sync(lock); + a++; + b.fetch_add(1,std::memory_order_acq_rel);; + } + + if (i==5) std::cout << lock << " " << a << ' ' << b << std::endl; + CPPUNIT_ASSERT(b==nt); + a=0; b=0; + +#pragma omp parallel + { + a++; + b.fetch_add(1,std::memory_order_acq_rel); + } + if (i==5) std::cout << lock << " " << a << ' ' << b << std::endl; + } + + + + +} + +void TestDetSet::fillSeq() { + std::cout << std::endl; + + DSTV detsets(2); + // unsigned int ntot=0; + + std::atomic lock(0); + std::atomic idet(0); + std::atomic trial(0); + int maxDet=100; +#pragma omp parallel + { + sync(lock); + while(true) { + int ldet = idet.load(std::memory_order_acquire); + if (!(ldet=maxDet) break; + unsigned int id=20+ldet; + bool done=false; + while(!done) { + try { + { + FF ff(detsets, id); // serialize + ff.push_back(100*ldet+3); + CPPUNIT_ASSERT(detsets.m_data.back().v==(100*ldet+3)); + ff.push_back(-(100*ldet+3)); + CPPUNIT_ASSERT(detsets.m_data.back().v==-(100*ldet+3)); + } + // read(detsets); // cannot read in parallel while filling in this case + done=true; + } catch (edm::Exception const&) { + trial.fetch_add(1,std::memory_order_acq_rel); + //read(detsets); + } + } + } + // read(detsets); + } + + std::cout << idet << ' ' << detsets.size() << std::endl; + read(detsets,true); + CPPUNIT_ASSERT(int(detsets.size())==maxDet); + std::cout << "trials " << trial << std::endl; +} + + + struct Getter final : public DSTV::Getter { + Getter(TestDetSet * itest):ntot(0), test(*itest){} + + void fill(TSFF& ff) override { + int n=ff.id()-20; + CPPUNIT_ASSERT(n>=0); + CPPUNIT_ASSERT(ff.size()==0); + ff.push_back((100*n+3)); + CPPUNIT_ASSERT(ff.size()==1); + CPPUNIT_ASSERT(ff[0]==100*n+3); + ff.push_back(-(100*n+3)); + CPPUNIT_ASSERT(ff.size()==2); + CPPUNIT_ASSERT(ff[1]==-(100*n+3)); + ntot.fetch_add(1,std::memory_order_acq_rel); + } + + std::atomic ntot; + TestDetSet & test; + }; + +void TestDetSet::fillPar() { + std::cout << std::endl; + std::shared_ptr pg(new Getter(this)); + Getter & g = *pg; + int maxDet=100; + std::vector v(maxDet); int k=20;for (auto &i:v) i=k++; + DSTV detsets(pg,v,2); + CPPUNIT_ASSERT(g.ntot==0); + CPPUNIT_ASSERT(detsets.onDemand()); + CPPUNIT_ASSERT(maxDet==int(detsets.size())); + + + std::atomic lock(0); + std::atomic idet(0); + std::atomic trial(0); + + std::atomic count(0); + + + DST df31 = detsets[31]; + + std::cout << "start parallel section" << std::endl; +#pragma omp parallel + { + sync(lock); + if (omp_get_thread_num()%2==0) { + DST df = detsets[25]; // everybody! + CPPUNIT_ASSERT(df.id()==25); + CPPUNIT_ASSERT(df.size()==2); + CPPUNIT_ASSERT(df[0]==100*(25-20)+3); + CPPUNIT_ASSERT(df[1]==-(100*(25-20)+3)); + + } + while(true) { + if (omp_get_thread_num()==0) read(detsets); + int ldet = idet.load(std::memory_order_acquire); + if (!(ldet=maxDet) break; + unsigned int id=20+ldet; + { + DST df = *detsets.find(id, true); + CPPUNIT_ASSERT(int(g.ntot)>0); + assert(df.id()==id); + assert(df.size()==2); + assert(df[0]==100*(id-20)+3); + assert(df[1]==-(100*(id-20)+3)); + + } + if (omp_get_thread_num()==1) read(detsets); + } + } + std::cout << "end parallel section" << std::endl; + + CPPUNIT_ASSERT(df31.id()==31); + CPPUNIT_ASSERT(df31.size()==2); + CPPUNIT_ASSERT(df31[0]==100*(31-20)+3); + CPPUNIT_ASSERT(df31[1]==-(100*(31-20)+3)); + + std::cout << "summary " << idet << ' ' << detsets.size() << ' ' << g.ntot << ' ' << count << std::endl; + read(detsets,true); + CPPUNIT_ASSERT(int(g.ntot)==maxDet); + CPPUNIT_ASSERT(int(detsets.size())==maxDet); +} From 58873f60e2c2c53b3ccde65cb57456301e89454f Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Fri, 13 Nov 2015 14:17:15 +0100 Subject: [PATCH 05/10] compiles --- .../interface/StripClusterizerAlgorithm.h | 14 +++++------- .../interface/ThreeThresholdAlgorithm.h | 13 ++++++----- .../src/StripClusterizerAlgorithm.cc | 2 +- .../src/StripClusterizerAlgorithmFactory.cc | 4 ---- .../src/ThreeThresholdAlgorithm.cc | 13 +++++------ .../test/StripByStripTestDriver.cc | 22 +++---------------- 6 files changed, 23 insertions(+), 45 deletions(-) diff --git a/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h b/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h index 7104f01402834..a408683a78a4c 100644 --- a/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h +++ b/RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h @@ -55,10 +55,10 @@ class StripClusterizerAlgorithm { //Offline DetSet interface typedef edmNew::DetSetVector output_t; - void clusterize(const edm::DetSetVector &, output_t &); - void clusterize(const edmNew::DetSetVector &, output_t &); - virtual void clusterizeDetUnit(const edm::DetSet &, output_t::TSFastFiller &) = 0; - virtual void clusterizeDetUnit(const edmNew::DetSet &, output_t::TSFastFiller &) = 0; + void clusterize(const edm::DetSetVector &, output_t &) const; + void clusterize(const edmNew::DetSetVector &, output_t &) const; + virtual void clusterizeDetUnit(const edm::DetSet &, output_t::TSFastFiller &) const = 0; + virtual void clusterizeDetUnit(const edmNew::DetSet &, output_t::TSFastFiller &) const = 0; //HLT stripByStrip interface virtual Det stripByStripBegin(uint32_t id)const = 0; @@ -69,7 +69,7 @@ class StripClusterizerAlgorithm { virtual void addFed(State & state, sistrip::FEDZSChannelUnpacker & unpacker, uint16_t ipair, output_t::TSFastFiller & out) const {} virtual void stripByStripAdd(State & state, uint16_t strip, uint8_t adc, output_t::TSFastFiller & out) const {} - virtual void stripByStripEnd(State & state, output_t::FastFiller & out) const {} + virtual void stripByStripEnd(State & state, output_t::TSFastFiller & out) const {} struct InvalidChargeException : public cms::Exception { public: InvalidChargeException(const SiStripDigi&); }; @@ -84,13 +84,11 @@ class StripClusterizerAlgorithm { StripClusterizerAlgorithm() : qualityLabel(""), noise_cache_id(0), gain_cache_id(0), quality_cache_id(0) {} - // uint32_t currentId() const {return detId;} - Det setDetId(const uint32_t) const; + Det findDetId(const uint32_t) const; bool isModuleBad(const uint32_t& id) const { return qualityHandle->IsModuleBad( id ); } bool isModuleUsable(const uint32_t& id) const { return qualityHandle->IsModuleUsable( id ); } std::string qualityLabel; - bool _setDetId; private: diff --git a/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h b/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h index 2ad2784287166..dae7ca6c39158 100644 --- a/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h +++ b/RecoLocalTracker/SiStripClusterizer/interface/ThreeThresholdAlgorithm.h @@ -11,14 +11,14 @@ class ThreeThresholdAlgorithm final : public StripClusterizerAlgorithm { using State = StripClusterizerAlgorithm::State; using Det = StripClusterizerAlgorithm::Det; - void clusterizeDetUnit(const edm::DetSet &, output_t::TSFastFiller &); - void clusterizeDetUnit(const edmNew::DetSet &, output_t::TSFastFiller &); + void clusterizeDetUnit(const edm::DetSet &, output_t::TSFastFiller &) const override; + void clusterizeDetUnit(const edmNew::DetSet &, output_t::TSFastFiller &) const override; Det stripByStripBegin(uint32_t id) const; // LazyGetter interface - void stripByStripAdd(State & state, uint16_t strip, uint8_t adc, std::vector& out) const; - void stripByStripEnd(State & state, std::vector& out) const; + void stripByStripAdd(State & state, uint16_t strip, uint8_t adc, std::vector& out) const override; + void stripByStripEnd(State & state, std::vector& out) const override; void addFed(State & state, sistrip::FEDZSChannelUnpacker & unpacker, uint16_t ipair, std::vector& out) const { while (unpacker.hasData()) { @@ -45,9 +45,10 @@ class ThreeThresholdAlgorithm final : public StripClusterizerAlgorithm { private: - template void clusterizeDetUnit_(const T&, output_t::TSFastFiller&); + template void clusterizeDetUnit_(const T&, output_t::TSFastFiller&) const; + ThreeThresholdAlgorithm(float, float, float, unsigned, unsigned, unsigned, std::string qualityLabel, - bool setDetId, bool removeApvShots, float minGoodCharge); + bool removeApvShots, float minGoodCharge); //constant methods with state information diff --git a/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithm.cc b/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithm.cc index acefc8d883af3..b8c6356c8d8dc 100644 --- a/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithm.cc +++ b/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithm.cc @@ -127,7 +127,7 @@ initialize(const edm::EventSetup& es) { StripClusterizerAlgorithm::Det StripClusterizerAlgorithm:: -setDetId(const uint32_t id) const { +findDetId(const uint32_t id) const { auto b = detIds.begin(); auto e = detIds.end(); auto p = std::lower_bound(b,e,id); diff --git a/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithmFactory.cc b/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithmFactory.cc index 3f03bd079783a..9f2d648396cf0 100644 --- a/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithmFactory.cc +++ b/RecoLocalTracker/SiStripClusterizer/src/StripClusterizerAlgorithmFactory.cc @@ -9,9 +9,6 @@ std::auto_ptr StripClusterizerAlgorithmFactory:: create(const edm::ParameterSet& conf) { std::string algorithm = conf.getParameter("Algorithm"); - bool setDetId=false; - if (conf.exists("setDetId")) - setDetId = conf.getParameter("setDetId"); if(algorithm == "ThreeThresholdAlgorithm") { return std::auto_ptr( new ThreeThresholdAlgorithm( @@ -22,7 +19,6 @@ create(const edm::ParameterSet& conf) { conf.getParameter("MaxSequentialBad"), conf.getParameter("MaxAdjacentBad"), conf.getParameter("QualityLabel"), - setDetId, conf.getParameter("RemoveApvShots"), clusterChargeCut(conf) )); diff --git a/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc b/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc index eb373594c57d7..2414b3c19a173 100644 --- a/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc +++ b/RecoLocalTracker/SiStripClusterizer/src/ThreeThresholdAlgorithm.cc @@ -9,20 +9,19 @@ ThreeThresholdAlgorithm:: ThreeThresholdAlgorithm(float chan, float seed, float cluster, unsigned holes, unsigned bad, unsigned adj, std::string qL, - bool setDetId, bool removeApvShots, float minGoodCharge) + bool removeApvShots, float minGoodCharge) : ChannelThreshold( chan ), SeedThreshold( seed ), ClusterThresholdSquared( cluster*cluster ), MaxSequentialHoles( holes ), MaxSequentialBad( bad ), MaxAdjacentBad( adj ), RemoveApvShots(removeApvShots), minGoodCharge(minGoodCharge) { - _setDetId=setDetId; qualityLabel = (qL); } template inline void ThreeThresholdAlgorithm:: -clusterizeDetUnit_(const digiDetSet& digis, output_t::TSFastFiller& output) { +clusterizeDetUnit_(const digiDetSet& digis, output_t::TSFastFiller& output) const { if(isModuleBad(digis.detId())) return; - auto const & det = setDetId( digis.detId()); + auto const & det = findDetId( digis.detId()); if (!det.valid()) return; #ifdef EDM_ML_DEBUG @@ -123,13 +122,13 @@ appendBadNeighbors(State & state) const { } -void ThreeThresholdAlgorithm::clusterizeDetUnit(const edm::DetSet& digis, output_t::TSFastFiller& output) {clusterizeDetUnit_(digis,output);} -void ThreeThresholdAlgorithm::clusterizeDetUnit(const edmNew::DetSet& digis, output_t::TSFastFiller& output) {clusterizeDetUnit_(digis,output);} +void ThreeThresholdAlgorithm::clusterizeDetUnit(const edm::DetSet& digis, output_t::TSFastFiller& output) const {clusterizeDetUnit_(digis,output);} +void ThreeThresholdAlgorithm::clusterizeDetUnit(const edmNew::DetSet& digis, output_t::TSFastFiller& output) const {clusterizeDetUnit_(digis,output);} StripClusterizerAlgorithm::Det ThreeThresholdAlgorithm:: stripByStripBegin(uint32_t id) const { - return setDetId( id ); + return findDetId( id ); } diff --git a/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc b/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc index 644fd81af7c40..9eb595d341a44 100644 --- a/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc +++ b/RecoLocalTracker/SiStripClusterizer/test/StripByStripTestDriver.cc @@ -29,26 +29,10 @@ produce(edm::Event& event, const edm::EventSetup& es) { edm::Handle< edm::DetSetVector > input; event.getByLabel(inputTag, input); - if(hlt);// hltFactory->eventSetup(es); - else algorithm->initialize(es); + algorithm->initialize(es); - for(auto const & inputDetSet : *input) { - - std::vector clusters; - if( hlt || algorithm->stripByStripBegin( inputDetSet.detId() ) ) { - for(auto const & digi : inputDetSet ) { - if(hlt);// hltFactory->algorithm()->add(clusters, inputDetSet->detId(), digi.strip(), digi.adc()); - else algorithm->stripByStripAdd(digi.strip(), digi.adc(), clusters); - } - if(hlt);// hltFactory->algorithm()->endDet(clusters, inputDetSet.detId()); - else algorithm->stripByStripEnd(clusters); - } - - if(!clusters.empty()) { - output_t::TSFastFiller filler(*output, inputDetSet.detId()); - for( unsigned i=0; istripByStripBegin( inputDetSet.detId()); if( !det.valid() ) continue; From 1577234cb62b0a6a6b75e98f2dfda12d5631e3c0 Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Fri, 13 Nov 2015 14:38:53 +0100 Subject: [PATCH 06/10] scale test with nthreads --- DataFormats/Common/test/DetSetNewTS_t.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/DataFormats/Common/test/DetSetNewTS_t.cpp b/DataFormats/Common/test/DetSetNewTS_t.cpp index 3429b59a5820e..e19224cbf35d1 100644 --- a/DataFormats/Common/test/DetSetNewTS_t.cpp +++ b/DataFormats/Common/test/DetSetNewTS_t.cpp @@ -98,6 +98,7 @@ class TestDetSet: public CppUnit::TestFixture void fillPar(); public: + int nth=1; std::vector sv; }; @@ -107,6 +108,12 @@ CPPUNIT_TEST_SUITE_REGISTRATION(TestDetSet); TestDetSet::TestDetSet() : sv(10){ DSTV::data_type v[10] = {0,1,2,3,4,5,6,7,8,9}; std::copy(v,v+10,sv.begin()); + std::atomic nt(1); + #pragma omp parallel + { + nt = omp_get_num_threads(); + } + nth = nt; } void read(DSTV const & detsets, bool all=false) { @@ -142,7 +149,7 @@ void TestDetSet::infrastructure() { b.fetch_add(1,std::memory_order_acq_rel);; } - if (i==5) std::cout << lock << " " << a << ' ' << b << std::endl; + if (i==5) std::cout << "threads "<< lock << " " << a << ' ' << b << std::endl; CPPUNIT_ASSERT(b==nt); a=0; b=0; @@ -151,12 +158,12 @@ void TestDetSet::infrastructure() { a++; b.fetch_add(1,std::memory_order_acq_rel); } - if (i==5) std::cout << lock << " " << a << ' ' << b << std::endl; + if (i==5) std::cout << "threads "<< lock << " " << a << ' ' << b << std::endl; + + nth = nt; } - - } void TestDetSet::fillSeq() { @@ -168,7 +175,7 @@ void TestDetSet::fillSeq() { std::atomic lock(0); std::atomic idet(0); std::atomic trial(0); - int maxDet=100; + int maxDet=100*nth; #pragma omp parallel { sync(lock); @@ -230,7 +237,7 @@ void TestDetSet::fillPar() { std::cout << std::endl; std::shared_ptr pg(new Getter(this)); Getter & g = *pg; - int maxDet=100; + int maxDet=100*nth; std::vector v(maxDet); int k=20;for (auto &i:v) i=k++; DSTV detsets(pg,v,2); CPPUNIT_ASSERT(g.ntot==0); From 181c3d1033b2923885d0e23beb09b56964e838a4 Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Fri, 13 Nov 2015 14:52:36 +0100 Subject: [PATCH 07/10] remove old from test --- .../python/test/CompareAlgorithms_cfg.py | 25 ++----------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/RecoLocalTracker/SiStripClusterizer/python/test/CompareAlgorithms_cfg.py b/RecoLocalTracker/SiStripClusterizer/python/test/CompareAlgorithms_cfg.py index 7a5a7116b2565..09d6e19e5f231 100644 --- a/RecoLocalTracker/SiStripClusterizer/python/test/CompareAlgorithms_cfg.py +++ b/RecoLocalTracker/SiStripClusterizer/python/test/CompareAlgorithms_cfg.py @@ -24,17 +24,6 @@ # Configuration which varies depending on what to compare -process.OldClusterizer = cms.EDProducer("SiStripClusterizer", - Clusterizer = cms.PSet( Algorithm = cms.string("OldThreeThresholdAlgorithm"), - ChannelThreshold = cms.double(2), - SeedThreshold = cms.double(3), - ClusterThreshold = cms.double(5), - MaxSequentialHoles = cms.uint32(0), - QualityLabel = cms.string("")), - DigiProducersList = cms.VInputTag( cms.InputTag('siStripDigis','ZeroSuppressed'), - cms.InputTag('siStripZeroSuppression','VirginRaw'), - cms.InputTag('siStripZeroSuppression','ProcessedRaw'), - cms.InputTag('siStripZeroSuppression','ScopeMode'))) process.NewClusterizer = cms.EDProducer("SiStripClusterizer", Clusterizer = cms.PSet( Algorithm = cms.string("ThreeThresholdAlgorithm"), ChannelThreshold = cms.double(2), @@ -66,18 +55,11 @@ SeedThreshold = cms.untracked.double(3.0), MaxHolesInCluster = cms.untracked.uint32(0), ClusterThreshold = cms.untracked.double(5.0), - DigiProducer = cms.InputTag('siStripDigis','ZeroSuppressed'), HLT = cms.bool(True) ) - -process.CompareOldNew = cms.EDAnalyzer("CompareClusters", - Clusters1 = cms.InputTag('OldClusterizer',''), - Clusters2 = cms.InputTag('NewClusterizer',''), - Digis = cms.InputTag('siStripDigis','ZeroSuppressed') - ) -process.CompareOldHLT = cms.EDAnalyzer("CompareClusters", - Clusters1 = cms.InputTag('OldClusterizer',''), +process.CompareNewHLT = cms.EDAnalyzer("CompareClusters", + Clusters1 = cms.InputTag('NewClusterizer',''), Clusters2 = cms.InputTag('HLTStripByStrip',''), Digis = cms.InputTag('siStripDigis','ZeroSuppressed') ) @@ -90,13 +72,10 @@ process.p1 = cms.Path( process.siStripDigis * process.siStripZeroSuppression * #process.profilerStart * - process.OldClusterizer * process.NewClusterizer * process.HLTStripByStrip * process.NewStripByStrip * #process.profilerStop * - - process.CompareOldNew * process.CompareOldHLT * process.CompareNewNew ) From 95953f1d1e9a808759386e3a3fa7b2458647d0cd Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Sat, 14 Nov 2015 10:31:18 +0100 Subject: [PATCH 08/10] fix test for clang, add flags to test reproducibility of CKfPattern --- DataFormats/Common/test/DetSetNewTS_t.cpp | 7 +++++ .../src/CkfTrackCandidateMakerBase.cc | 30 ++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/DataFormats/Common/test/DetSetNewTS_t.cpp b/DataFormats/Common/test/DetSetNewTS_t.cpp index e19224cbf35d1..41c619316e8e4 100644 --- a/DataFormats/Common/test/DetSetNewTS_t.cpp +++ b/DataFormats/Common/test/DetSetNewTS_t.cpp @@ -11,7 +11,14 @@ #include #include +#ifndef __clang__ +// CLANG does not support OpenMP #include +#else +inline +int omp_get_thread_num() {return 1;} +int omp_get_num_threads(){return 1;} +#endif #include typedef std::mutex Mutex; // typedef std::lock_guard Lock; diff --git a/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc b/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc index 1205073f737b3..f975161e606c0 100644 --- a/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc +++ b/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc @@ -41,6 +41,10 @@ #include #include +// #define VI_SORTSEED +// #define VI_REPRODUCIBLE +// #define VI_TBB + #include #ifdef VI_TBB #include "tbb/parallel_for.h" @@ -84,15 +88,16 @@ namespace cms{ // theSeedLabel = InputTag(conf_.getParameter("SeedProducer"),conf_.getParameter("SeedLabel")); // else theSeedLabel= iC.consumes >(conf.getParameter("src")); +#ifndef VI_REPRODUCIBLE if ( conf.exists("maxSeedsBeforeCleaning") ) maxSeedsBeforeCleaning_=conf.getParameter("maxSeedsBeforeCleaning"); - +#endif if (conf.existsAs("clustersToSkip")) { skipClusters_ = true; maskPixels_ = iC.consumes(conf.getParameter("clustersToSkip")); maskStrips_ = iC.consumes(conf.getParameter("clustersToSkip")); } - +#ifndef VI_REPRODUCIBLE std::string cleaner = conf.getParameter("RedundantSeedCleaner"); if (cleaner == "SeedCleanerByHitPosition") { theSeedCleaner = new SeedCleanerByHitPosition(); @@ -111,7 +116,13 @@ namespace cms{ } else { throw cms::Exception("RedundantSeedCleaner not found", cleaner); } +#endif +#ifdef VI_REPRODUCIBLE + std::cout << "CkfTrackCandidateMaker in reproducible setting" << std::endl; + assert(nullptr==theSeedCleaner); + assert(0>=maxSeedsBeforeCleaning_); +#endif } @@ -217,11 +228,14 @@ namespace cms{ size_t collseed_size = collseed->size(); unsigned int indeces[collseed_size]; for (auto i=0U; i< collseed_size; ++i) indeces[i]=i; - // std::random_shuffle(indeces,indeces+collseed_size); - /* - * here only for reference: does not seems to help + + +#ifdef VI_SORTSEED + // std::random_shuffle(indeces,indeces+collseed_size); + + // here only for reference: does not seems to help auto const & seeds = *collseed; @@ -231,7 +245,7 @@ namespace cms{ { val[i] = seeds[i].startingState().pt();}; // { val[i] = std::abs((*seeds[i].recHits().first).surface()->eta());} - + /* unsigned long long val[collseed_size]; for (auto i=0U; i< collseed_size; ++i) { if (seeds[i].nHits()<2) { val[i]=0; continue;} @@ -243,13 +257,13 @@ namespace cms{ val[i] |= (unsigned long long)(hit.firstClusterRef().key())<<32; } } - + */ std::sort(indeces,indeces+collseed_size, [&](unsigned int i, unsigned int j){return val[i] Date: Sat, 14 Nov 2015 15:31:56 +0100 Subject: [PATCH 09/10] make tracking fully reproducible (independent of seed order --- .../plugins/GroupedCkfTrajectoryBuilder.cc | 1 + .../CkfPattern/src/CkfTrackCandidateMakerBase.cc | 16 ++++++++++++---- .../interface/TrackProducerAlgorithm.icc | 1 + 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/RecoTracker/CkfPattern/plugins/GroupedCkfTrajectoryBuilder.cc b/RecoTracker/CkfPattern/plugins/GroupedCkfTrajectoryBuilder.cc index c22c9be11af90..4eff924a1f6b4 100644 --- a/RecoTracker/CkfPattern/plugins/GroupedCkfTrajectoryBuilder.cc +++ b/RecoTracker/CkfPattern/plugins/GroupedCkfTrajectoryBuilder.cc @@ -284,6 +284,7 @@ GroupedCkfTrajectoryBuilder::buildTrajectories (const TrajectorySeed& seed, } */ + boost::shared_ptr pseed(new TrajectorySeed(seed)); result.reserve(work_.size()); for (TempTrajectoryContainer::const_iterator it = work_.begin(), ed = work_.end(); it != ed; ++it) { diff --git a/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc b/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc index f975161e606c0..7f436e9208bcd 100644 --- a/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc +++ b/RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc @@ -264,10 +264,12 @@ namespace cms{ // std::cout << spt(indeces[0]) << ' ' << spt(indeces[collseed_size-1]) << std::endl; #endif - + std::atomic ntseed(0); auto theLoop = [&](size_t ii) { auto j = indeces[ii]; + ntseed++; + // to be moved inside a par section (how with tbb??) std::vector theTmpTrajectories; @@ -361,12 +363,18 @@ namespace cms{ theLoop(j); } #endif - + assert(ntseed==collseed_size); if (theSeedCleaner) theSeedCleaner->done(); - // std::cout << "VICkfPattern " << "rawResult trajectories found = " << rawResult.size() << std::endl; + // std::cout << "VICkfPattern " << "rawResult trajectories found = " << rawResult.size() << " in " << ntseed << " seeds " << collseed_size << std::endl; + +#ifdef VI_REPRODUCIBLE + // sort trajectory + std::sort(rawResult.begin(), rawResult.end(),[](const Trajectory & a, const Trajectory & b) + { return a.seedRef().key() < b.seedRef().key();}); + //{ return a.chiSquared()*b.ndof() < b.chiSquared()*a.ndof();}); +#endif - // Step E: Clean the results to avoid duplicate tracks // Rejected ones just flagged as invalid. theTrajectoryCleaner->clean(rawResult); diff --git a/RecoTracker/TrackProducer/interface/TrackProducerAlgorithm.icc b/RecoTracker/TrackProducer/interface/TrackProducerAlgorithm.icc index d5080e2b69c12..fbbcf25722dbf 100644 --- a/RecoTracker/TrackProducer/interface/TrackProducerAlgorithm.icc +++ b/RecoTracker/TrackProducer/interface/TrackProducerAlgorithm.icc @@ -80,6 +80,7 @@ TrackProducerAlgorithm::runWithCandidate(const TrackingGeometry * theG, ++ntc; } LogDebug("TrackProducer") << "Number of Tracks found: " << cont << "\n"; + // std::cout << "VICkfProducer " << "Number of Tracks found: " << cont << std::endl; } From 69c8b5cb1dd8ccfa38e2258b84214b7618036cca Mon Sep 17 00:00:00 2001 From: Vincenzo Innocente Date: Wed, 18 Nov 2015 19:40:58 +0100 Subject: [PATCH 10/10] cleanup + one missing protection --- .../Common/interface/DetSetVectorNew.h | 86 ++++++++----------- DataFormats/Common/test/DetSetNewTS_t.cpp | 7 +- DataFormats/Common/test/DetSetNew_t.cpp | 1 + 3 files changed, 43 insertions(+), 51 deletions(-) diff --git a/DataFormats/Common/interface/DetSetVectorNew.h b/DataFormats/Common/interface/DetSetVectorNew.h index 1e5abd30746f6..f8db96391a022 100644 --- a/DataFormats/Common/interface/DetSetVectorNew.h +++ b/DataFormats/Common/interface/DetSetVectorNew.h @@ -14,16 +14,14 @@ #include "FWCore/Utilities/interface/Exception.h" #include "FWCore/Utilities/interface/GCC11Compatibility.h" -#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__) && !defined(__ROOTCLING__) +#if !defined(__ROOTCLING__) #define USE_ATOMIC // #warning using atomic #endif -#ifdef USE_ATOMIC #include #include #include -#endif #include #include @@ -55,19 +53,11 @@ namespace edmNew { struct DetSetVectorTrans { DetSetVectorTrans(): filling(false){} -#ifndef USE_ATOMIC - mutable bool filling; - private: - DetSetVectorTrans& operator=(const DetSetVectorTrans&){return *this;} - DetSetVectorTrans(const DetSetVectorTrans&){} - public: -#else DetSetVectorTrans& operator=(const DetSetVectorTrans&) = delete; DetSetVectorTrans(const DetSetVectorTrans&) = delete; DetSetVectorTrans(DetSetVectorTrans&&) = default; DetSetVectorTrans& operator=(DetSetVectorTrans&&) = default; mutable std::atomic filling; -#endif boost::any getter; @@ -82,28 +72,36 @@ namespace edmNew { typedef unsigned int id_type; struct Item { + Item(id_type i=0, int io=-1, size_type is=0) : id(i), offset(io), size(is){} - id_type id; -#ifdef USE_ATOMIC - // Item(Item&&)=default; Item & operator=(Item&& rh)=default; + Item(Item const & rh) noexcept : - id(rh.id),offset(rh.offset.load(std::memory_order_acquire)),size(rh.size) { + id(rh.id),offset(int(rh.offset)),size(rh.size) { } Item & operator=(Item const & rh) noexcept { - id=rh.id;offset=rh.offset.load(std::memory_order_acquire);size=rh.size; return *this; + id=rh.id;offset=int(rh.offset);size=rh.size; return *this; } Item(Item&& rh) noexcept : - id(std::move(rh.id)),offset(rh.offset.load(std::memory_order_acquire)),size(std::move(rh.size)) { + id(std::move(rh.id)),offset(int(rh.offset)),size(std::move(rh.size)) { } Item & operator=(Item&& rh) noexcept { - id=std::move(rh.id);offset=rh.offset.load(std::memory_order_acquire);size=std::move(rh.size); return *this; + id=std::move(rh.id);offset=int(rh.offset);size=std::move(rh.size); return *this; } + + id_type id; +#ifdef USE_ATOMIC std::atomic offset; + bool initialize() { + int expected = -1; + return offset.compare_exchange_strong(expected,-2); + } #else int offset; #endif size_type size; + bool uninitialized() const { return (-1)==offset;} + bool initializing() const { return (-2)==offset;} bool isValid() const { return offset>=0;} bool operator<(Item const &rh) const { return id::id_type id_type; typedef typename DetSetVector::size_type size_type; -#ifdef USE_ATOMIC + // here just to make the compiler happy static DetSetVector::Item & dummy() { + assert(false); static DetSetVector::Item d; return d; } FastFiller(DetSetVector & iv, id_type id, bool isaveEmpty=false) : @@ -209,11 +208,10 @@ namespace edmNew { v.pop_back(item.id); } assert(v.filling==true); - v.filling.store(false,std::memory_order_release); + v.filling=false; } -#endif void abort() { v.pop_back(item.id); @@ -286,9 +284,12 @@ namespace edmNew { typedef typename DetSetVector::size_type size_type; #ifdef USE_ATOMIC + // here just to make the compiler happy static DetSetVector::Item & dummy() { + assert(false); static DetSetVector::Item d; return d; } + // this constructor is not supposed to be used in Concurrent mode TSFastFiller(DetSetVector & iv, id_type id) : v(iv), item(v.ready()? v.push_back(id): dummy()) { assert(v.filling==true); v.filling = false;} @@ -298,17 +299,18 @@ namespace edmNew { } ~TSFastFiller() { bool expected=false; - while (!v.filling.compare_exchange_weak(expected,true,std::memory_order_acq_rel)) { expected=false; nanosleep(0,0);} + while (!v.filling.compare_exchange_weak(expected,true)) { expected=false; nanosleep(0,0);} int offset = v.m_data.size(); - if (v.m_data.capacity()0&& (*p).offset>-1 && + if ( (*p).size>0 && (*p).isValid() && m_data.size()==(*p).offset+(*p).size) m_data.resize((*p).offset); m_ids.erase( m_ids.begin()+(p-m_ids.begin())); @@ -501,7 +499,7 @@ namespace edmNew { bool isValid(id_type i) const { const_IdIter p = findItem(i); - return p!=m_ids.end() && (*p).offset!=-1; + return p!=m_ids.end() && (*p).isValid(); } /* @@ -651,23 +649,15 @@ namespace edmNew { template inline void DetSetVector::updateImpl(Item & item) { // no getter or already updated -/* - if (getter.empty()) assert(item.offset>=0); - if (item.offset!=-1 || getter.empty() ) return; - item.offset = int(m_data.size()); - FastFiller ff(*this,item,true); - (*boost::any_cast >(&getter))->fill(ff); -*/ - if (getter.empty()) { assert(item.offset>=0); return;} + if (getter.empty()) { assert(item.isValid()); return;} #ifdef USE_ATOMIC - int expected = -1; - if (item.offset.compare_exchange_strong(expected,-2,std::memory_order_acq_rel)) { - assert(item.offset==-2); + if (item.initialize() ){ + assert(item.initializing()); { TSFastFiller ff(*this,item); (*boost::any_cast >(&getter))->fill(ff); } - assert(item.offset>=0); + assert(item.isValid()); } #endif } @@ -678,15 +668,15 @@ namespace edmNew { typename Container::Item const & item, bool update) { #ifdef USE_ATOMIC // if an item is being updated we wait - if (update)icont.update(item); - while(item.offset.load(std::memory_order_acquire)<-1) nanosleep(0,0); + if (update) icont.update(item); + while(item.initializing()) nanosleep(0,0); bool expected=false; - while (!icont.filling.compare_exchange_weak(expected,true,std::memory_order_acq_rel)) { expected=false; nanosleep(0,0);} + while (!icont.filling.compare_exchange_weak(expected,true)) { expected=false; nanosleep(0,0);} #endif m_data=&icont.data(); #ifdef USE_ATOMIC - icont.filling.store(false,std::memory_order_release); + icont.filling=false; #endif m_id=item.id; m_offset = item.offset; diff --git a/DataFormats/Common/test/DetSetNewTS_t.cpp b/DataFormats/Common/test/DetSetNewTS_t.cpp index 41c619316e8e4..82f9772ca3929 100644 --- a/DataFormats/Common/test/DetSetNewTS_t.cpp +++ b/DataFormats/Common/test/DetSetNewTS_t.cpp @@ -187,9 +187,9 @@ void TestDetSet::fillSeq() { { sync(lock); while(true) { - int ldet = idet.load(std::memory_order_acquire); + int ldet = idet; if (!(ldet=maxDet) break; unsigned int id=20+ldet; bool done=false; @@ -205,7 +205,7 @@ void TestDetSet::fillSeq() { // read(detsets); // cannot read in parallel while filling in this case done=true; } catch (edm::Exception const&) { - trial.fetch_add(1,std::memory_order_acq_rel); + trial++;; //read(detsets); } } @@ -247,6 +247,7 @@ void TestDetSet::fillPar() { int maxDet=100*nth; std::vector v(maxDet); int k=20;for (auto &i:v) i=k++; DSTV detsets(pg,v,2); + detsets.reserve(maxDet,100*maxDet); CPPUNIT_ASSERT(g.ntot==0); CPPUNIT_ASSERT(detsets.onDemand()); CPPUNIT_ASSERT(maxDet==int(detsets.size())); diff --git a/DataFormats/Common/test/DetSetNew_t.cpp b/DataFormats/Common/test/DetSetNew_t.cpp index eb1a1a0524a28..15d374376a437 100644 --- a/DataFormats/Common/test/DetSetNew_t.cpp +++ b/DataFormats/Common/test/DetSetNew_t.cpp @@ -253,6 +253,7 @@ void TestDetSet::filling() { void TestDetSet::fillingTS() { DSTV detsets(2); + detsets.reserve(5,100); unsigned int ntot=0; for (unsigned int n=1;n<5;++n) { unsigned int id=20+n;