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

fix a race in jobsystem

Browse Source 
We were decrementing activeJobCount after removing the job from the
queue, which could cause other threads in the pool to preempt us before
the decrement, causing them to spin forever trying to get a non-existant
job, until the decrement actually happened.

Now we always decrement first and fix-up the count if we couldn't get
a job from the queues. The race is inverted, and doesn't cause threads
to spin a long time.
This commit is contained in:
Mathias Agopian
2021-09-13 14:20:08 -07:00
committed by Mathias Agopian
parent 7f2c9c2326
commit 2feb0ad325
2 changed files with 39 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -323,11 +323,6 @@ bool JobSystem::execute(JobSystem::ThreadState& state) noexcept {
if (job) {
assert(job->runningJobCount.load(std::memory_order_relaxed) >= 1);
UTILS_UNUSED_IN_RELEASE
uint32_t activeJobs = mActiveJobs.fetch_sub(1, std::memory_order_relaxed);
assert(activeJobs); // whoops, we were already at 0
HEAVY_SYSTRACE_VALUE32("JobSystem::activeJobs", activeJobs - 1);
if (UTILS_LIKELY(job->function)) {
HEAVY_SYSTRACE_NAME("job->function");
job->function(job->storage, *this, job);
@@ -446,18 +441,8 @@ void JobSystem::run(Job*& job) noexcept {
ThreadState& state(getState());
// increase the active job count before we add the job to the queue, because otherwise
// the job could run and finish before the counter is incremented, which would trigger
// an assert() in execute(). Either way, it's not "wrong", but the assert() is useful.
uint32_t activeJobs = mActiveJobs.fetch_add(1, std::memory_order_relaxed);
put(state.workQueue, job);
HEAVY_SYSTRACE_VALUE32("JobSystem::activeJobs", activeJobs + 1);
// wake-up a thread if needed...
wakeOne();
// after run() returns, the job is virtually invalid (it'll die on its own)
job = nullptr;
}