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