Qriva / MonoBehaviourTree

Simple event driven Behaviour tree for Unity projects
MIT License
238 stars 17 forks source link

GC.Alloc every frame #16

Open laicasaane opened 3 weeks ago

laicasaane commented 3 weeks ago

Today I do a profiling and notice that the current usage of Variable<T>.AddListener and .RemoveListener causes at least 4 allocations every frame.


Each of these methods creates 1 allocation. 1 delegate instance is created out of OnVariableChange.

// IsSetCondition.cs
// NumberCondition.cs

public override void OnAllowInterrupt()
{
    [...] .GetVariable().AddListener(OnVariableChange);
}

public override void OnDisallowInterrupt()
{
    [...] .GetVariable().RemoveListener(OnVariableChange);
}

To eliminate these, we can just cache the delegate instances in private fields and use them instead.

public class IsSetCondition : Condition
{
    private readonly ChangeListener<bool> _boolListener;
    private readonly ChangeListener<GameObject> _gameObjectListener;
    private readonly ChangeListener<Transform> _transformListener;

    public IsSetCondition()
    {
        _boolListener = OnVariableChange;
        _gameObjectListener = OnVariableChange;
        _transformListener = OnVariableChange;
    }

    public override void OnAllowInterrupt()
    {
        [...] .GetVariable().AddListener(_boolListener);
    }
}

Each of these also creates another allocation.

// Variable.cs

public void AddListener(ChangeListener<T> listener)
{
    listeners += listener;
}

public void RemoveListener(ChangeListener<T> listener)
{
    listeners -= listener;
}

I haven't understood the logic of Variable<T>.listeners so I can't propose a solution for now.

Qriva commented 3 weeks ago

I was not aware that passing method creates garbage at that point. Is it actually the case? because I used similar thing numerous times and I could not notice that somehow. Maybe because I did not pass them, but added directly to event. Cache of delegates may be solution for those if thats the case.

The bottom, I am aware that event (or actually every another event after first one) creates more and more garbage. The idea of listeners is that the tree or nodes can react to changes and it's key feature, because the whole tree is driven by them. Those things and other design flaws are the reason why I wanted to rewrite the whole thing with better user interface and functions, but it requires a lot of effort and there are not so many people who use it, or perhaps there are? To solve event listeners we would need rework with breaking changes and introduce interfaces I guess.

laicasaane commented 3 weeks ago

because I used similar thing numerous times and I could not notice that somehow.

The Profiler in Timeline mode will show GC.Allocs in pink. That is the best way to spot them without having to dig into deep levels of nested methods.

or perhaps there are?

It's hard to know. However, we can know the monthly downloads through OpenUPM.