Skip to content

Commit

Permalink
Removed inappropriate use of SerialTaskQueue from ClusterShapeLazyGetter
Browse files Browse the repository at this point in the history
A SerialTaskQueue was added to ClusterShapeLazyGetter in an attempt
to make it thread safe. Unfortunately, the implementation was not safe
since the internals of SiPixelClusterShapeCache could be changed
while another thread was reading the data outside of the queue.
In addition, performance measurements done using VTune showed
the fequent calls to SerialTaskQueue were causing a heavy CPU load
when run with 8 threads.
The thread safety of SiPixelClusterShapeCache will have to be handled
in a different way in the future once we start using more than one
thread per Event.
  • Loading branch information
Dr15Jones committed Oct 14, 2015
1 parent b4b632c commit 331305a
Showing 1 changed file with 6 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/EventSetup.h"
#include "FWCore/Utilities/interface/EDGetToken.h"
#include "FWCore/Concurrency/interface/SerialTaskQueue.h"

#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Utilities/interface/InputTag.h"
Expand Down Expand Up @@ -31,18 +30,15 @@ namespace {
~ClusterShapeLazyGetter() {}

void fill(const SiPixelClusterShapeCache::ClusterRef& cluster, const PixelGeomDetUnit *pixDet, const SiPixelClusterShapeCache& constCache) const override {
taskQueue_.pushAndWait([this, &cluster, pixDet, &constCache]{
if(constCache.isFilled(cluster))
return;
SiPixelClusterShapeCache& cache = const_cast<SiPixelClusterShapeCache&>(constCache);
this->data_.size.clear();
this->clusterShape_.determineShape(*pixDet, *cluster, this->data_);
cache.insert(cluster, this->data_);
});
if(constCache.isFilled(cluster))
return;
SiPixelClusterShapeCache& cache = const_cast<SiPixelClusterShapeCache&>(constCache);
this->data_.size.clear();
this->clusterShape_.determineShape(*pixDet, *cluster, this->data_);
cache.insert(cluster, this->data_);
}

private:
mutable edm::SerialTaskQueue taskQueue_; // not sure if this is the best synchronization mechanism
mutable ClusterData data_; // reused
mutable ClusterShape clusterShape_;
};
Expand Down

0 comments on commit 331305a

Please sign in to comment.