From 629ea1036ec53f55ffad1cd7d283b68c10a9b307 Mon Sep 17 00:00:00 2001 From: Ilya Matveev Date: Tue, 28 Sep 2021 17:29:42 +0700 Subject: [PATCH] [K/N][Runtime] Support thread state switching in spin locks Issue #KT-49013 Fixed --- .../runtime/src/legacymm/cpp/Memory.cpp | 8 +- .../runtime/src/main/cpp/Exceptions.cpp | 18 +--- kotlin-native/runtime/src/main/cpp/Memory.h | 14 ++++ .../runtime/src/main/cpp/MultiSourceQueue.hpp | 10 +-- .../src/main/cpp/MultiSourceQueueTest.cpp | 6 +- kotlin-native/runtime/src/main/cpp/Mutex.hpp | 37 ++++++++- .../runtime/src/main/cpp/MutexTest.cpp | 49 +++++++++++ .../runtime/src/main/cpp/ObjCExport.mm | 8 +- .../runtime/src/main/cpp/ObjCInterop.mm | 4 +- .../runtime/src/main/cpp/SingleLockList.hpp | 2 +- .../src/main/cpp/SingleLockListTest.cpp | 6 +- .../runtime/src/mm/cpp/GlobalsRegistry.hpp | 10 ++- .../runtime/src/mm/cpp/MutexTest.cpp | 83 +++++++++++++++++++ .../runtime/src/mm/cpp/ObjectFactory.hpp | 6 +- .../runtime/src/mm/cpp/StableRefRegistry.hpp | 12 +-- 15 files changed, 223 insertions(+), 50 deletions(-) create mode 100644 kotlin-native/runtime/src/main/cpp/MutexTest.cpp create mode 100644 kotlin-native/runtime/src/mm/cpp/MutexTest.cpp diff --git a/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp b/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp index dee4d9cc378..780131a43be 100644 --- a/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp +++ b/kotlin-native/runtime/src/legacymm/cpp/Memory.cpp @@ -227,14 +227,14 @@ class CycleDetector : private kotlin::Pinned, public KonanAllocatorAware { } void insertCandidate(KRef candidate) { - std::lock_guard guard(lock_); + std::lock_guard guard(lock_); auto it = candidateList_.insert(candidateList_.begin(), candidate); candidateInList_.emplace(candidate, it); } void removeCandidate(KRef candidate) { - std::lock_guard guard(lock_); + std::lock_guard guard(lock_); auto it = candidateInList_.find(candidate); if (it == candidateInList_.end()) @@ -243,7 +243,7 @@ class CycleDetector : private kotlin::Pinned, public KonanAllocatorAware { candidateInList_.erase(it); } - kotlin::SpinLock lock_; + kotlin::SpinLock lock_; using CandidateList = KStdList; CandidateList candidateList_; KStdUnorderedMap candidateInList_; @@ -2994,7 +2994,7 @@ ScopedRefHolder::~ScopedRefHolder() { CycleDetectorRootset CycleDetector::collectRootset() { auto& detector = instance(); CycleDetectorRootset rootset; - std::lock_guard 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/Exceptions.cpp b/kotlin-native/runtime/src/main/cpp/Exceptions.cpp index 425206d6d09..e7052d9f45c 100644 --- a/kotlin-native/runtime/src/main/cpp/Exceptions.cpp +++ b/kotlin-native/runtime/src/main/cpp/Exceptions.cpp @@ -47,18 +47,6 @@ void ThrowException(KRef exception) { namespace { -// This function accesses a TLS variable under the hood, so it must not be called from a thread destructor -// (see kotlin::mm::IsCurrentThreadRegistered, kotlin::mm::GetMemoryState()). -[[nodiscard]] kotlin::ThreadStateGuard setNativeStateForRegisteredThread(bool reentrant = true) { - if (kotlin::mm::IsCurrentThreadRegistered()) { - return kotlin::ThreadStateGuard(kotlin::ThreadState::kNative, reentrant); - } else { - // The current thread is not registered in the Kotlin runtime, - // just return an empty guard which doesn't actually switch the state. - return kotlin::ThreadStateGuard(); - } -} - class { /** * Timeout 5 sec for concurrent (second) terminate attempt to give a chance the first one to finish. @@ -73,7 +61,7 @@ class { // block() is supposed to be NORETURN, otherwise go to normal abort() konan::abort(); } else { - auto guard = setNativeStateForRegisteredThread(); + kotlin::NativeOrUnregisteredThreadGuard guard(/* reentrant = */ true); sleep(timeoutSec); // We come here when another terminate handler hangs for 5 sec, that looks fatally broken. Go to forced exit now. } @@ -147,12 +135,12 @@ class TerminateHandler : private kotlin::Pinned { terminateWithUnhandledException(e.GetExceptionObject()); } catch (...) { // Not a Kotlin exception - call default handler - auto guard = setNativeStateForRegisteredThread(); + kotlin::NativeOrUnregisteredThreadGuard guard(/* reentrant = */ true); queuedHandler(); } } // Come here in case of direct terminate() call or unknown exception - go to default terminate handler. - auto guard = setNativeStateForRegisteredThread(); + kotlin::NativeOrUnregisteredThreadGuard guard(/* reentrant = */ true); queuedHandler(); } diff --git a/kotlin-native/runtime/src/main/cpp/Memory.h b/kotlin-native/runtime/src/main/cpp/Memory.h index 215aa55ecfe..01bfe54ccd1 100644 --- a/kotlin-native/runtime/src/main/cpp/Memory.h +++ b/kotlin-native/runtime/src/main/cpp/Memory.h @@ -497,6 +497,20 @@ ALWAYS_INLINE inline R CallWithThreadState(R(*function)(Args...), Args... args) return function(std::forward(args)...); } +class NativeOrUnregisteredThreadGuard final : private MoveOnly { +public: + explicit NativeOrUnregisteredThreadGuard(bool reentrant = false) noexcept { + // The default ctor of ThreadStateGuard doesn't set the state. + // So the actual state switching is performed only if the thread is registered. + if (kotlin::mm::IsCurrentThreadRegistered()) { + backingGuard_ = kotlin::ThreadStateGuard(kotlin::ThreadState::kNative, reentrant); + } + } + +private: + ThreadStateGuard backingGuard_; +}; + extern const bool kSupportsMultipleMutators; } // namespace kotlin diff --git a/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp b/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp index 309730e8f52..f6bd8f0b4a0 100644 --- a/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp +++ b/kotlin-native/runtime/src/main/cpp/MultiSourceQueue.hpp @@ -16,7 +16,7 @@ namespace kotlin { // A queue that is constructed by collecting subqueues from several `Producer`s. -template +template class MultiSourceQueue { public: class Producer; @@ -66,7 +66,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_); } @@ -114,7 +114,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, @@ -123,7 +123,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_); KStdList remainingDeletions; auto it = deletionQueue_.begin(); @@ -154,7 +154,7 @@ private: // which is important for GC mark phase. KStdList queue_; KStdList deletionQueue_; - SpinLock mutex_; + Mutex mutex_; }; } // namespace kotlin diff --git a/kotlin-native/runtime/src/main/cpp/MultiSourceQueueTest.cpp b/kotlin-native/runtime/src/main/cpp/MultiSourceQueueTest.cpp index 40f060fed33..dc9f8810a43 100644 --- a/kotlin-native/runtime/src/main/cpp/MultiSourceQueueTest.cpp +++ b/kotlin-native/runtime/src/main/cpp/MultiSourceQueueTest.cpp @@ -18,8 +18,8 @@ using namespace kotlin; namespace { -template -KStdVector Collect(MultiSourceQueue& queue) { +template +KStdVector Collect(MultiSourceQueue& queue) { KStdVector result; for (const auto& element : queue.LockForIter()) { result.push_back(element); @@ -29,7 +29,7 @@ KStdVector Collect(MultiSourceQueue& queue) { } // namespace -using IntQueue = MultiSourceQueue; +using IntQueue = MultiSourceQueue>; TEST(MultiSourceQueueTest, Insert) { IntQueue queue; diff --git a/kotlin-native/runtime/src/main/cpp/Mutex.hpp b/kotlin-native/runtime/src/main/cpp/Mutex.hpp index bec1bf16b7e..80545175b90 100644 --- a/kotlin-native/runtime/src/main/cpp/Mutex.hpp +++ b/kotlin-native/runtime/src/main/cpp/Mutex.hpp @@ -20,11 +20,20 @@ #include #include "KAssert.h" +#include "Memory.h" #include "Utils.hpp" namespace kotlin { -class SpinLock : private Pinned { +enum class MutexThreadStateHandling { + kIgnore, kSwitchIfRegistered +}; + +template +class SpinLock; + +template <> +class SpinLock : private Pinned { public: void lock() noexcept { while (!__sync_bool_compare_and_swap(&atomicInt, 0, 1)) { @@ -43,6 +52,32 @@ private: int32_t atomicInt = 0; }; +template <> +class SpinLock : private Pinned { +public: + void lock() noexcept { + // Fast path without thread state switching. + if (__sync_bool_compare_and_swap(&atomicInt, 0, 1)) { + return; + } + + kotlin::NativeOrUnregisteredThreadGuard guard(/* reentrant = */ true); + while (!__sync_bool_compare_and_swap(&atomicInt, 0, 1)) { + // TODO: yield. + } + } + + 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; +}; + } // namespace kotlin #endif // RUNTIME_MUTEX_H diff --git a/kotlin-native/runtime/src/main/cpp/MutexTest.cpp b/kotlin-native/runtime/src/main/cpp/MutexTest.cpp new file mode 100644 index 00000000000..8172a29711d --- /dev/null +++ b/kotlin-native/runtime/src/main/cpp/MutexTest.cpp @@ -0,0 +1,49 @@ +/* + * Copyright 2010-2021 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license + * that can be found in the LICENSE file. + */ + +#include "Mutex.hpp" + +#include +#include + +#include "gtest/gtest.h" +#include "TestSupport.hpp" + +using namespace kotlin; + +template +class MutexTest : public testing::Test {}; + +using LockTypes = testing::Types, SpinLock>; +TYPED_TEST_SUITE(MutexTest, LockTypes); + +TYPED_TEST(MutexTest, SmokeDetachedThread) { + using LockUnderTest = TypeParam; + + LockUnderTest lock; + std::thread secondThread; + std::atomic started = false; + std::atomic protectedCounter = 0; + + { + std::unique_lock guard1(lock); + secondThread = std::thread([&lock, &started, &protectedCounter]() { + started = true; + std::unique_lock guard2(lock); + protectedCounter++; + }); + + protectedCounter++; + while (!started) { std::this_thread::yield(); } + + // Wait to give the second thread a chance to try to acquire the lock. + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + EXPECT_EQ(protectedCounter, 1); + } + secondThread.join(); + EXPECT_EQ(protectedCounter, 2); +} + + diff --git a/kotlin-native/runtime/src/main/cpp/ObjCExport.mm b/kotlin-native/runtime/src/main/cpp/ObjCExport.mm index 79740d8e6e7..dbc309d714a 100644 --- a/kotlin-native/runtime/src/main/cpp/ObjCExport.mm +++ b/kotlin-native/runtime/src/main/cpp/ObjCExport.mm @@ -982,7 +982,7 @@ static const TypeInfo* createTypeInfo(Class clazz, const TypeInfo* superType, co return result; } -static kotlin::SpinLock typeInfoCreationMutex; +static kotlin::SpinLock typeInfoCreationMutex; static const TypeInfo* getOrCreateTypeInfo(Class clazz) { const TypeInfo* result = Kotlin_ObjCExport_getAssociatedTypeInfo(clazz); @@ -996,7 +996,7 @@ static const TypeInfo* getOrCreateTypeInfo(Class clazz) { theForeignObjCObjectTypeInfo : getOrCreateTypeInfo(superClass); - std::lock_guard lockGuard(typeInfoCreationMutex); + std::lock_guard lockGuard(typeInfoCreationMutex); result = Kotlin_ObjCExport_getAssociatedTypeInfo(clazz); // double-checking. if (result == nullptr) { @@ -1016,7 +1016,7 @@ const TypeInfo* Kotlin_ObjCExport_createTypeInfoWithKotlinFieldsFrom(Class clazz return createTypeInfo(clazz, superType, fieldsInfo); } -static kotlin::SpinLock classCreationMutex; +static kotlin::SpinLock classCreationMutex; static int anonymousClassNextId = 0; static void addVirtualAdapters(Class clazz, const ObjCTypeAdapter* typeAdapter) { @@ -1092,7 +1092,7 @@ static Class getOrCreateClass(const TypeInfo* typeInfo) { } else { Class superClass = getOrCreateClass(typeInfo->superType_); - std::lock_guard 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 ffd73c3ff58..8d51ec3ebb6 100644 --- a/kotlin-native/runtime/src/main/cpp/ObjCInterop.mm +++ b/kotlin-native/runtime/src/main/cpp/ObjCInterop.mm @@ -253,7 +253,7 @@ static void AddMethods(Class clazz, const struct ObjCMethodDescription* methods, } } -static kotlin::SpinLock classCreationMutex; +static kotlin::SpinLock classCreationMutex; static int anonymousClassNextId = 0; NO_EXTERNAL_CALLS_CHECK static Class allocateClass(const KotlinObjCClassInfo* info) { @@ -284,7 +284,7 @@ NO_EXTERNAL_CALLS_CHECK static Class allocateClass(const KotlinObjCClassInfo* in } void* CreateKotlinObjCClass(const KotlinObjCClassInfo* info) { - std::lock_guard 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 9352e2f0c13..a1c9f041b71 100644 --- a/kotlin-native/runtime/src/main/cpp/SingleLockList.hpp +++ b/kotlin-native/runtime/src/main/cpp/SingleLockList.hpp @@ -20,7 +20,7 @@ namespace kotlin { // TODO: Consider different locking mechanisms. -template +template class SingleLockList : private Pinned { public: class Node; diff --git a/kotlin-native/runtime/src/main/cpp/SingleLockListTest.cpp b/kotlin-native/runtime/src/main/cpp/SingleLockListTest.cpp index b576059dd43..77c28c236c9 100644 --- a/kotlin-native/runtime/src/main/cpp/SingleLockListTest.cpp +++ b/kotlin-native/runtime/src/main/cpp/SingleLockListTest.cpp @@ -19,7 +19,7 @@ using namespace kotlin; namespace { -using IntList = SingleLockList; +using IntList = SingleLockList>; } // namespace @@ -376,7 +376,7 @@ private: } // namespace TEST(SingleLockListTest, PinnedType) { - SingleLockList list; + SingleLockList> list; constexpr int kFirst = 1; auto* itemNode = list.Emplace(kFirst); @@ -414,7 +414,7 @@ private: TEST(SingleLockListTest, Destructor) { testing::StrictMock> hook; { - SingleLockList list; + SingleLockList> list; auto* first = list.Emplace(hook.AsStdFunction())->Get(); auto* second = list.Emplace(hook.AsStdFunction())->Get(); auto* third = list.Emplace(hook.AsStdFunction())->Get(); diff --git a/kotlin-native/runtime/src/mm/cpp/GlobalsRegistry.hpp b/kotlin-native/runtime/src/mm/cpp/GlobalsRegistry.hpp index e06a4e5ef70..ddba721b40a 100644 --- a/kotlin-native/runtime/src/mm/cpp/GlobalsRegistry.hpp +++ b/kotlin-native/runtime/src/mm/cpp/GlobalsRegistry.hpp @@ -15,16 +15,18 @@ namespace kotlin { namespace mm { class GlobalsRegistry : Pinned { + using Mutex = SpinLock; + public: - class ThreadQueue : public MultiSourceQueue::Producer { + class ThreadQueue : public MultiSourceQueue::Producer { public: explicit ThreadQueue(GlobalsRegistry& registry) : Producer(registry.globals_) {} // Do not add fields as this is just a wrapper and Producer does not have virtual destructor. }; - using Iterable = MultiSourceQueue::Iterable; + using Iterable = MultiSourceQueue::Iterable; - using Iterator = MultiSourceQueue::Iterator; + using Iterator = MultiSourceQueue::Iterator; GlobalsRegistry(); ~GlobalsRegistry(); @@ -47,7 +49,7 @@ public: private: // TODO: Add-only MultiSourceQueue can be made more efficient. Measure, if it's a problem. - MultiSourceQueue globals_; + MultiSourceQueue globals_; }; } // namespace mm diff --git a/kotlin-native/runtime/src/mm/cpp/MutexTest.cpp b/kotlin-native/runtime/src/mm/cpp/MutexTest.cpp new file mode 100644 index 00000000000..fd1d2d75f12 --- /dev/null +++ b/kotlin-native/runtime/src/mm/cpp/MutexTest.cpp @@ -0,0 +1,83 @@ +/* + * Copyright 2010-2021 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license + * that can be found in the LICENSE file. + */ + +#include "Mutex.hpp" + +#include + +#include "gtest/gtest.h" +#include "TestSupport.hpp" +#include "ThreadState.hpp" + +using namespace kotlin; + +namespace { + +template +class Mode { +public: + using Lock = SpinLock; + static constexpr MutexThreadStateHandling threadStateHandling = Handling; + static constexpr ThreadState initialState = State; +}; + +} // namespace + +template +class MutexTestNewMM : public testing::Test {}; + +using TestModes = testing::Types< + Mode, + Mode, + Mode, + Mode>; + +TYPED_TEST_SUITE(MutexTestNewMM, TestModes); + +TYPED_TEST(MutexTestNewMM, SmokeAttachedThread) { + RunInNewThread([](){ + using LockUnderTest = typename TypeParam::Lock; + ThreadState initialThreadState = TypeParam::initialState; + + LockUnderTest lock; + std::thread secondThread; + std::atomic started = false; + std::atomic protectedCounter = 0; + mm::ThreadData* secondThreadData = nullptr; + + { + std::unique_lock guard1(lock); + secondThread = std::thread([&lock, &started, &protectedCounter, &secondThreadData, &initialThreadState]() { + ScopedMemoryInit init; + SwitchThreadState(init.memoryState(), initialThreadState, /* reentrant = */ true); + + secondThreadData = init.memoryState()->GetThreadData(); + ASSERT_EQ(secondThreadData->state(), initialThreadState); + + started = true; + std::unique_lock guard2(lock); + EXPECT_EQ(secondThreadData->state(), initialThreadState); + protectedCounter++; + }); + + protectedCounter++; + while (!started) { + std::this_thread::yield(); + } + + // Wait to give the second thread a chance to try to acquire the lock. + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + EXPECT_EQ(protectedCounter, 1); + + auto expectedSecondThreadState = + (TypeParam::threadStateHandling == MutexThreadStateHandling::kSwitchIfRegistered) + ? ThreadState::kNative + : initialThreadState; + EXPECT_EQ(secondThreadData->state(), expectedSecondThreadState); + } + secondThread.join(); + EXPECT_EQ(protectedCounter, 2); + }); +} diff --git a/kotlin-native/runtime/src/mm/cpp/ObjectFactory.hpp b/kotlin-native/runtime/src/mm/cpp/ObjectFactory.hpp index cf2d46d878d..26f1a84ac36 100644 --- a/kotlin-native/runtime/src/mm/cpp/ObjectFactory.hpp +++ b/kotlin-native/runtime/src/mm/cpp/ObjectFactory.hpp @@ -163,7 +163,7 @@ public: return; } - std::lock_guard guard(owner_.mutex_); + std::lock_guard guard(owner_.mutex_); owner_.AssertCorrectUnsafe(); @@ -328,7 +328,7 @@ public: private: ObjectFactoryStorage& owner_; // weak - std::unique_lock guard_; + std::unique_lock> guard_; }; ~ObjectFactoryStorage() { @@ -389,7 +389,7 @@ private: unique_ptr root_; Node* last_ = nullptr; size_t size_ = 0; - SpinLock mutex_; + SpinLock mutex_; }; class SimpleAllocator { diff --git a/kotlin-native/runtime/src/mm/cpp/StableRefRegistry.hpp b/kotlin-native/runtime/src/mm/cpp/StableRefRegistry.hpp index 399197f38e5..8276df6a03c 100644 --- a/kotlin-native/runtime/src/mm/cpp/StableRefRegistry.hpp +++ b/kotlin-native/runtime/src/mm/cpp/StableRefRegistry.hpp @@ -15,16 +15,18 @@ namespace mm { // Registry for all objects that have references outside of Kotlin. class StableRefRegistry : Pinned { + using Mutex = SpinLock; + public: - class ThreadQueue : public MultiSourceQueue::Producer { + class ThreadQueue : public MultiSourceQueue::Producer { public: explicit ThreadQueue(StableRefRegistry& registry) : Producer(registry.stableRefs_) {} // Do not add fields as this is just a wrapper and Producer does not have virtual destructor. }; - using Iterable = MultiSourceQueue::Iterable; - using Iterator = MultiSourceQueue::Iterator; - using Node = MultiSourceQueue::Node; + using Iterable = MultiSourceQueue::Iterable; + using Iterator = MultiSourceQueue::Iterator; + using Node = MultiSourceQueue::Node; StableRefRegistry(); ~StableRefRegistry(); @@ -64,7 +66,7 @@ private: // before posting queue to the global registry) // // TODO: Measure to understand, if this approach is problematic. - MultiSourceQueue stableRefs_; + MultiSourceQueue stableRefs_; }; } // namespace mm