Scarfski/filament
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

remove all uses of our custom spinlock

Browse Source 
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]
This commit is contained in:
Mathias Agopian
2024-01-19 10:35:49 -08:00
committed by Sungun Park
parent 0736f3c3b3
commit 7b384fb5e8
9 changed files with 11 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
NEW_RELEASE_NOTES.md
View File

@@ -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.

9
filament/src/details/ColorGrading.cpp
View File

@@ -30,11 +30,12 @@
#include <math/vec4.h>
#include <utils/JobSystem.h>
#include <utils/SpinLock.h>
#include <utils/Mutex.h>
#include <utils/Systrace.h>
#include <cmath>
#include <cstdlib>
#include <mutex>
#include <tuple>
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<utils::SpinLock> lock(configLock);
std::lock_guard<utils::Mutex> 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<utils::SpinLock> lock(configLock);
std::lock_guard<utils::Mutex> lock(configLock);
config = c;
}
half4* UTILS_RESTRICT p = (half4*) data + b * config.lutDimension * config.lutDimension;

1
libs/utils/CMakeLists.txt
View File

@@ -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
)

15
libs/utils/benchmark/benchmark_allocators.cpp
View File

@@ -38,7 +38,6 @@ protected:
utils::Arena<utils::ObjectPoolAllocator<Payload>, LockingPolicy::NoLock> mPoolAllocatorNoLock;
utils::Arena<utils::ObjectPoolAllocator<Payload>, std::mutex> mPoolAllocatorStdMutex;
utils::Arena<utils::ObjectPoolAllocator<Payload>, utils::Mutex> mPoolAllocatorUtilsMutex;
utils::Arena<utils::ObjectPoolAllocator<Payload>, LockingPolicy::SpinLock> mPoolAllocatorSpinlock;
utils::Arena<utils::ThreadSafeObjectPoolAllocator<Payload>, 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<Payload>(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);

15
libs/utils/benchmark/benchmark_mutex.cpp
View File

@@ -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();

2
libs/utils/include/utils/Allocator.h
View File

@@ -22,7 +22,6 @@
#include <utils/debug.h>
#include <utils/memalign.h>
#include <utils/Mutex.h>
#include <utils/SpinLock.h>
#include <atomic>
#include <cstddef>
@@ -478,7 +477,6 @@ struct NoLock {
void unlock() noexcept { }
};
using SpinLock = utils::SpinLock;
using Mutex = utils::Mutex;
} // namespace LockingPolicy

2
libs/utils/include/utils/JobSystem.h
View File

@@ -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<std::thread::id, ThreadState *> mThreadMap;
};

90
libs/utils/include/utils/SpinLock.h
View File

@@ -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 <utils/compiler.h>
#include <utils/Mutex.h>
#include <atomic>
#include <type_traits>
#include <assert.h>
#include <stddef.h>
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

6
libs/utils/src/JobSystem.cpp
View File

@@ -293,7 +293,7 @@ void JobSystem::wakeOne() noexcept {
}
inline JobSystem::ThreadState& JobSystem::getState() noexcept {
std::lock_guard<utils::SpinLock> lock(mThreadMapLock);
std::lock_guard<utils::Mutex> 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<utils::SpinLock> lock(mThreadMapLock);
std::unique_lock<utils::Mutex> 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<utils::SpinLock> lock(mThreadMapLock);
std::lock_guard<utils::Mutex> 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");