- Home /
Multithreading freezes editor
I outsourced my terrain generation into threads. The threads don't use any unity api stuff and all variables which the thread and the main thread have access to are thread safed via a lock.
My problem is that after a few seconds (sometime 3-4s and sometimes 30s or 40s) the unity editor freezes. If I open up the task manager the cpu and memory percentages are still changing (cpu is about 40-50% and memory about 300mb).
I've already tried unmanged threads and managed thread with the C# ThreadPool. The communication with the main thread goes over a status variable (which is locked) so if the task has finished the main thread collects the data and creates a mesh (every update the main thread checks for the status of the thread).
I've tried it on windows 10 with Unity 5.3/5.4 and on mac os x with unity 5.3/5.4.
Here's the shortened code:
Edit: Updated code, now using volatile
public enum SurfaceCalculationStatus{
NewJob,
Processing,
DataReady,
Ready,
Aborted
}
public class SurfaceCalculation {
private SurfaceRenderType srt;
//Some members...
private volatile SurfaceCalculationStatus scs;
public SurfaceCalculation(){
scs = SurfaceCalculationStatus.Ready;
ThreadPool.QueueUserWorkItem((o) => { ThreadFunction();});
}
public void Init(int x, int z, int size, int gridSize, SurfaceRenderType srt, Chunk c){
//Some inits...
scs = SurfaceCalculationStatus.NewJob;
}
private void ThreadFunction()
{
while (true) {
if(scs == SurfaceCalculationStatus.Aborted)
{
return;
}
if (scs == SurfaceCalculationStatus.NewJob) {
scs = SurfaceCalculationStatus.Processing;
//Heavy calculations
scs = SurfaceCalculationStatus.DataReady;
}
Thread.Sleep (100);
}
}
public void AbortThread(){
scs = SurfaceCalculationStatus.Aborted;
}
//Some Getter & Setters...
public void SetReady(){
scs = SurfaceCalculationStatus.Ready;
}
public bool IsReady(){
return scs == SurfaceCalculationStatus.Ready;
}
public bool IsData(){
return scs == SurfaceCalculationStatus.DataReady;
}
}
Edit: This is the class/logic which manages the threads. If I comment out the line
sc.SetReady ();
at the Update() method there is no freezing.
public class SurfaceFactory {
private static List<Chunk>[] waitingQueue;
private static SurfaceCalculation[] inProgressQueue;
public static void Init(){
waitingQueue = new List<Chunk>[WorldSettings.lvlRadii.Length];
inProgressQueue = new SurfaceCalculation[SystemInfo.processorCount * 8];
for (int i = 0; i < inProgressQueue.Length; i++) {
inProgressQueue [i] = new SurfaceCalculation ();
}
for (int i = 0; i < waitingQueue.Length; i++) {
waitingQueue [i] = new List<Chunk> ();
}
}
public static void CreateSurface(Chunk c){
if (GetFreeThreadCound() > 0) {
StartSurfaceCalculation (c);
} else {
waitingQueue[c.GetLOD()].Add (c);
}
}
public static void Update(){
for (int i = inProgressQueue.Length - 1; i >= 0; i--) {
SurfaceCalculation sc = inProgressQueue [i];
if (sc.IsData ()) {
//Thread finished
/*Chunk c = sc.GetChunk();
Mesh mesh = new Mesh ();
mesh.vertices = sc.GetVertices ();
mesh.triangles = sc.GetIndices ();
mesh.RecalculateNormals ();
c.SetMesh (mesh);*/
sc.SetReady ();
}
}
int numberOfWaitingChunks = 0;
for (int i = 0; i < waitingQueue.Length; i++) {
numberOfWaitingChunks += waitingQueue [i].Count;
}
int numberFreeThreads = GetFreeThreadCound ();
while (numberOfWaitingChunks > 0 && numberFreeThreads > 0){
for (int i = 0; i < waitingQueue.Length; i++) {
if (waitingQueue [i].Count > 0) {
Chunk c = waitingQueue [i][0];
waitingQueue[i].RemoveAt (0);
StartSurfaceCalculation (c);
numberFreeThreads--;
}
}
}
}
public static void AbortThreads(){
for (int i = 0; i < inProgressQueue.Length; i++) {
inProgressQueue [i].AbortThread ();
}
}
private static void StartSurfaceCalculation(Chunk c){
SurfaceCalculation sc = GetFreeThread ();
if (sc != null) {
sc.Init (c.GetChunkXMeters (false), c.GetChunkZMeters (false), c.GetSize (), c.GetGridSize (), c.GetSurfaceRenderType (), c);
} else {
waitingQueue [c.GetLOD ()].Add (c);
}
}
private static int GetFreeThreadCound(){
int c = 0;
for (int i = 0; i < inProgressQueue.Length; i++) {
if (inProgressQueue [i].IsReady ())
c++;
}
return c;
}
private static SurfaceCalculation GetFreeThread(){
for (int i = 0; i < inProgressQueue.Length; i++) {
if (inProgressQueue [i].IsReady ())
return inProgressQueue [i];
}
return null;
}
}
Answer by Bunny83 · Aug 26, 2016 at 07:35 PM
You never want to lock the whole code of the threaded function. That will lock every other thread that tries to aquire a lock while your function still runs. That completely destroys the purpose of using a thread in the first place and is prone for creating dead-locks.
How do you actually use that class? How do you managed those jobs? In such cases you usually don't need any locks as long as you declare your state variable as volatile. As long as the sequence when someone is allowed to change the state of the variable is clear there are no locks required.
Note: This only works for variable read / write operations which are "atomic". This is only given for primitive types (excluding double, long, ulong, decimal). Since the default underlying type for enums is "int", reading and writing an enum is an atomic operation. "volatile" ensures that the compiler does not do any optimisations on that variable.
So as long as only one thread will write the variable everything is fine. In your case the order is clear:
state Thread who has write-permission
-----------------------------------------
Ready Main thread --> can change to "NewJob" or "Aborted"
NewJob Worker thread --> can change to "Processing" or "DataReady"
Processing Worker thread --> can change to "DataReady"
DataReady Main thread --> can change to "Ready" or "NewJob" or "Aborted"
However to be on the safe side it's better to declare seperate variables for commands and state. Commands are only set by the main thread and read by the worker and the state is only set by the worker and read by the main thread,
Anyways if you want to use a lock (maybe to allow setting the aborted state while still processing), make sure you hold the lock as short as possible.
lock (locker)
{
scs = SurfaceCalculationStatus.Processing;
}
//Heavy calculations
lock (locker)
{
if (scs ==SurfaceCalculationStatus.Aborted) // important to not overwrite an external abort request.
return;
scs = SurfaceCalculationStatus.DataReady;
}
Also keep in mind that you always have to terminate the threads when running inside the editor. The playmode does not affect running threads. This can lead to massive problems inside the editor.
It's generally not recommended to use Thread.Abort, however in the case of Unity it might be a good idea as backup procedure.
It's generally adviced to have a dedicated "abort" variable which should be checked by the worker frequently during operation to terminate the thread within a short time.
Thanks for your detailed answer. I changed my code so now I'm using volatile for the status which should work. I also edited my questions so now there's the updated code + the code of my manager class which creates the threads and manages them. You can also see when I quit the game the status of every thread will be changed which causes a return in the ThreadFunction so all threads will be shut down. Furthermore I recognized that if I comment out the line`sc.SetReady ();` in SurfaceFactory.Update() there is no freezing anymore. So something goes wrong when I reuse the thread to calculate some more data.
You still seem to use the "scs" variable to ter$$anonymous$$ate the thread from outside, As i said that can be dangerous when you don't check if you can ter$$anonymous$$ate it at the moment. If the thread currently has the authority over the scs variable you shouldn't touch it from the outside.
Furthermore you shouldn't use a threadpool in your case. A threadpool is ment to manage the threads for you. You are supposed to queue work items that do process something and when the work is done the thread is returned to the pool. Since your thread function has an infinite loop the thread is never returned to the pool unless you ter$$anonymous$$ate your loop. In that case you should create the Thread yourself.
Also ins$$anonymous$$d of using "sleep" it's way better to use a AutoResetEvent. It allows a thread to wait for an event to occur. They are commonly used to let a thread sleep until it's required.
I have quickly written a general purpose JobQueue class. Where i implemented almost everything i've mentioned before. All you have to do is derive your own job class from "JobItem" and override the DoWork method. A Job should include all data that is needed to process the job and a place to store the result.