On Thu, Mar 6, 2014 at 6:08 PM, Chris Jones <notifications@github.com>wrote:

> In EventFilter/Utilities/plugins/FastMonitoringService.cc:
>
> > +    //make sure that all path names are retrieved before allowing ministate to change
> > +    //hack: assume memory is synchronized after ~50 events seen by each stream
> > +    if (unlikely(eventCountForPathInit_[sc.streamID()]<50) && false==collectedPathList_[sc.streamID()]->load(std::memory_order_acquire))
> > +    {
> > +      initPathsLock_.lock();
> > +      if (firstEventId_[sc.streamID()]==0)
> > +	firstEventId_[sc.streamID()]=sc.eventID().event();
> > +      if (sc.eventID().event()==firstEventId_[sc.streamID()])
> > +      {
> > +	encPath_[sc.streamID()].update((void*)&pc.pathName());
> > +	initPathsLock_.unlock();
> > +	return;
> > +      }
> > +      else {
> > +	collectedPathList_[sc.streamID()]->store(true,std::memory_order_seq_cst);
> > +        fmt_.m_data.ministateBins_=encPath_[sc.streamID()].vecsize();
>
> From my own personal experience (yours may differ), I have found that
> using mutex to be extremely error prone. Instead I break code into tasks
> which can be either parallel or serial and I use the Intel Thread Building
> block data structures in the case where I have a container accessed by
> multiple threads. Using the TBB containers, I was able to make the entire
> MessageLogger thread safe and lock free.
>

I agree that using tbb structures could be useful here (in particular the
TBB concurrent_hash_map), even if we don't get rid of mutexes in all places.

As your expert eye quickly noticed quite a few issues, I agree that we need
to work on this more and do a careful review. Therefore I will close the
pull request for now.

Thanks for your review!

Srecko


>  --
> Reply to this email directly or view it on GitHub<https://github.com/cms-sw/cmssw/pull/2661/files#r10350230>
> .
>