Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added more robust shutdown code for our dmJobThread implementation #8762

Merged
merged 1 commit into from
Apr 6, 2024
Merged

Added more robust shutdown code for our dmJobThread implementation #8762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 13 additions & 12 deletions engine/dlib/src/dlib/job_thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ struct JobThreadContext
#if defined(DM_HAS_THREADS)
dmMutex::HMutex m_Mutex;
dmConditionVariable::HConditionVariable m_WakeupCond;
int32_atomic_t m_Run;
bool m_Run;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for atomics if we already use a mutex.

#endif
};

Expand Down Expand Up @@ -89,16 +89,19 @@ static void PutDone(JobThreadContext* ctx, JobItem* item)
static void JobThread(void* _ctx)
{
JobThreadContext* ctx = (JobThreadContext*)_ctx;
while (dmAtomicGet32(&ctx->m_Run) != 0)
while (true)
{
JobItem item;
JobItem item = {};
Comment on lines +92 to +94
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between the loop check and the dmConditionVariable::Wait(), the main thread set run=0 and also signalled the wakeup condition...

{
DM_MUTEX_SCOPED_LOCK(ctx->m_Mutex);

if (!ctx->m_Run)
break;

while(ctx->m_Work.Empty())
{
dmConditionVariable::Wait(ctx->m_WakeupCond, ctx->m_Mutex);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we got here, the signal had already been sent which meant that the wait would then stall forever, as there wouldn't be any new signal coming.


if(dmAtomicGet32(&ctx->m_Run) == 0)
if (!ctx->m_Run)
return;
}
item = ctx->m_Work.Pop();
Expand Down Expand Up @@ -130,7 +133,7 @@ HContext Create(const JobThreadCreationParams& create_params)
#if defined(DM_HAS_THREADS)
context->m_ThreadContext.m_Mutex = dmMutex::New();
context->m_ThreadContext.m_WakeupCond = dmConditionVariable::New();
context->m_ThreadContext.m_Run = 1;
context->m_ThreadContext.m_Run = true;

uint32_t thread_count = dmMath::Min(create_params.m_ThreadCount, DM_MAX_JOB_THREAD_COUNT);
context->m_Threads.SetCapacity(thread_count);
Expand All @@ -152,14 +155,12 @@ void Destroy(HContext context)
return;

#if defined(DM_HAS_THREADS)
dmAtomicStore32(&context->m_ThreadContext.m_Run, 0);
{
DM_MUTEX_SCOPED_LOCK(context->m_ThreadContext.m_Mutex);
// Wake up the worker threads so it can exit and allow us to join
for (int i = 0; i < context->m_Threads.Size(); ++i)
{
dmConditionVariable::Signal(context->m_ThreadContext.m_WakeupCond);
}

context->m_ThreadContext.m_Run = false;;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we know that the Run and Wait() are within the same protected scope on the thread and here as well.


dmConditionVariable::Broadcast(context->m_ThreadContext.m_WakeupCond);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to wakeup all threads at once.

}

for (int i = 0; i < context->m_Threads.Size(); ++i)
Expand Down
4 changes: 2 additions & 2 deletions engine/dlib/src/dmsdk/dlib/condition_variable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ namespace dmConditionVariable

/*# signal condition variable
*
* Signal condition variable, effectively unblocks at least one of the waithing threads blocked
* Signal condition variable, effectively unblocks at least one of the waiting threads blocked
* by the condition variable.
*
* @name dmConditionVariable::Signal
Expand All @@ -82,7 +82,7 @@ namespace dmConditionVariable

/*# broadcast condition variable
*
* Broadcast condition variable, effectively unblocks all of the waithing threads blocked
* Broadcast condition variable, effectively unblocks all of the waiting threads blocked
* by the condition variable.
*
* @name dmConditionVariable::Broadcast
Expand Down