WAYN-Games / MGM-Ability

Other
88 stars 17 forks source link

Possible memory leak #3

Closed nicolasgramlich closed 4 years ago

nicolasgramlich commented 4 years ago

Back when you posted your original code in the Unity forums and I started massaging the code to work for my game, I noticed a real world memory leak scenario.

The EffectTriggerSystem effectively creates a NativeStream allocated via Allocator.TempJob here:

https://github.com/WAYNGROUP/MGM-Skill/blob/832fbea7701f8ef56f792e0f273d0182aec94191/Runtime/Systems/EffectTriggerSystems.cs#L121

https://github.com/WAYNGROUP/MGM-Skill/blob/832fbea7701f8ef56f792e0f273d0182aec94191/Runtime/Systems/EffectConsumerSystems.cs#L61-L65

which is fine as long as the EffectConsumerSystem actually runs and runs https://github.com/WAYNGROUP/MGM-Skill/blob/832fbea7701f8ef56f792e0f273d0182aec94191/Runtime/Systems/EffectConsumerSystems.cs#L159

... which might NOT happen when (if I recall correctly) i.e. no HealthEffect was produced in the current tick, then the ECS system doesn't run the i.e. HealthEffectConsumerSystem at all.

My solution was to add a [AlwaysUpdateSystem] to all the Consumer Systems in order to ensure proper cleanup.

TL;DR Basically the producer always runs, so we need to make sure the consumer also always cleans up.

WAYN-Games commented 4 years ago

Not sure the [AlwaysUpdateSystem] will fix the issue. The line https://github.com/WAYNGROUP/MGM-Skill/blob/832fbea7701f8ef56f792e0f273d0182aec94191/Runtime/Systems/EffectConsumerSystems.cs#L134-L137 should prevent the disposal of the stream if nothing was written to it. That line was added to avoid crashing unity with this kind of error :

Stack Trace of Crashed Thread 5508: 0x00007FF9BE5C1118 (f3d00ce56407e17404fc4c6a37bcc82) 4196B055CA5927BD ERROR: SymGetSymFromAddr64, GetLastError: 'Tentative d’accès à une adresse non valide.' (Address: 00007FF7BC1E626E)

This happen only when the job debugger is not enabled.

I need to make a proper test case for this.

PS : the leak detection does not raise any issue, so not sure if there is actually a memory leak. I need to explore the NateveStream source to check.

WAYN-Games commented 4 years ago

Testing the issue with the following code to ensure deallocation of the stream gives an error :

            // If the producer did not actually write anything to the stream, the native stream will not be flaged as created.
            // In that case we don't need to do anything.
            // Not doing this checks actually result in a non authrorized access to the memory and crashes Unity.
            if (!_effectStream.IsCreated)
            {
                _effectStream.Dispose();
                return;
            }

InvalidOperationException: The NativeArray has been deallocated, it is not allowed to access it Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle.CheckDeallocateAndThrow (Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle handle) (at :0) Unity.Collections.LowLevel.Unsafe.DisposeSentinel.Dispose (Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle& safety, Unity.Collections.LowLevel.Unsafe.DisposeSentinel& sentinel) (at :0) Unity.Collections.NativeStream.Dispose () (at Library/PackageCache/com.unity.collections@0.7.1-preview.3/Unity.Collections/NativeStream.cs:129) WaynGroup.Mgm.Skill.EffectConsumerSystem`2[EFFECT,EFFECT_CTX].OnUpdate () (at Packages/wayn-group.mgm.skill/Runtime/Systems/EffectConsumerSystems.cs:142)

WAYN-Games commented 4 years ago

Forum thread created to investigate further the native stream behavior, hopefully with hte help of unity guys : https://forum.unity.com/threads/nativestream-memory-leak.907571/

WAYN-Games commented 4 years ago

Explaination above actually solve the case where the tirgger system is not run. [AlwaysUpdateSystem] is a suitable fix for the memory leak issue when the consumer system is not run.