[PATCH] Add a lock() function in PassRegistry to speed up multi-thread synchronization.
David Blaikie
dblaikie at gmail.com
Thu Feb 26 14:41:32 PST 2015
I was thinking something more like this (attached) - an optional scoped
device. (at some point we'll probably just want to make the lock holding
devices movable, but it's not needed in this case because Optional::emplace
suffices)
Also, <atomic> was missing from PassRegistry.h
On Thu, Feb 26, 2015 at 4:36 AM, Erik Eckstein <eeckstein at apple.com> wrote:
> 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> 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/20150226/ff888029/attachment.html>
-------------- next part --------------
diff --git include/llvm/PassRegistry.h include/llvm/PassRegistry.h
index 8c28ef5..97e2bde 100644
--- include/llvm/PassRegistry.h
+++ include/llvm/PassRegistry.h
@@ -25,6 +25,7 @@
#include "llvm/PassInfo.h"
#include "llvm/Support/CBindingWrapping.h"
#include "llvm/Support/RWMutex.h"
+#include <atomic>
#include <vector>
namespace llvm {
@@ -41,6 +42,9 @@ struct PassRegistrationListener;
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 +56,7 @@ class PassRegistry {
std::vector<PassRegistrationListener *> Listeners;
public:
- PassRegistry() {}
+ PassRegistry() : locked(false) {}
~PassRegistry();
/// getPassRegistry - Access the global registry object, which is
@@ -60,6 +64,10 @@ public:
/// 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;
diff --git lib/IR/PassRegistry.cpp lib/IR/PassRegistry.cpp
index b879fef..df71666 100644
--- lib/IR/PassRegistry.cpp
+++ lib/IR/PassRegistry.cpp
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/PassRegistry.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/IR/Function.h"
#include "llvm/PassSupport.h"
#include "llvm/Support/Compiler.h"
@@ -39,13 +40,23 @@ PassRegistry *PassRegistry::getPassRegistry() {
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;
}
@@ -55,6 +66,9 @@ const PassInfo *PassRegistry::getPassInfo(StringRef Arg) const {
//
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 +82,8 @@ void PassRegistry::registerPass(const PassInfo &PI, bool ShouldFree) {
if (ShouldFree)
ToFree.push_back(std::unique_ptr<const PassInfo>(&PI));
+
+ assert(!locked && "PassRegistry locked during registering a pass");
}
void PassRegistry::enumerateWith(PassRegistrationListener *L) {
More information about the llvm-commits
mailing list