[PATCH] Add a lock() function in PassRegistry to speed up multi-thread synchronization.

[Erik Eckstein]

Thanks for your comment!
I updated the patch.

> On 20 Feb 2015, at 17:05, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> On Feb 20, 2015 5:28 AM, "Erik Eckstein" <eeckstein at apple.com <mailto:eeckstein at apple.com>> wrote:
> >
> > With this patch I'm trying to solve the following problem:
> >
> > I'm running llvm in multiple threads (each instance uses its private LLVMContext).
> > With a release-asserts build of llvm I got terrible multi-thread performance. I found that the problem is the mutex in PassRegistry::getPassInfo(), mainly called from PMDataManager::verifyPreservedAnalysis().
> >
> > My first idea was to make an option to disable verifyPreservedAnalysis(), even in an asserts-build.
> > But I didn't like it because I don't want to give up this verification and the mutex also cause overhead outside verifyPreservedAnalysis().
> >
> > So I did the following:
> > I added a lock() function in PassRegistry which I call when I'm sure that all passes are registered.
> > After the PassRegistry is locked it can safely access its maps without using a mutex.
> >
> > This completely solves the multi-thread performance problem.
> > The use of the lock() function is optional. If not used, nothing changes.
> >
> > http://reviews.llvm.org/D7787 <http://reviews.llvm.org/D7787>
> >
> > Files:
> >   include/llvm/PassRegistry.h
> >   lib/IR/PassRegistry.cpp
> >
> > Index: include/llvm/PassRegistry.h
> > ===================================================================
> > --- include/llvm/PassRegistry.h
> > +++ include/llvm/PassRegistry.h
> > @@ -41,6 +41,9 @@
> >  class PassRegistry {
> >    mutable sys::SmartRWMutex<true> Lock;
> >
> > +  /// Only if false, synchronization must use the Lock mutex.
> > +  std::atomic<bool> locked;
> > +
> >    /// PassInfoMap - Keep track of the PassInfo object for each registered pass.
> >    typedef DenseMap<const void *, const PassInfo *> MapType;
> >    MapType PassInfoMap;
> > @@ -52,7 +55,7 @@
> >    std::vector<PassRegistrationListener *> Listeners;
> >
> >  public:
> > -  PassRegistry() {}
> > +  PassRegistry() : locked(false) {}
> >    ~PassRegistry();
> >
> >    /// getPassRegistry - Access the global registry object, which is
> > @@ -60,6 +63,10 @@
> >    /// llvm_shutdown.
> >    static PassRegistry *getPassRegistry();
> >
> > +  /// Enables fast thread synchronization in getPassInfo().
> > +  /// After calling lock() no more passes may be registered.
> > +  void lock() { locked = true; }
> > +
> >    /// getPassInfo - Look up a pass' corresponding PassInfo, indexed by the pass'
> >    /// type identifier (&MyPass::ID).
> >    const PassInfo *getPassInfo(const void *TI) const;
> > Index: lib/IR/PassRegistry.cpp
> > ===================================================================
> > --- lib/IR/PassRegistry.cpp
> > +++ lib/IR/PassRegistry.cpp
> > @@ -39,15 +39,29 @@
> >  PassRegistry::~PassRegistry() {}
> >
> >  const PassInfo *PassRegistry::getPassInfo(const void *TI) const {
> > -  sys::SmartScopedReader<true> Guard(Lock);
> > -  MapType::const_iterator I = PassInfoMap.find(TI);
> > -  return I != PassInfoMap.end() ? I->second : nullptr;
> > +  if (locked) {
> > +    // We don't need thread synchronization because the PassInfoMap is read-only
> > +    // after the PassRegistry is locked.
> > +    MapType::const_iterator I = PassInfoMap.find(TI);
> > +    return I != PassInfoMap.end() ? I->second : nullptr;
> > +  } else {
> 
> Else after return (& another way Tod o this might be to put the guard in an Optional object and then just share the actual lookup code)
> 
> > +    sys::SmartScopedReader<true> Guard(Lock);
> > +    MapType::const_iterator I = PassInfoMap.find(TI);
> > +    return I != PassInfoMap.end() ? I->second : nullptr;
> > +  }
> >  }
> >
> >  const PassInfo *PassRegistry::getPassInfo(StringRef Arg) const {
> > -  sys::SmartScopedReader<true> Guard(Lock);
> > -  StringMapType::const_iterator I = PassInfoStringMap.find(Arg);
> > -  return I != PassInfoStringMap.end() ? I->second : nullptr;
> > +  if (locked) {
> > +    // We don't need thread synchronization because the PassInfoStringMap is
> > +    // read-only after the PassRegistry is locked.
> > +    StringMapType::const_iterator I = PassInfoStringMap.find(Arg);
> > +    return I != PassInfoStringMap.end() ? I->second : nullptr;
> > +  } else {
> > +    sys::SmartScopedReader<true> Guard(Lock);
> > +    StringMapType::const_iterator I = PassInfoStringMap.find(Arg);
> > +    return I != PassInfoStringMap.end() ? I->second : nullptr;
> > +  }
> >  }
> >
> >  //===----------------------------------------------------------------------===//
> > @@ -55,6 +69,9 @@
> >  //
> >
> >  void PassRegistry::registerPass(const PassInfo &PI, bool ShouldFree) {
> > +
> > +  assert(!locked && "Trying to register a pass in a locked PassRegistry");
> > +
> >    sys::SmartScopedWriter<true> Guard(Lock);
> >    bool Inserted =
> >        PassInfoMap.insert(std::make_pair(PI.getTypeInfo(), &PI)).second;
> > @@ -68,6 +85,8 @@
> >
> >    if (ShouldFree)
> >      ToFree.push_back(std::unique_ptr<const PassInfo>(&PI));
> > +
> > +  assert(!locked && "PassRegistry locked during registering a pass");
> >  }
> >
> >  void PassRegistry::enumerateWith(PassRegistrationListener *L) {
> >
> > EMAIL PREFERENCES
> >   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
> >

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150226/60c159c0/attachment.html>