Skip to content

Commit

Permalink
Merge pull request #7332 from Dr15Jones/fixThreadSafetyOfMVAComputer
Browse files Browse the repository at this point in the history 
Fix thread safety problem in PhysicsTools/MVAComputer
  • Loading branch information
@ktf
ktf committed Jan 24, 2015
2 parents bf44b7e + f5ce02f commit 8f2894c
Show file tree
Hide file tree
Showing 11 changed files with 307 additions and 33 deletions.
1 change: 1 addition & 0 deletions PhysicsTools/MVAComputer/BuildFile.xml
View file
Expand Up @@ -5,6 +5,7 @@
<use name="rootcore"/>
<use name="boost"/>
<use name="zlib"/>
<use name="tbb"/>
<export>
<lib name="1"/>
</export>
6 changes: 4 additions & 2 deletions PhysicsTools/MVAComputer/interface/MVAComputer.h
View file
Expand Up @@ -139,8 +139,9 @@ class MVAComputer {

inline void eval(const VarProcessor *proc, int *outConf,
double *output, int *loop,
VarProcessor::LoopCtx& ctx,
unsigned int offset, unsigned int out) const
{ proc->eval(values_, conf_, output, outConf, loop, offset); }
{ proc->eval(values_, conf_, output, outConf, loop, ctx, offset); }

inline double output(unsigned int output) const
{ return values_[conf_[output]]; }
Expand All @@ -158,7 +159,8 @@ class MVAComputer {
DerivContext() : n_(0) {}

void eval(const VarProcessor *proc, int *outConf,
double *output, int *loop,
double *output, int *loop,
VarProcessor::LoopCtx& ctx,
unsigned int offset, unsigned int out) const;

double output(unsigned int output,
Expand Down
4 changes: 2 additions & 2 deletions PhysicsTools/MVAComputer/interface/ProcessRegistry.h
View file
Expand Up @@ -18,7 +18,7 @@


#include <string>
#include <map>
#include "tbb/concurrent_unordered_map.h"



Expand Down Expand Up @@ -92,7 +92,7 @@ namespace PhysicsTools
const ProcessRegistry *process);
static void unregisterProcess(const char *name);

typedef std::map<std::string, const ProcessRegistry*> RegistryMap;
typedef tbb::concurrent_unordered_map<std::string, const ProcessRegistry*> RegistryMap;

/// return map of all registered processes, allocate if necessary
static RegistryMap *getRegistry();
Expand Down
6 changes: 3 additions & 3 deletions PhysicsTools/MVAComputer/interface/ProcessRegistry.icc
View file
Expand Up @@ -24,10 +24,10 @@ Base_t *ProcessRegistry<Base_t, CalibBase_t, Parent_t>::Factory::create(
{ return ProcessRegistry::create(name, calib, parent); }

template<class Base_t, class CalibBase_t, class Parent_t>
std::map<std::string, const ProcessRegistry<Base_t, CalibBase_t, Parent_t>*>
tbb::concurrent_unordered_map<std::string, const ProcessRegistry<Base_t, CalibBase_t, Parent_t>*>
*ProcessRegistry<Base_t, CalibBase_t, Parent_t>::getRegistry()
{
static struct Sentinel {
[[cms::thread_safe]] static struct Sentinel {
Sentinel() : instance(new RegistryMap) {}
~Sentinel() { delete instance; instance = 0; }

Expand Down Expand Up @@ -57,7 +57,7 @@ void ProcessRegistry<Base_t, CalibBase_t, Parent_t>::registerProcess(
template<class Base_t, class CalibBase_t, class Parent_t>
void ProcessRegistry<Base_t, CalibBase_t, Parent_t>::unregisterProcess(
const char *name)
{ RegistryMap *map = getRegistry(); if (map) map->erase(name); }
{ RegistryMap *map = getRegistry(); if (map) map->unsafe_erase(name); }

} // namespace PhysicsTools

Expand Down
35 changes: 30 additions & 5 deletions PhysicsTools/MVAComputer/interface/VarProcessor.h
View file
Expand Up @@ -86,6 +86,27 @@ class VarProcessor :
Context *ctx;
};


/** \class LoopCtx
*
* \short Hold context information for looping processors
*
* VarProcessor instances which allow looping need to keep
* track of the state of the loop between calls.
*
************************************************************/
class LoopCtx {
public:
LoopCtx(): index_(0), offset_(0), size_(0) {}
inline unsigned int& index() { return index_;}
inline unsigned int& offset() { return offset_;}
inline unsigned int& size() { return size_;}
private:
unsigned int index_;
unsigned int offset_;
unsigned int size_ ;
};

virtual ~VarProcessor();

/// called from the discriminator computer to configure processor
Expand All @@ -94,22 +115,23 @@ class VarProcessor :
/// run the processor evaluation pass on this processor
inline void
eval(double *input, int *conf, double *output, int *outConf,
int *loop, unsigned int offset) const
int *loop, LoopCtx& loopCtx, unsigned int offset) const
{
ValueIterator iter(inputVars.iter(), input, conf,
output, outConf, loop, offset);
output, outConf, loop, loopCtx, offset);
eval(iter, nInputVars);
}

/// run the processor evaluation pass on this processor and compute derivatives
void deriv(double *input, int *conf, double *output, int *outConf,
int *loop, unsigned int offset, unsigned int in,
int *loop, LoopCtx& ctx, unsigned int offset, unsigned int in,
unsigned int out, std::vector<double> &deriv) const;

enum LoopStatus { kStop, kNext, kReset, kSkip };

virtual LoopStatus loop(double *output, int *outConf,
unsigned int nOutput,
LoopCtx& ctx,
unsigned int &nOffset) const
{ return kStop; }

Expand Down Expand Up @@ -219,6 +241,8 @@ class VarProcessor :
/// test for end of input variable iterator
inline operator bool() const { return cur; }

inline LoopCtx& loopCtx() { return ctx;}

/// move to next input variable
ValueIterator &operator ++ ()
{
Expand All @@ -244,8 +268,8 @@ class VarProcessor :

ValueIterator(BitSet::Iterator cur, double *values,
int *conf, double *output, int *outConf,
int *loop, unsigned int offset) :
cur(cur), offset(offset), start(values + offset),
int *loop, LoopCtx& ctx, unsigned int offset) :
cur(cur), ctx(ctx), offset(offset), start(values + offset),
values(values), conf(conf), loop(loop),
output(output + offset), outConf(outConf)
{
Expand All @@ -259,6 +283,7 @@ class VarProcessor :

private:
BitSet::Iterator cur;
LoopCtx& ctx;
const unsigned int offset;
double *const start;
double *values;
Expand Down
8 changes: 3 additions & 5 deletions PhysicsTools/MVAComputer/src/AtomicId.cc
View file
Expand Up @@ -23,14 +23,12 @@ namespace { // anonymous
private:
typedef std::multiset<const char *, StringLess> IdSet;

IdSet idSet;
static std::allocator<char> stringAllocator;
mutable std::mutex mutex;
IdSet idSet;
std::allocator<char> stringAllocator;
std::mutex mutex;
};
} // anonymous namespace

std::allocator<char> IdCache::stringAllocator;

IdCache::~IdCache()
{
for(std::multiset<const char*, StringLess>::iterator iter =
Expand Down
9 changes: 6 additions & 3 deletions PhysicsTools/MVAComputer/src/MVAComputer.cc
View file
Expand Up @@ -176,6 +176,7 @@ void MVAComputer::evalInternal(T &ctx) const
int *loopOutConf = outConf;
int *loopStart = 0;
double *loopOutput = output;
VarProcessor::LoopCtx loopCtx;

VarProcessor::LoopStatus status = VarProcessor::kNext;
unsigned int offset = 0;
Expand All @@ -192,6 +193,7 @@ void MVAComputer::evalInternal(T &ctx) const
if (status != VarProcessor::kSkip)
ctx.eval(&*iter->processor, outConf, output,
loopStart ? loopStart : loopOutConf,
loopCtx,
offset, iter->nOutput);

#ifdef DEBUG_EVAL
Expand All @@ -210,7 +212,7 @@ void MVAComputer::evalInternal(T &ctx) const
#endif

status = loop->processor->loop(output, outConf,
nextOutput, offset);
nextOutput, loopCtx, offset);

if (status == VarProcessor::kReset) {
outConf = loopOutConf;
Expand Down Expand Up @@ -309,10 +311,11 @@ void MVAComputer::writeCalibration(std::ostream &os,

void MVAComputer::DerivContext::eval(
const VarProcessor *proc, int *outConf, double *output,
int *loop, unsigned int offset, unsigned int out) const
int *loop, VarProcessor::LoopCtx& ctx,
unsigned int offset, unsigned int out) const
{
proc->deriv(values(), conf(), output, outConf,
loop, offset, n(), out, deriv_);
loop, ctx, offset, n(), out, deriv_);
}

double MVAComputer::DerivContext::output(unsigned int output,
Expand Down
21 changes: 10 additions & 11 deletions PhysicsTools/MVAComputer/src/ProcForeach.cc
View file
Expand Up @@ -44,6 +44,7 @@ class ProcForeach : public VarProcessor {
ValueIterator iter, unsigned int n) const override;
virtual LoopStatus loop(double *output, int *conf,
unsigned int nOutput,
LoopCtx& ctx,
unsigned int &nOffset) const override;

private:
Expand All @@ -56,12 +57,6 @@ class ProcForeach : public VarProcessor {
unsigned int count;
};

inline void reset() const { index = offset = size = 0; }

mutable unsigned int index;
mutable unsigned int offset;
mutable unsigned int size;

unsigned int count;
};

Expand All @@ -77,7 +72,6 @@ ProcForeach::ProcForeach(const char *name,

void ProcForeach::configure(ConfIterator iter, unsigned int n)
{
reset();
iter << Variable::FLAG_NONE;
while(iter)
iter << iter++(Variable::FLAG_MULTIPLE);
Expand All @@ -104,12 +98,14 @@ ProcForeach::configureLoop(ConfigCtx::Context *ctx_, ConfigCtx::iterator begin,

void ProcForeach::eval(ValueIterator iter, unsigned int n) const
{
auto const offset = iter.loopCtx().offset();
iter(offset);

auto& loopSize = iter.loopCtx().size();
while(iter) {
unsigned int size = iter.size();
if (!this->size)
this->size = size;
if (!loopSize)
loopSize = size;

double value = iter[offset];
iter(value);
Expand All @@ -120,6 +116,7 @@ void ProcForeach::eval(ValueIterator iter, unsigned int n) const
std::vector<double> ProcForeach::deriv(
ValueIterator iter, unsigned int n) const
{
auto const offset = iter.loopCtx().offset();
std::vector<unsigned int> offsets;
unsigned int in = 0, out = 0;
while(iter) {
Expand All @@ -137,13 +134,16 @@ std::vector<double> ProcForeach::deriv(

VarProcessor::LoopStatus
ProcForeach::loop(double *output, int *conf,
unsigned int nOutput, unsigned int &nOffset) const
unsigned int nOutput, LoopCtx& ctx, unsigned int &nOffset) const
{
auto& index = ctx.index();
bool endIteration = false;
if (index++ == count) {
index = 0;
endIteration = true;
}
auto& offset = ctx.offset();
auto& size = ctx.size();

if (offset == 0 && !endIteration) {
for(int cur = *conf + size; nOutput--; cur += size)
Expand All @@ -152,7 +152,6 @@ ProcForeach::loop(double *output, int *conf,

if (endIteration) {
if (++offset >= size) {
reset();
return kStop;
} else
return kReset;
Expand Down
5 changes: 3 additions & 2 deletions PhysicsTools/MVAComputer/src/VarProcessor.cc
View file
Expand Up @@ -119,12 +119,13 @@ VarProcessor *ProcessRegistry<VarProcessor, Calibration::VarProcessor,
}

void VarProcessor::deriv(double *input, int *conf, double *output,
int *outConf, int *loop, unsigned int offset,
int *outConf, int *loop, LoopCtx& ctx,
unsigned int offset,
unsigned int in, unsigned int out_,
std::vector<double> &deriv) const
{
ValueIterator iter(inputVars.iter(), input, conf,
output, outConf, loop, offset);
output, outConf, loop, ctx, offset);

eval(iter, nInputVars);

Expand Down
3 changes: 3 additions & 0 deletions PhysicsTools/MVAComputer/test/BuildFile.xml
View file
Expand Up @@ -17,3 +17,6 @@
<use name="CondCore/PluginSystem"/>
<flags EDM_PLUGIN="1"/>
</library>
<bin name="testPhysicsToolsMVAComputer" file="testMVAComputer.cppunit.cc">
<use name="cppunit"/>
</bin>

0 comments on commit 8f2894c

Please sign in to comment.