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

[eeckstein]

Summary

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.

[resistor]

Hi Erik,

[eeckstein]

Hi Owen, Chandler,

Do you mean with a release *+* asserts build?

Yes, sorry I wrote it wrong.

I think my initial description was not very good. I'll try to explailn again: there are two issues

1. multithread-performance problem in the assert build: I understand that assert builds are slower than release builds, but in this case it's a factor of 4 when running with 4 threads, i.e. the multithreaded version is slower than the single-threaded version! This makes the asserts build useless for my purpose.

2. Even in the release build there is a performance penalty of about 5% because of the mutex.

With my proposed patch both problems would be solved.

[mehdi_amini]

lib/IR/PassRegistry.cpp

52

It is unfortunate to lose RAII, is there a good reason why SmartScopedReader cannot take an extra parameter to be constructed without locking? (like std::unique_lock())

[eeckstein]

New version using llvm::Optional

[mehdi_amini]

The patch LGTM.
However Chandler was not convinced in the first place, and asked "Do you have a test case that shows a severe problem without asserts?".
You are mentioning 5% perf improvement in single-threaded release build, can you tell how to reproduce?

lib/IR/PassRegistry.cpp

46

"Pedantic" comment: since your motivation is purely performance here, you may want to replace all uses of `locked`` with `locked.load(memory_order_consume)``. Maybe in a separate private method like that:

bool isLocked() { return locked.load(memory_order_consume); }

Especially since I don't think there is a guarantee that atomic<bool> is lock_free.

[eeckstein]

You are mentioning 5% perf improvement in single-threaded release build, can you tell how to reproduce?

It is reproducible in following scenario:

4 threads on a 3,2 GHz Intel Core i5

A shared working queue contains ~100 independent llvm modules (each llvm module has its own LLVMContext).
Each thread runs following loop:

while queue is not empty {

fetch an llvm module from the working queue
create llvm passes
call PassRegistry::lock()
run the llvm passes

}

[eeckstein]

Committed in r231276.

Thanks all for reviewing!

lib/IR/PassRegistry.cpp

46

Thanks for this suggestion. But I'm not sure if memory_order_consume is OK here. I think it must be memory_order_acquire.
I prefer to be conservative in this case and let it as it is now.

include/llvm/PassRegistry.h

 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;

   std::vector<PassRegistrationListener *> Listeners;
 
 public:
-  PassRegistry() {}
+  PassRegistry() : locked(false) {}
   ~PassRegistry();
 
   /// getPassRegistry - Access the global registry object, which is

   /// 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;

lib/IR/PassRegistry.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/PassRegistry.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/IR/Function.h"
 #include "llvm/PassSupport.h"
 #include "llvm/Support/Compiler.h"

 PassRegistry::~PassRegistry() {}
 
 const PassInfo *PassRegistry::getPassInfo(const void *TI) const {
-  sys::SmartScopedReader<true> Guard(Lock);
+  // We don't need thread synchronization after the PassRegistry is locked
+  // (that means: is read-only).
+  Optional<sys::SmartScopedReader<true>> Guard;
+  if (!locked)
+    Guard.emplace(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);
+  // We don't need thread synchronization after the PassRegistry is locked
+  // (that means: is read-only).
+  Optional<sys::SmartScopedReader<true>> Guard;
+  if (!locked)
+    Guard.emplace(Lock);
+
   StringMapType::const_iterator I = PassInfoStringMap.find(Arg);
   return I != PassInfoStringMap.end() ? I->second : nullptr;
 }

 //
 
 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;

 
   if (ShouldFree)
     ToFree.push_back(std::unique_ptr<const PassInfo>(&PI));
+
+  assert(!locked && "PassRegistry locked during registering a pass");
 }
 
 void PassRegistry::enumerateWith(PassRegistrationListener *L) {