This is an archive of the discontinued LLVM Phabricator instance.

Change base mutex implementations to use STL-provided mutexes
AbandonedPublic

Authored by zturner on Jun 5 2014, 10:06 AM.

Details

Summary

This is a re-submission of an earlier broken patch.

Change llvm::sys::Mutex implementation to use std::mutex and std::recursive_mutex.

This patch deletes the platform specific mutex implementations, and delegates to the underlying STL implementation. Because STL mutex and recursive_mutex are implemented as separate classes, it is no longer possible to specify as a runtime param whether a recursive mutex is desired, and instead the decision must be made at compile time.

As part of this refactor, the ScopedLock class is deleted in favor of MutexGuard, which provides equivalent functionality.

To make reviewing easier, you can refer to the following table of equivalences in the format (old_type -> new_type)

SmartMutex<true>([true]) -> RecursiveDebugMutex

SmartMutex<false>([true]) -> RecursiveMutex
sys::Mutex -> SmartMutex<false>() -> RecursiveMutex

SmartMutex<true>(false) -> NonRecursiveDebugMutex
SmartMutex<false>(false) -> NonRecursiveMutex

One caveat of note is that on Windows std::*mutex cannot be used during an atexit handler. To get around this, a new type AtexitSafeMutex is defined, and on Windows this is typedefed to a critical section.

Diff Detail

Event Timeline

zturner updated this revision to Diff 10143.Jun 5 2014, 10:06 AM
zturner retitled this revision from to Change base mutex implementations to use STL-provided mutexes.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: rnk, aaron.ballman, chandlerc.
zturner added a subscriber: Unknown Object (MLST).
aaron.ballman edited edge metadata.Jun 5 2014, 12:27 PM

Is there a clang companion patch as well?

~Aaron

Yes, but that one hasn't changed since the last time so I didn't bother re-uploading it. The old one is still up. It's mentioned in the Testing section, but it's D3961

Also make sure you're clang repo is up-to-date at least to r210225. That fixes the test failures we saw last time.

zturner updated this revision to Diff 10195.Jun 6 2014, 4:34 PM
zturner edited edge metadata.

Remove the Mutex.h dependency on windows.h, and address various other review comments.

This solution is still less than ideal, because we need to include some windows preprocessor checks inside of Mutex.h. The best solution in my opinion would require allowing some platform specific headers to be in the include tree (for example, include\llvm\Support\Windows\CriticalSectionMutex.h). This would remove all of the preprocessor code from Mutex.h, and allow anyone who needs this to simply write #include "llvm/Support/Windows/CriticalSectionMutex.h".

chandlerc edited edge metadata.Jun 6 2014, 6:06 PM

Backing up a second, and setting aside all aspects of windows.h, I think
this patch is going in the wrong direction at a very fundamental level.

I think it is a really huge mistake for LLVM to continue to use its own
Mutex class. I don't think you should change it, I think you should remove
it, and use std::mutex. I understand that this may be hard, but I think
time would be better spent working on those hard problems. I see a few
elements that you'll need to address:

  1. Fixing the code in atexit handlers. I'm not 100% certain of the right

approach here, but my strong suspicion is that they should not be using a
mutex *at all*. LLVM should have *very* little code in atexit handlers as a
library, and that code really should be customized for the single threaded
and strange circumstance in which it runs. I know you've already started
down this path. In general, that should be a nice separable bit.

  1. You or someone else should start a discussion on llvmdev@ about whether

or not we need to avoid the *overhead* of an uncontended mutex lock when
LLVM is not multithreaded. I think that the answer is "no" based on a lot
of experience with reasonably good mutex implementations, but we need to
collect more opinions about this and understand the specific use cases. If
there are mutexes which materially impact the performance of LLVM when
running in a single thread, they will also be performance problems for
multithreaded code (by virtue of crazy amounts of contention) and should be
fixed directly.

  1. Perhaps even before doing #2, we should sort out whether the

multithreaded support in LLVM should be a compile time or runtime
parameter. While I strongly feel it should be compile time if present at
all, this isn't a discussion for a long, twisty patch review thread. =] It
needs its own discussion and a wider audience.

I also suspect you could go ahead and convert some users of mutexes to
std::mutex while these things are in-flight. For example, I agree with Greg
who said on the LLDB list that he would prefer LLDB just use std::mutex --
why not go ahead and start that migration? If LLD still uses something
else, that would be another good candidate. Both of those have a decent
history of being multithreaded always, and so it seems uncontentious there
even while we try to sort out exactly what is right for the core libraries
that support JITs and other more complex users.

-Chandler

zturner abandoned this revision.Jun 9 2014, 8:45 PM
lib/Support/Windows/DynamicLibrary.inc