Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GameObjectContext pool memory leak fix. Todo: cover by test #214

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kkohno
Copy link

@kkohno kkohno commented Feb 11, 2021

Thank you for sending a pull request! Please make sure you read the contribution guidelines

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • No compiler errors or warnings

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Issue Number: #118

What is the new behavior?

only failing test

Does this introduce a breaking change?

  • Yes
  • No

Other information

On real case it work fine, when my system frequently reloads the scenes, but test are fails. I not sure, that this test available at all, because .Net Garbage Collector has no exact dispose method call time (correct me, if i am not right)...

On which Unity version has this been tested?

  • 2020.4 LTS
  • 2020.3
  • 2020.2
  • 2020.1
  • 2019.4 LTS
  • 2019.3
  • 2019.2
  • 2019.1
  • 2018.4 LTS

Scripting backend:

  • Mono
  • IL2CPP

Note: Every pull request is tested on the Continuous Integration (CI) system to confirm that it works in Unity.

Ideally, the pull request will pass ("be green"). This means that all tests pass and there are no errors. However, it is not uncommon for the CI infrastructure itself to fail on specific platforms or for so-called "flaky" tests to fail ("be red"). Each CI failure must be manually inspected to determine the cause.

CI starts automatically when you open a pull request, but only Releasers/Collaborators can restart a CI run. If you believe CI is giving a false negative, ask a Releaser to restart the tests.

@kkohno kkohno changed the title GameObjectContext pool memory leak test failure issue #118 GameObjectContext pool memory leak test failure Feb 11, 2021
@Mathijs-Bakker Mathijs-Bakker marked this pull request as draft February 11, 2021 11:50
@Mathijs-Bakker Mathijs-Bakker added do not close Do not close this issue do not merge Do not merge PR until further notice labels Feb 11, 2021
@kkohno
Copy link
Author

kkohno commented Feb 11, 2021

Seems that StaticMemoryPoolRegistry is cause of leaks. It is only Editor feature. But in some cases it is big trouble. For example - the scanning anti-cheat system for editor...

@kkohno
Copy link
Author

kkohno commented Feb 11, 2021

Maybe WeakReference is key to solve this:

StaticMemoryPoolRegistry.cs:

namespace Zenject
{
#if UNITY_EDITOR
    public static class StaticMemoryPoolRegistry
    {
        public static event Action<IMemoryPool> PoolAdded = delegate {};
        public static event Action<IMemoryPool> PoolRemoved = delegate {};

        readonly static List<WeakReference> _pools = new List<WeakReference>();

        public static IEnumerable<WeakReference> Pools
        {
            get { return _pools; }
        }

        public static void Add(IMemoryPool memoryPool)
        {
            _pools.Add(new WeakReference(memoryPool));
            PoolAdded(memoryPool);
        }

        public static void Remove(IMemoryPool memoryPool)
        {
            _pools.RemoveWithConfirm(new WeakReference(memoryPool));
            PoolRemoved(memoryPool);
        }
    }
#endif
}

@kkohno
Copy link
Author

kkohno commented Feb 11, 2021

yes, my code is working... but i changed MpmView.cs too, for output correct pools and exclude memory leaks. I can make pool request

@kkohno kkohno changed the title GameObjectContext pool memory leak test failure GameObjectContext pool memory leak fix. Todo: cover by test Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not close Do not close this issue do not merge Do not merge PR until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants