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.

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

[Core] ThreadPool: shutdown idle threads #302

Merged
merged 6 commits into from Jan 19, 2019
Merged

Conversation

@Eideren
Copy link
Collaborator

Eideren commented Dec 27, 2018

ThreadPool never close off the threads it spawns, see Discord discussion for more details.

@Kryptos-FR I did a couple of changes, how would you deal with synchronization ?

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Dec 27, 2018

Also, we just ignore exceptions created by the delegates it runs, shouldn't we handle them in some way ?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 27, 2018

Yes I also noticed this exception swallowing when we discussed about it in Discord channel, not good...

Any proposal?
What do we want for default behavior if exception, do we keep running or make the app crash?
Should we offer an event? (i.e. Dispatcher.UnhandledException)
Or simply let it go to Application.UnhandledException?

PS: Let me know if you prefer to merge this one first and do exception later in a separate PR, or deal with it as part of this PR.

@xen2 xen2 self-requested a review Dec 27, 2018
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Dec 27, 2018

Also concerning synchronization, please keep the SpinLock.

This is highly performance critical (we use that for microtasks and spawn hundreds of them per frame).
In most case the wait should be very brief and SpinLock has many advantages for that (very fast if no lock, value type, etc...)
The engineer who worked on it spend quite some time doing measurements/tests.
We don't want to change that without further test/measurements and a good reason to do so.

sources/core/Xenko.Core/Threading/ThreadPool.cs Outdated Show resolved Hide resolved
sources/core/Xenko.Core/Threading/ThreadPool.cs Outdated Show resolved Hide resolved
sources/core/Xenko.Core/Threading/ThreadPool.cs Outdated Show resolved Hide resolved
sources/core/Xenko.Core/Threading/ThreadPool.cs Outdated Show resolved Hide resolved
Eideren added 2 commits Dec 27, 2018

if (activeThreadCount + 1 >= workers.Count && workers.Count < maxThreadCount)
var curWorkingCount = Interlocked.CompareExchange(ref workingCount, 0, 0);
var curAliveCount = Interlocked.CompareExchange(ref aliveCount, 0, 0);

This comment has been minimized.

Copy link
@xen2

xen2 Jan 18, 2019

Member

@Kryptos-FR I think it looks confusing.
I would say, let's go back to a more simple version (while making sure each var is read only once):

var curAliveCount = aliveThreadCount;
if (workingCount + 1 >= curAliveCount && curAliveCount < maxThreadCount)
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 18, 2019

BTW I can think of at least one race condition & potential deadlock:

If all threads are finishing, and they are after the while loop but before the Interlocked.Decrement(ref aliveCount).
QueueWorkItem won't add the item (because curAliveCount == maxThreadCount still) but then the other thread finishes and everything is stuck unless another task is queued.
I guess risk is higher if maxThreadCount is very low.

Probably unlikely, but multithreading program often tend to have those worst case you think are never possible once in a while given enough time :) (esp. since all the threads might be started at approx the same time)

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jan 18, 2019

I could also compute the result of the time delta comparison outside of the spinlock, might as well trade some precision with shorter spinlock duration, what do you guys think ?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 19, 2019

@Eideren
If you move the comparison and aliveCount-- outside of the spinLock, it will go back to previous situation with the race condition.
I suppose you have something slightly different in mind? (i.e. release lock to do the comparison and then take the look again if it looks like we are really really exiting and do the test+possible exit again? Sounds good to me)

@Eideren

This comment has been minimized.

Copy link
Collaborator Author

Eideren commented Jan 19, 2019

I wasn't as clear as I should have, here's what I meant in my previous message

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 19, 2019

@Eideren OK got it, looks good to me!

…double->long conversion)
@xen2 xen2 merged commit 993704f into xenko3d:master Jan 19, 2019
2 checks passed
2 checks passed
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 19, 2019

Merged, thanks!

@Eideren Eideren deleted the Eideren:threadpool_timeout branch Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.