From a41e1cb66bd2d5134e2c5cd129e15fce10251ee7 Mon Sep 17 00:00:00 2001 From: Alexander Shabalin Date: Mon, 14 Dec 2020 12:38:46 +0300 Subject: [PATCH] Refactor Mutex.hpp (#4594) * Remove `LockGuard` in favour of `std::lock_guard` * Rename `SimpleMutex` to `SpinLock` --- .../runtime/src/legacymm/cpp/Memory.cpp | 9 ++-- .../runtime/src/main/cpp/MultiSourceQueue.hpp | 8 ++-- kotlin-native/runtime/src/main/cpp/Mutex.hpp | 44 +++++++------------ .../runtime/src/main/cpp/ObjCExport.mm | 10 +++-- .../runtime/src/main/cpp/ObjCInterop.mm | 5 ++- .../runtime/src/main/cpp/SingleLockList.hpp | 6 +-- 6 files changed, 38 insertions(+), 44 deletions(-) diff --git a/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp b/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp index 06dc0838663..e46ef5aad73 100644 --- a/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp +++ b/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp @@ -18,6 +18,7 @@ #include #include // for offsetof +#include // Allow concurrent global cycle collector. #define USE_CYCLIC_GC 0 @@ -218,14 +219,14 @@ class CycleDetector : private kotlin::Pinned { } void insertCandidate(KRef candidate) { - LockGuard guard(lock_); + std::lock_guard guard(lock_); auto it = candidateList_.insert(candidateList_.begin(), candidate); candidateInList_.emplace(candidate, it); } void removeCandidate(KRef candidate) { - LockGuard guard(lock_); + std::lock_guard guard(lock_); auto it = candidateInList_.find(candidate); if (it == candidateInList_.end()) @@ -234,7 +235,7 @@ class CycleDetector : private kotlin::Pinned { candidateInList_.erase(it); } - SimpleMutex lock_; + kotlin::SpinLock lock_; using CandidateList = KStdList; CandidateList candidateList_; KStdUnorderedMap candidateInList_; @@ -2990,7 +2991,7 @@ ScopedRefHolder::~ScopedRefHolder() { CycleDetectorRootset CycleDetector::collectRootset() { auto& detector = instance(); CycleDetectorRootset rootset; - LockGuard guard(detector.lock_); + std::lock_guard guard(detector.lock_); for (auto* candidate: detector.candidateList_) { // Only frozen candidates are to be analyzed. if (!isPermanentOrFrozen(candidate)) diff --git a/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp b/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp index 0c933744aec..9352006a424 100644 --- a/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp +++ b/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp @@ -65,7 +65,7 @@ public: for (auto& node : queue_) { node.owner_ = nullptr; } - std::lock_guard guard(owner_.mutex_); + std::lock_guard guard(owner_.mutex_); owner_.queue_.splice(owner_.queue_.end(), queue_); owner_.deletionQueue_.splice(owner_.deletionQueue_.end(), deletionQueue_); } @@ -108,7 +108,7 @@ public: explicit Iterable(MultiSourceQueue& owner) noexcept : owner_(owner), guard_(owner_.mutex_) {} MultiSourceQueue& owner_; // weak - std::unique_lock guard_; + std::unique_lock guard_; }; // Lock `MultiSourceQueue` for safe iteration. If element was scheduled for deletion, @@ -117,7 +117,7 @@ public: // Lock `MultiSourceQueue` and apply deletions. Only deletes elements that were published. void ApplyDeletions() noexcept { - std::lock_guard guard(mutex_); + std::lock_guard guard(mutex_); std::list remainingDeletions; auto it = deletionQueue_.begin(); @@ -143,7 +143,7 @@ private: // which is important for GC mark phase. std::list queue_; std::list deletionQueue_; - SimpleMutex mutex_; + SpinLock mutex_; }; } // namespace kotlin diff --git a/kotlin-native/runtime/src/main/cpp/Mutex.hpp b/kotlin-native/runtime/src/main/cpp/Mutex.hpp index 87778b15ce4..bec1bf16b7e 100644 --- a/kotlin-native/runtime/src/main/cpp/Mutex.hpp +++ b/kotlin-native/runtime/src/main/cpp/Mutex.hpp @@ -18,41 +18,31 @@ #define RUNTIME_MUTEX_H #include + #include "KAssert.h" #include "Utils.hpp" -class SimpleMutex { - private: - int32_t atomicInt = 0; +namespace kotlin { - public: - void lock() { - while (!__sync_bool_compare_and_swap(&atomicInt, 0, 1)) { - // TODO: yield. +class SpinLock : private Pinned { +public: + void lock() noexcept { + while (!__sync_bool_compare_and_swap(&atomicInt, 0, 1)) { + // TODO: yield. + } } - } - void unlock() { - if (!__sync_bool_compare_and_swap(&atomicInt, 1, 0)) { - RuntimeAssert(false, "Unable to unlock"); + void unlock() noexcept { + if (!__sync_bool_compare_and_swap(&atomicInt, 1, 0)) { + RuntimeAssert(false, "Unable to unlock"); + } } - } + +private: + // TODO: Consider using `std::atomic_flag`. + int32_t atomicInt = 0; }; -// TODO: use std::lock_guard instead? -template -class LockGuard : private kotlin::Pinned { - public: - explicit LockGuard(Mutex& mutex_) : mutex(mutex_) { - mutex.lock(); - } - - ~LockGuard() { - mutex.unlock(); - } - - private: - Mutex& mutex; -}; +} // namespace kotlin #endif // RUNTIME_MUTEX_H diff --git a/kotlin-native/runtime/src/main/cpp/ObjCExport.mm b/kotlin-native/runtime/src/main/cpp/ObjCExport.mm index e98988da83c..e615ef07ea2 100644 --- a/kotlin-native/runtime/src/main/cpp/ObjCExport.mm +++ b/kotlin-native/runtime/src/main/cpp/ObjCExport.mm @@ -21,6 +21,8 @@ #if KONAN_OBJC_INTEROP +#import + #import #import #import @@ -959,7 +961,7 @@ static const TypeInfo* createTypeInfo(Class clazz, const TypeInfo* superType, co return result; } -static SimpleMutex typeInfoCreationMutex; +static kotlin::SpinLock typeInfoCreationMutex; static const TypeInfo* getOrCreateTypeInfo(Class clazz) { const TypeInfo* result = Kotlin_ObjCExport_getAssociatedTypeInfo(clazz); @@ -973,7 +975,7 @@ static const TypeInfo* getOrCreateTypeInfo(Class clazz) { theForeignObjCObjectTypeInfo : getOrCreateTypeInfo(superClass); - LockGuard lockGuard(typeInfoCreationMutex); + std::lock_guard lockGuard(typeInfoCreationMutex); result = Kotlin_ObjCExport_getAssociatedTypeInfo(clazz); // double-checking. if (result == nullptr) { @@ -993,7 +995,7 @@ const TypeInfo* Kotlin_ObjCExport_createTypeInfoWithKotlinFieldsFrom(Class clazz return createTypeInfo(clazz, superType, fieldsInfo); } -static SimpleMutex classCreationMutex; +static kotlin::SpinLock classCreationMutex; static int anonymousClassNextId = 0; static void addVirtualAdapters(Class clazz, const ObjCTypeAdapter* typeAdapter) { @@ -1067,7 +1069,7 @@ static Class getOrCreateClass(const TypeInfo* typeInfo) { } else { Class superClass = getOrCreateClass(typeInfo->superType_); - LockGuard lockGuard(classCreationMutex); // Note: non-recursive + std::lock_guard lockGuard(classCreationMutex); // Note: non-recursive result = typeInfo->writableInfo_->objCExport.objCClass; // double-checking. if (result == nullptr) { diff --git a/kotlin-native/runtime/src/main/cpp/ObjCInterop.mm b/kotlin-native/runtime/src/main/cpp/ObjCInterop.mm index 3b312cc3f95..453a3d60d1b 100644 --- a/kotlin-native/runtime/src/main/cpp/ObjCInterop.mm +++ b/kotlin-native/runtime/src/main/cpp/ObjCInterop.mm @@ -24,6 +24,7 @@ #include #include #include +#include #include "Memory.h" #include "MemorySharedRefs.hpp" @@ -240,7 +241,7 @@ static void AddMethods(Class clazz, const struct ObjCMethodDescription* methods, } } -static SimpleMutex classCreationMutex; +static kotlin::SpinLock classCreationMutex; static int anonymousClassNextId = 0; static Class allocateClass(const KotlinObjCClassInfo* info) { @@ -271,7 +272,7 @@ static Class allocateClass(const KotlinObjCClassInfo* info) { } void* CreateKotlinObjCClass(const KotlinObjCClassInfo* info) { - LockGuard lockGuard(classCreationMutex); + std::lock_guard lockGuard(classCreationMutex); void* createdClass = *info->createdClass; if (createdClass != nullptr) { diff --git a/kotlin-native/runtime/src/main/cpp/SingleLockList.hpp b/kotlin-native/runtime/src/main/cpp/SingleLockList.hpp index 06cbdaee316..cebf5c033d2 100644 --- a/kotlin-native/runtime/src/main/cpp/SingleLockList.hpp +++ b/kotlin-native/runtime/src/main/cpp/SingleLockList.hpp @@ -17,7 +17,7 @@ namespace kotlin { // TODO: Consider different locking mechanisms. -template +template class SingleLockList : private Pinned { public: class Node : Pinned { @@ -71,7 +71,7 @@ public: Node* Emplace(Args... args) noexcept { auto* nodePtr = new Node(args...); std::unique_ptr node(nodePtr); - LockGuard guard(mutex_); + std::lock_guard guard(mutex_); if (root_) { root_->previous = node.get(); } @@ -82,7 +82,7 @@ public: // Using `node` including its referred `Value` after `Erase` is undefined behaviour. void Erase(Node* node) noexcept { - LockGuard guard(mutex_); + std::lock_guard guard(mutex_); if (root_.get() == node) { root_ = std::move(node->next); if (root_) {