[PATCH] Change base mutex implementations to use STL-provided mutexes

Alp Toker alp at nuanti.com
Fri Jun 6 13:10:38 PDT 2014


On 06/06/2014 22:57, Zachary Turner wrote:
> The motivation is just that we should be relying on standard 
> implementations as they're more likely to be robust, battle tested, 
> and higher performance than our own hand-rolled implementations.  For 
> example, there are many locations in the code that roll a custom 
> version of once-only initialization.  As a future task, I would like 
> to convert all of them to std::call_once.  Just as a general principle 
> of not re-inventing the wheel.
>
> As for the platform specific header includes, I guess that's 
> ultimately a separate discussion, and this change can be made without 
> introducing platform-specific header includes into Mutex.h.  I am 
> currently working on a revision to this patch which removes the 
> windows.h include from mutex.h, so I will upload that later.

Great :-) The review thread didn't show up when I wrote my response for 
some reason (my bad) and I see now the work is heading in the right 
direction.

It's perfectly reasonable to use standard implementations -- thanks for 
working on this.

Please do take care to hide any conditionals and platform header 
includes in the implementation though. That's the secret sauce that 
makes LLVM tools, plugins and modules trivially portable compared to 
other software projects of this scale.

Alp.


>
>
> On Fri, Jun 6, 2014 at 12:32 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>     Hi Zachary,
>
>     In LLVM platform-specific code goes in the implementation files
>     (.inc) and never in the portable headers. Certainly for a
>     high-level class like Mutex I wouldn't expect to see
>     platform-specific conditionals exposed.
>
>     A couple of exceptions have slipped in like Support/FileSystem.h
>     and Support/Process.h with ifdef WIN32 but those are meant to get
>     fixed at some point.
>
>     On a more practical level it's not a good idea to include
>     windows.h globally throughout the public headers. These headers
>     need to get included in portable software that embeds clang for
>     tooling etc. and windows.h is known to be problematic.
>
>     For thread primitives, the previous implementation looks broadly
>     correct. You've explained the mechanics of your changes in detail
>     but can you explain the motivation here? We need to find a better
>     way to achieve that without introducing all these OS-specific
>     low-level headers globally into embedding applications.
>
>     Alp.
>
>
>
>
>     On 05/06/2014 20:06, Zachary Turner wrote:
>
>         Hi rnk, aaron.ballman, chandlerc,
>
>         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.
>
>         http://reviews.llvm.org/D4033
>
>         Files:
>            include/llvm/ExecutionEngine/ExecutionEngine.h
>            include/llvm/IR/ValueMap.h
>            include/llvm/Support/Mutex.h
>            include/llvm/Support/MutexGuard.h
>            lib/CodeGen/PseudoSourceValue.cpp
>            lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>            lib/ExecutionEngine/ExecutionEngine.cpp
>            lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp
>            lib/ExecutionEngine/JIT/JIT.cpp
>            lib/ExecutionEngine/JIT/JITEmitter.cpp
>            lib/ExecutionEngine/OProfileJIT/OProfileWrapper.cpp
>            lib/ExecutionEngine/RuntimeDyld/GDBRegistrar.cpp
>            lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
>            lib/IR/LeakDetector.cpp
>            lib/IR/LegacyPassManager.cpp
>            lib/Support/CMakeLists.txt
>            lib/Support/CrashRecoveryContext.cpp
>            lib/Support/DynamicLibrary.cpp
>            lib/Support/Mutex.cpp
>            lib/Support/PluginLoader.cpp
>            lib/Support/Statistic.cpp
>            lib/Support/Threading.cpp
>            lib/Support/Timer.cpp
>            lib/Support/Unix/Mutex.inc
>            lib/Support/Unix/Process.inc
>            lib/Support/Unix/Signals.inc
>            lib/Support/Windows/DynamicLibrary.inc
>            lib/Support/Windows/Mutex.inc
>            lib/Target/NVPTX/NVPTXUtilities.cpp
>            unittests/IR/ValueMapTest.cpp
>
>
>         _______________________________________________
>         llvm-commits mailing list
>         llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>         http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>

-- 
http://www.nuanti.com
the browser experts




More information about the llvm-commits mailing list