diff --git a/NEW_RELEASE_NOTES.md b/NEW_RELEASE_NOTES.md index bbe05da77f..d232447b65 100644 --- a/NEW_RELEASE_NOTES.md +++ b/NEW_RELEASE_NOTES.md @@ -10,3 +10,4 @@ appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md). - Metal: fix some shader artifacts by disabling fast math optimizations. - backend: remove `atan2` overload which had a typo and wasn't useful. Fixes b/320856413. +- 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");