<div dir="ltr">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)<br><br>Also, <atomic> was missing from PassRegistry.h</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 26, 2015 at 4:36 AM, Erik Eckstein <span dir="ltr"><<a href="mailto:eeckstein@apple.com" target="_blank">eeckstein@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Thanks for your comment!<div>I updated the patch.</div><div><div class="h5"><div><br><div><blockquote type="cite"><div>On 20 Feb 2015, at 17:05, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><p dir="ltr"><br>
On Feb 20, 2015 5:28 AM, "Erik Eckstein" <<a href="mailto:eeckstein@apple.com" target="_blank">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" target="_blank">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/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</p>
</div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>