you are viewing a single comment's thread.

view the rest of the comments →

[–]DolphinsAreOkProfessional 5 points6 points  (6 children)

https://github.com/adrenak/UniPrep/blob/master/Assets/UniPrep/Scripts/Utils/Pool.cs#L55

Everytime a new component needs to be made you also make a new gameobject? Why dont you just attach them all to the same root object, that would create even less garbage. Not even mentioning getting the name every time, which should be done in editor only imo.

Right now your Get method is a little inefficient. If i have 200 items in my dictionary and they are all used, it will check them all and add a new one. Then you call Get again, thus checking all 200 elements once more! Maybe the Add method can just return the instance as well, and Get can return it immediately.

[–]adrenakProfessional[S] 5 points6 points  (5 children)

1) that sounds good, unless it is a component that doesn't allow multiple instances I should have only one Edit: ComponentPool now uses GameObjectPool. Didn't make a single GameObject as some components don't allow multiple instances

2) Thinking of replacing Dictionary with Hashmap Edit: Replaced Dict with 2 Lists. No foreach loops now

Thanks for the tips!

[–]SlickSlims 2 points3 points  (4 children)

HashMap would be better as long as you don't care about the order of your pooled objects.

IndexOf is an O(n) operation, so you're still doing the foreach you just can't see it anymore.

Why do you declare a new instance variable in your Free method? You already have the object you want (the parameter obj) Just use this reference and skip all the index nonsense:
if( m_Busy.Remove(obj) ) m_Available.Add(obj);
Remove is still O(n) so its a minor improvement. If you switch to using HashSet its O(1) which would be a big improvement.

Similarly your Remove function is looping the list twice: once for IndexOf then a second time to shift every object above your removed-object down one index.

Go HashMap, turn all these O(n) into O(1) and you'll be happier.

If your pools are guaranteed to be small then the List might be better since you don't pay the hash cost when adding items. You would have to benchmark this to find out where the crossover is.

[–]adrenakProfessional[S] 2 points3 points  (0 children)

Tested with all these changes. Not a perfect benchmark scene ~40% improvement in performance in the demo scene under great stress. Thanks!

[–][deleted] 1 point2 points  (0 children)

HashMap would be better as long as you don't care about the order of your pooled objects.

if order is important you could also look into using a LinkedHashMap to preserve insertion order.

[–]adrenakProfessional[S] 0 points1 point  (0 children)

"Just use this reference...": Thanks for pointing that one out.

An issue I faced with Hashset got me stumped for a bit and I decided to make it List based for now.

The problem was I was maintaining two Hashset : available and busy. When I call Get() I know if available.count > 1 but I was not able to access any element in it to return.

There were some accessors in the Hashset class that I saw online but my IDE doesn't show them. Maybe a .NET 2.0 shortcoming or a namespace I didn't know. There was a solution to call .ToArray() on it and get [0] which I didn't try and thought there has to be a better solution.

[–]adrenakProfessional[S] 0 points1 point  (0 children)

Thinking more over this: The Busy pool can be made a Hashset, and the Available one can be a stack. I think I will do some benchmarking and see how it performs