koobas.hobune.stream
Optimising a function that sometimes takes too long to run
I became aware of a problem a while ago in Unity, where if you call a function to return a value, if the calculation takes too long, the return value just returns, 0, null or whatever it was previously. Generally, whenever this issue crops up, it's in prototype code, and it's fairly easy to optimise out by altering the function.
However, currently I am working on a prototype, and to me what seems like an innoucous function is sufferring from the same problem. I originally created a dictionary with a string key, but after I changed some other designs, I just changed the key to an int, meaning to change to a list of lists later, and looped over the dictionary.
The code looked like this:
//Setting up dictionary and biomes
private Dictionary<int, List<Biome>> biomeSetDictionary = new Dictionary<int, List<Biome>>();
// Use this for initialization
void Start()
{
List<Biome> reefSet = new List<Biome>();
List<Biome> safeShallowsSet = new List<Biome>();
List<Biome> ventSet = new List<Biome>();
List<Biome> kelpForestSet = new List<Biome>();
List<Biome> bloodKelpSet = new List<Biome>();
List<Biome> grassyPlateauSet = new List<Biome>();
List<Biome> islandSet = new List<Biome>();
for (int i = 0; i < biomes.Count; i++)
{
switch (biomes[i].bSet)
{
case Globals.BiomeSet.BloodKelp:
bloodKelpSet.Add(biomes[i]);
break;
case Globals.BiomeSet.GrassyPlateau:
grassyPlateauSet.Add(biomes[i]);
break;
case Globals.BiomeSet.Islands:
islandSet.Add(biomes[i]);
break;
case Globals.BiomeSet.KelpForest:
kelpForestSet.Add(biomes[i]);
break;
case Globals.BiomeSet.Reefs:
reefSet.Add(biomes[i]);
break;
case Globals.BiomeSet.SafeShallows:
safeShallowsSet.Add(biomes[i]);
break;
case Globals.BiomeSet.Vents:
ventSet.Add(biomes[i]);
break;
}
}
biomeSetDictionary.Add(0, bloodKelpSet); // 2 entries
biomeSetDictionary.Add(1, grassyPlateauSet); // 4 entries
biomeSetDictionary.Add(2, islandSet); // 2 entries
biomeSetDictionary.Add(3, kelpForestSet); // 4 entries
biomeSetDictionary.Add(4, reefSet); // 4 entries
biomeSetDictionary.Add(5, safeShallowsSet); // 4 entries
biomeSetDictionary.Add(6, ventSet); // 2 entries
}
public List<Biome> FilterOwnedSets(Globals.BiomeOwner team)
{
List<Biome> filtered = new List<Biome>();
Globals.BiomeOwner testOwner;
bool keep = true;
for(int i = 0; i < biomeSetDictionary.Count; i++)
{
testOwner = biomeSetDictionary[i][0].bOwner;
if(testOwner == team)
{
for (int j = 0; j < biomeSetDictionary[i].Count; j++)
{
if (testOwner != biomeSetDictionary[i][j].bOwner)
{
keep = false;
break;
}
}
if (keep)
{
filtered.AddRange(biomeSetDictionary[i]);
}
}
}
return filtered;
}
When playing the game, I would see the number of biomes owned in completed sets go - 0,0,0,0,0,0,4,0,4,8,0,0,0,0,0
The first 4 was when I completed my first set, but most other time the result was zero. So I changed my dictionary to a list of lists like follows:
private List<List<Biome>> biomeSetList = new List<List<Biome>>();
// Use this for initialization
void Start()
{
List<Biome> reefSet = new List<Biome>();
List<Biome> safeShallowsSet = new List<Biome>();
List<Biome> ventSet = new List<Biome>();
List<Biome> kelpForestSet = new List<Biome>();
List<Biome> bloodKelpSet = new List<Biome>();
List<Biome> grassyPlateauSet = new List<Biome>();
List<Biome> islandSet = new List<Biome>();
for (int i = 0; i < biomes.Count; i++)
{
switch (biomes[i].bSet)
{
case Globals.BiomeSet.BloodKelp:
bloodKelpSet.Add(biomes[i]);
break;
case Globals.BiomeSet.GrassyPlateau:
grassyPlateauSet.Add(biomes[i]);
break;
case Globals.BiomeSet.Islands:
islandSet.Add(biomes[i]);
break;
case Globals.BiomeSet.KelpForest:
kelpForestSet.Add(biomes[i]);
break;
case Globals.BiomeSet.Reefs:
reefSet.Add(biomes[i]);
break;
case Globals.BiomeSet.SafeShallows:
safeShallowsSet.Add(biomes[i]);
break;
case Globals.BiomeSet.Vents:
ventSet.Add(biomes[i]);
break;
}
}
biomeSetList.Add(bloodKelpSet); // 2 entries
biomeSetList.Add(grassyPlateauSet); // 4 entries
biomeSetList.Add(islandSet); //2 entries
biomeSetList.Add(kelpForestSet); //4 entries
biomeSetList.Add(reefSet); //4 entries
biomeSetList.Add(safeShallowsSet); // 4 entries
biomeSetList.Add(ventSet); // 2 entries
}
The code in the FilterOwnedSets function remained identical, as you use the same methods to go through lists as dictionaries, but because we were iterating over list of lists instead of dictionary of lists, I got the following results:
0,0,0,0,2,6,6,0,14,0,14,14,0,22,22,22
So now it only takes too long some of the time, i didn't lose any territories at all during the run through, so the numbers should have always remained constant or gone up. Is there a good way to optimise my FilterOwnedSets function, or will I need to look at another way of doing this altogether?
You never reset the keep variable, so when you encounter the first list with a mismatching owner in it, nothing gets added after that.
The test case here has a dictionary with 7 keys containing lists of 3 items on average... hardly enough complexity to cause any problems to modern computers. In fact I've never heard of or encountered the problem you mention (long functions returning wrong values) in my 5 years of using Unity daily. Not even in that puzzle solver i made where a function took 10 minutes to solve some of the longest puzzles. I'd be interested to hear more if other people have encountered such a thing.
Thanks for the answer, it's always the little things. :(
As regards the ti$$anonymous$$g, this was something that I initially read on these answer pages, when looking for a reason the particularly long problem I was running (creating normal distribution) was producing the right results at the end of the function, but the value being used in the calling function by the resultant variable was always null.
By breaking the function down into 10 smaller methods, focusing on a part of the equation each, and calculating their bit as neccesary, the return values were always populated correctly, so I agreed with the reasoning.
I am not a highly experienced dev and have only been using unity 2 years, but my experience personally has agreed with the answer I read.
Also note I was bemused by the fact this function could take too long, I referred to it as innocuos in the main post :)
Cool. Glad to be of help. Sry if I sounded harsh :D I answered on my phone so i had to get straight to the point.
No worries, appreciate the help and didn't take it as harsh at all. When someone makes a statement that goes against personal experience, it's natural to question it.
Put this script to a worker thread. This post and this one explain quite a lot.
To optimize, you can also make all the lists be some specific initial size, it might use more memory than needed, but it is less likely that you will need to reallocate the whole list (which costs performance).
Outside the worker thread try not to blindly access the variable when it is locked since it will make unity's main thread wait for your worker thread to unlock the variable, which defeats the whole purpose of multithreading.
I tried making an array of arrays and ran that alongside the other method for comparison, but they both missed the same objects on the same loops. The code there is just a small sample of the class that I cut out and used for testing this issue, the actual class is much larger and unfortunately cannot run in a worker thread due to code obligations (it runs on a gameobject) :)
I mean still use lists, but set their initial capacity to a bigger value to avoid reallocations.
That is the point. You have to think differently and make it not use not thread-safe unity APIs. The current sample you showed would perfectly work on a worker thread. Is there some other code missing in the script provided?
No other code related to this problem no. The Biomes being iterated over are the monobehaviour enabled scripts of the biome gameobjects. I do not wish to play russian roulette with monobehaviour scripts in a worker thread :) I appreciate your advice, and it is something I will look to use in the future, but it is not appropriate in this particular instance.
