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

[David Blaikie]

On Feb 20, 2015 5:28 AM, "Erik Eckstein" <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
>
> 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/
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> 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/20150220/a562d53f/attachment.html>