<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Jun 7, 2014 at 5:47 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+chandlerc, aaronballman, in case there are additional carryovers and/or issues from the review thread which I've left out.</div>
<div><br></div><div><br></div>I have a patch up for review[1, 2] that attempts to replace LLVM's mutex implementation with std::mutex and std::recursive_mutex. While the patch seems to work, there are questions surrounding whether or not the approach used is correct. <div>
<br></div><div>I'll try to summarize the issues as best I can, in hopes of getting some feedback from a broader audience, to make sure this is done correctly:</div><div><br></div><div>1) Should support multi-threading be a compile-time or runtime parameter in LLVM? </div>
<div><br></div><div>Currently it is both. It is compile-time through the use of the define LLVM_ENABLE_THREADS, and it is runtime through the use of functions llvm_start_multithreaded, llvm_is_multithreaded, etc. I and some others feel like runtime support for multi-threading could be removed, and it should be compile-time only. However, I am not aware of all the ways in which this is being used, so this is where I would like some feedback. The issues I have with runtime multithreading support are the following:</div>
<div><br></div><div>* It leads to confusing code. At any given point, is multi-threading enabled or disabled? You never know without calling llvm_is_multithreaded, but even calling that is inherently racy, because someone else could disable it after it returns.</div>
<div><br></div><div>* It leads to subtle bugs. clang_createIndex, the first time it's called, enables multi-threading. What happens if someone else disables it later? Things like this shouldn't even be possible. </div>
<div><br></div><div>* Not all platforms even support threading to begin with. This works now because llvm_start_multithreaded(), if the compile time flag is set to disable threads, simply does nothing. But this decision should be made by someone else. Nobody really checks the return value from llvm_start_multithreaded anyway, so there's already probably bugs where someone tries to start multi-threading, and it fails.</div>
<div><br></div><div>* What does it actually mean to turn multi-threading support on and off?</div><div><br></div><div>Anybody that tries to do this is almost certainly broken due to some edge cases about when it's on and when it's off. So this goes back to the first two points about confusing code and subtle bugs.</div>
</div></blockquote><div><br></div><div>+1</div><div>I am in favor of removing the run-time check. We've seen lots of code like this "if (have_threads) mu->lock();", and it was very often broken.</div><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div>2) What should happen when you try to acquire a mutex in an app with threading disabled?</div><div><br></div><div>If this is a compile-time parameter, the solution is simple: make an empty mutex class that satisfies the Lockable concept, and have its methods do nothing. Then typedef something like llvm::mutex to be either std::mutex or llvm::null_mutex accordingly. If this is a runtime parameter, it's more complicated, and we should ask ourselves whether or not it's necessary to avoid the overhead of acquiring an uncontended mutex in single threaded apps. For what it's worth, all reasonable STL implementations will use a lightweight mutex implementation, which generally require less than 100 nanoseconds to acquire uncontended, so I think we don't need to care, but again some feedback would be nice.</div>
<div><br></div><div>3) We have some debug code in our mutex implementation that is intended to try to help catch deadlocks and/or race conditions. Do we need this code?</div><div><br></div><div>I think we can get by without it, but again some feedback about how, if at all, people are using this would be nice. For example, if you want to detect deadlocks and race conditions you can use TSan. </div>
</div></blockquote><div><br></div><div>The problem with race detection is that it slows down the process, but I doubt that a debug code in mutex can catch (many) races.</div><div><br></div><div>As for the deadlocks, indeed it is possible to add deadlock detection directly to std::mutex and std::spinlock code. </div>
<div>It may even end up being more efficient than a standalone deadlock detector -- </div><div>but only if we can add an extra word to the mutex/spinlock object.</div><div>The deadlock detector's overhead comes primarily from two tasks: </div>
<div> 1. get the metadata for the lock in question. </div><div> 2. query the lock-acquisition-order graph to see if there is a loop. </div><div><br></div><div>If the lock-acquisition-order graph is sparse (99% of cases we've seen), then the task #1 may constitute up to 50% of the overhead.</div>
<div>If we can add a word to std::mutex/std::spinlock data structures then the task #1 becomes trivial.</div><div>If we can't add a word for some reason, then I see no benefit from adding debug mode directly to STL compared to a standalone deadlock detector. <br>
</div><div><br></div><div>(FTR: our deadlock detector in tsan: <a href="https://code.google.com/p/thread-sanitizer/wiki/DeadlockDetector">https://code.google.com/p/thread-sanitizer/wiki/DeadlockDetector</a>)</div><div><br>
</div><div>--kcc </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div>Thanks!</div><div>Zach</div><div><br></div><div><br></div><div>[1] - <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220051.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220051.html</a></div>
<div>[2] - <a href="http://reviews.llvm.org/D4033" target="_blank">http://reviews.llvm.org/D4033</a></div></div>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div></div>