From 7b384fb5e8144bbff6317521d73bc3bb057c3c3c Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 19 Jan 2024 10:35:49 -0800 Subject: [PATCH] remove all uses of our custom spinlock This has caused issues and over time we have reduced the use of spinlocks, it was only used in few places and we still have evidence that it's causing ANRs. We use utils::Mutex instead which is a low overhead mutex implementation on Linux systems. FIXES=[321101014] --- NEW_RELEASE_NOTES.md | 2 + filament/src/details/ColorGrading.cpp | 9 +- libs/utils/CMakeLists.txt | 1 - libs/utils/benchmark/benchmark_allocators.cpp | 15 ---- libs/utils/benchmark/benchmark_mutex.cpp | 15 ---- libs/utils/include/utils/Allocator.h | 2 - libs/utils/include/utils/JobSystem.h | 2 +- libs/utils/include/utils/SpinLock.h | 90 ------------------- libs/utils/src/JobSystem.cpp | 6 +- 9 files changed, 11 insertions(+), 131 deletions(-) delete mode 100644 libs/utils/include/utils/SpinLock.h diff --git a/NEW_RELEASE_NOTES.md b/NEW_RELEASE_NOTES.md index 4a1a9c7fa7..0c8949c7f2 100644 --- a/NEW_RELEASE_NOTES.md +++ b/NEW_RELEASE_NOTES.md @@ -7,3 +7,5 @@ for next branch cut* header. appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md). ## Release notes for next branch cut + +- utils: remove usages of `SpinLock`. Fixes b/321101014. diff --git a/filament/src/details/ColorGrading.cpp b/filament/src/details/ColorGrading.cpp index 6bd40a4141..e02f029080 100644 --- a/filament/src/details/ColorGrading.cpp +++ b/filament/src/details/ColorGrading.cpp @@ -30,11 +30,12 @@ #include #include -#include +#include #include #include #include +#include #include namespace filament { @@ -648,9 +649,9 @@ FColorGrading::FColorGrading(FEngine& engine, const Builder& builder) { Config c; // This lock protects the data inside Config, which is written to by the Filament thread, // and read from multiple Job threads. - utils::SpinLock configLock; + utils::Mutex configLock; { - std::lock_guard lock(configLock); + std::lock_guard const lock(configLock); c.lutDimension = builder->dimension; c.adaptationTransform = adaptationTransform(builder->whiteBalance); c.colorGradingIn = selectColorGradingTransformIn(builder->toneMapping); @@ -687,7 +688,7 @@ FColorGrading::FColorGrading(FEngine& engine, const Builder& builder) { [data, converted, b, &c, &configLock, builder](JobSystem&, JobSystem::Job*) { Config config; { - std::lock_guard lock(configLock); + std::lock_guard lock(configLock); config = c; } half4* UTILS_RESTRICT p = (half4*) data + b * config.lutDimension * config.lutDimension; diff --git a/libs/utils/CMakeLists.txt b/libs/utils/CMakeLists.txt index 9f6b33bba7..e385a84150 100644 --- a/libs/utils/CMakeLists.txt +++ b/libs/utils/CMakeLists.txt @@ -37,7 +37,6 @@ set(DIST_HDRS ${PUBLIC_HDR_DIR}/${TARGET}/PrivateImplementation-impl.h ${PUBLIC_HDR_DIR}/${TARGET}/SingleInstanceComponentManager.h ${PUBLIC_HDR_DIR}/${TARGET}/Slice.h - ${PUBLIC_HDR_DIR}/${TARGET}/SpinLock.h ${PUBLIC_HDR_DIR}/${TARGET}/StructureOfArrays.h ${PUBLIC_HDR_DIR}/${TARGET}/unwindows.h ) diff --git a/libs/utils/benchmark/benchmark_allocators.cpp b/libs/utils/benchmark/benchmark_allocators.cpp index b37e5c0232..f243d7953c 100644 --- a/libs/utils/benchmark/benchmark_allocators.cpp +++ b/libs/utils/benchmark/benchmark_allocators.cpp @@ -38,7 +38,6 @@ protected: utils::Arena, LockingPolicy::NoLock> mPoolAllocatorNoLock; utils::Arena, std::mutex> mPoolAllocatorStdMutex; utils::Arena, utils::Mutex> mPoolAllocatorUtilsMutex; - utils::Arena, LockingPolicy::SpinLock> mPoolAllocatorSpinlock; utils::Arena, LockingPolicy::NoLock> mPoolAllocatorAtomic; }; @@ -48,7 +47,6 @@ Allocators::Allocators() : mPoolAllocatorNoLock("nolock", POOL_ITEM_COUNT * sizeof(Payload)), mPoolAllocatorStdMutex("std::mutex", POOL_ITEM_COUNT * sizeof(Payload)), mPoolAllocatorUtilsMutex("utils::Mutex", POOL_ITEM_COUNT * sizeof(Payload)), - mPoolAllocatorSpinlock("spinlock", POOL_ITEM_COUNT * sizeof(Payload)), mPoolAllocatorAtomic("atomic", POOL_ITEM_COUNT * sizeof(Payload)) { } @@ -81,15 +79,6 @@ BENCHMARK_DEFINE_F(Allocators, poolAllocator_utils_mutex)(benchmark::State& stat } } -BENCHMARK_DEFINE_F(Allocators, poolAllocator_spinlock)(benchmark::State& state) { - auto& pool = mPoolAllocatorSpinlock; - PerformanceCounters pc(state); - for (auto _ : state) { - Payload* p = pool.alloc(1); - pool.free(p); - } -} - BENCHMARK_DEFINE_F(Allocators, poolAllocator_atomic)(benchmark::State& state) { auto& pool = mPoolAllocatorAtomic; PerformanceCounters pc(state); @@ -107,10 +96,6 @@ BENCHMARK_REGISTER_F(Allocators, poolAllocator_utils_mutex) ->ThreadRange(1, 4) ->Threads(benchmark::CPUInfo::Get().num_cpus * 2); -BENCHMARK_REGISTER_F(Allocators, poolAllocator_spinlock) - ->ThreadRange(1, 4) - ->Threads(benchmark::CPUInfo::Get().num_cpus * 2); - BENCHMARK_REGISTER_F(Allocators, poolAllocator_atomic) ->ThreadRange(1, 4) ->Threads(benchmark::CPUInfo::Get().num_cpus * 2); diff --git a/libs/utils/benchmark/benchmark_mutex.cpp b/libs/utils/benchmark/benchmark_mutex.cpp index b455d424cd..71e4f21caf 100644 --- a/libs/utils/benchmark/benchmark_mutex.cpp +++ b/libs/utils/benchmark/benchmark_mutex.cpp @@ -41,15 +41,6 @@ static void BM_utils_mutex(benchmark::State& state) { } } -static void BM_spinlock(benchmark::State& state) { - static LockingPolicy::SpinLock l; - PerformanceCounters pc(state); - for (auto _ : state) { - l.lock(); - l.unlock(); - } -} - BENCHMARK(BM_std_mutex) ->Threads(1) ->Threads(2) @@ -61,9 +52,3 @@ BENCHMARK(BM_utils_mutex) ->Threads(2) ->Threads(8) ->ThreadPerCpu(); - -BENCHMARK(BM_spinlock) - ->Threads(1) - ->Threads(2) - ->Threads(8) - ->ThreadPerCpu(); diff --git a/libs/utils/include/utils/Allocator.h b/libs/utils/include/utils/Allocator.h index 82d1d1ccee..3564a417b3 100644 --- a/libs/utils/include/utils/Allocator.h +++ b/libs/utils/include/utils/Allocator.h @@ -22,7 +22,6 @@ #include #include #include -#include #include #include @@ -478,7 +477,6 @@ struct NoLock { void unlock() noexcept { } }; -using SpinLock = utils::SpinLock; using Mutex = utils::Mutex; } // namespace LockingPolicy diff --git a/libs/utils/include/utils/JobSystem.h b/libs/utils/include/utils/JobSystem.h index 041c6d9c55..b3bf83e81a 100644 --- a/libs/utils/include/utils/JobSystem.h +++ b/libs/utils/include/utils/JobSystem.h @@ -387,7 +387,7 @@ private: uint8_t mParallelSplitCount = 0; // # of split allowable in parallel_for Job* mRootJob = nullptr; - utils::SpinLock mThreadMapLock; // this should have very little contention + utils::Mutex mThreadMapLock; // this should have very little contention tsl::robin_map mThreadMap; }; diff --git a/libs/utils/include/utils/SpinLock.h b/libs/utils/include/utils/SpinLock.h deleted file mode 100644 index e7ce00afac..0000000000 --- a/libs/utils/include/utils/SpinLock.h +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef TNT_UTILS_SPINLOCK_H -#define TNT_UTILS_SPINLOCK_H - -#include - -#include - -#include -#include - -#include -#include - -namespace utils { -namespace details { - -class SpinLock { - std::atomic_flag mLock = ATOMIC_FLAG_INIT; - -public: - void lock() noexcept { - UTILS_PREFETCHW(&mLock); -#ifdef __ARM_ACLE - // we signal an event on this CPU, so that the first yield() will be a no-op, - // and falls through the test_and_set(). This is more efficient than a while { } - // construct. - UTILS_SIGNAL_EVENT(); - do { - yield(); - } while (mLock.test_and_set(std::memory_order_acquire)); -#else - goto start; - do { - yield(); -start: ; - } while (mLock.test_and_set(std::memory_order_acquire)); -#endif - } - - void unlock() noexcept { - mLock.clear(std::memory_order_release); -#ifdef __ARM_ARCH_7A__ - // on ARMv7a SEL is needed - UTILS_SIGNAL_EVENT(); - // as well as a memory barrier is needed - __dsb(0xA); // ISHST = 0xA (b1010) -#else - // on ARMv8 we could avoid the call to SE, but we'd need to write the - // test_and_set() above by hand, so the WFE only happens without a STRX first. - UTILS_BROADCAST_EVENT(); -#endif - } - -private: - inline void yield() noexcept { - // on x86 call pause instruction, on ARM call WFE - UTILS_WAIT_FOR_EVENT(); - } -}; -} // namespace details - -#if UTILS_HAS_SANITIZE_THREAD -// Active spins with atomics slow down execution too much under ThreadSanitizer. -using SpinLock = Mutex; -#elif defined(__ARM_ARCH_7A__) -// We've had problems with "wfe" on some ARM-V7 devices, causing spurious SIGILL -using SpinLock = Mutex; -#else -using SpinLock = details::SpinLock; -#endif - -} // namespace utils - -#endif // TNT_UTILS_SPINLOCK_H diff --git a/libs/utils/src/JobSystem.cpp b/libs/utils/src/JobSystem.cpp index 00ada7c51b..9473926a54 100644 --- a/libs/utils/src/JobSystem.cpp +++ b/libs/utils/src/JobSystem.cpp @@ -293,7 +293,7 @@ void JobSystem::wakeOne() noexcept { } inline JobSystem::ThreadState& JobSystem::getState() noexcept { - std::lock_guard lock(mThreadMapLock); + std::lock_guard lock(mThreadMapLock); auto iter = mThreadMap.find(std::this_thread::get_id()); ASSERT_PRECONDITION(iter != mThreadMap.end(), "This thread has not been adopted."); return *iter->second; @@ -585,7 +585,7 @@ void JobSystem::runAndWait(JobSystem::Job*& job) noexcept { void JobSystem::adopt() { const auto tid = std::this_thread::get_id(); - std::unique_lock lock(mThreadMapLock); + std::unique_lock lock(mThreadMapLock); auto iter = mThreadMap.find(tid); ThreadState* const state = iter == mThreadMap.end() ? nullptr : iter->second; lock.unlock(); @@ -618,7 +618,7 @@ void JobSystem::adopt() { void JobSystem::emancipate() { const auto tid = std::this_thread::get_id(); - std::lock_guard lock(mThreadMapLock); + std::lock_guard lock(mThreadMapLock); auto iter = mThreadMap.find(tid); ThreadState* const state = iter == mThreadMap.end() ? nullptr : iter->second; ASSERT_PRECONDITION(state, "this thread is not an adopted thread");