Array in nested struct not disposed after cloning - not sure if bug or just stupid?

Unity Engine
, ,
1

I have a class with a nested struct, this struct has a NativeArray. Everything works fine when I work with the original instance.

But as soon as I make a copy via copy constructor or Clone() I get the “A Native Collection has not been disposed, resulting in a memory leak.” error messages in the console even though, when stepping through, the collection is marked as “IsCreated = false” respectively it shows an IDisposedException in the debugger.

Unless I’m doing something illegal with the array to begin with this seems a bug in Collections 1.4.0. I hope this isn’t just because I’m not using the “recommended” version (1.2.4). Unity version is 2022.1.14f1.

Here’s the minimal test case I made:

public class TestNotDisposed
{
    private NestedStruct _nested = new(Allocator.Persistent);
    public TestNotDisposed() {}
    public TestNotDisposed(TestNotDisposed other) => _nested = new NestedStruct(other._nested);
    public void Dispose() => _nested.Dispose();
    private struct NestedStruct
    {
        private NativeArray<int> _values;
        public NestedStruct(Allocator allocator) => _values = new NativeArray<int>(4, allocator);
        public NestedStruct(NestedStruct other)
            : this(Allocator.Persistent) => _values.CopyFrom(other._values);
        public void Dispose() => _values.Dispose();
    }
}

To raise the issue just run this code anywhere, ie in a test case:

        var test = new TestNotDisposed();
        var copy1 = new TestNotDisposed(test);
 
        copy1.Dispose();
        test.Dispose();

I tried disabling Burst but that had no effect.
It works fine if I use Allocator.Temp but TempJob and Persistent raise this issue.

2

Problem is that lifecycle of any native container should be controlled by “something” that creates that container. And since IsCreated is not copied, resulting allocated container cannot be disposed. This is by design, and prevents memory leaks.

Temp allocations are cleared in the frame they were allocated automatically, so containers don’t even need to call .Dispose (although its good practise to be able to switch allocators fast);

Usually, you don’t want to put native containers inside native containers, or struct, etc;
If you do need it, best bet is to use unsafe collections, or write your own native container:

Writing custom native container can be tricky sometimes, or when working in unsafe context in general, so I highly recommend running Unity with:

"Unity.exe -debugallocator -projectPath pathToProject"

launch arguments when writing them.

Otherwise you’ll get hardcrashes on wrong accesses / unallocated memory blocks, etc;

3

Not sure I understand. In this example, TestNotDisposed creates and references the NestedStruct instance that holds the NativeArray. When you make a copy of that class, a new class instance with a new instance of NestedStruct with a new NativeArray is created - so the lifetime and ownership should be the same as when calling new TestNotDisposed().

In other words: there is no reference to a native container passed on to the cloned instance. Instead, a new container is allocated and the contents of the other container is copied via CopyFrom().

Your advice to not put native containers inside a struct runs counter to any IJob(ParallelFor). Please clarify what you mean by that. Especially why I would need to write a custom container for the above example. That feels like overengineering.

I understand the part of nested containers, though I don’t think that’s even possible anymore.

4

Okay, I’ll explain the “just stupid” part. :wink:

Notice the difference?

public class TestNotDisposed
{
    private NestedStruct _nested;
    public TestNotDisposed() => _nested = new NestedStruct(Allocator.Persistent);
    public TestNotDisposed(TestNotDisposed other) => _nested = new NestedStruct(other._nested);
    public void Dispose() => _nested.Dispose();

    private struct NestedStruct
    {
        private NativeArray<int> _values;
        public NestedStruct(Allocator allocator) => _values = new NativeArray<int>(4, allocator);

        public NestedStruct(NestedStruct other)
            : this(Allocator.Persistent) => _values.CopyFrom(other._values);

        public void Dispose() => _values.Dispose();
    }
}

The issue is that the _nested field is initialized with a new instance with a field initializer. Of course, that also happens when the copy constructor runs, which then replaces _nested with a second new instance, the first goes down the drain and isn’t disposed. In my defense: the field initializer was in another partial class file.

The fix is to simply leave the field uninitialized and instantiate it in the constructors.

5

Nesting native containers inside native containers without control over how memory is allocated - is a bad idea. Jobs aren’t native containers. They do not allocate unmanaged memory on their own.

That class is pretty much native container, except its not implemented properly as one.

Perhaps its some sort of bridge hack to the MonoBehaviour side, but even then, its better to avoid mixing managed & unmanaged memory allocations.

Alternative solution would be is to avoid allocation in the constructor completely. Instead pass container from outside as a reference. Whatever’s creating “TestNotDisposed” should be in charge of memory disposal.

6

I gave this another shot just to verify that I remembered it correctly.

It turns out that you can legally do this outside Jobs:

var arrayInArray = new NativeArray<NativeArray<int>>(0, Allocator.Temp);

But as soon as you have a field like this with nested arrays in a Job (even if the field isn’t used) you’ll get this exception:

InvalidOperationException: Unity.Collections.NativeArray`1[System.Int32] used in NativeArray<Unity.Collections.NativeArray`1[System.Int32]> must be unmanaged (contain no managed types) and cannot itself be a native container type.

Anyway, I think you may have been mislead by the minimalist nature of the test case. Consider the TestNotDisposed class as the manager for the native collections. It’s absolutely a viable concept as long as the class does not expose the native collections it contains (or only does so using AsParallelReader() / ReadOnly variants) and it clearly communicates that the instance needs disposal (ie implements IDisposable).

In my case, this class is GMesh with the nested struct GraphData that has five native arrays/lists holding vertices, edges, loops, faces and counts for each element type. So the GMesh is a wrapper around native types, and provides a managed interface (so to speak) for setting it up and modifying it but also running Jobs inside where it makes sense, such as creating a plane. GMesh itself is IDisposable and forwards the Dispose() to GraphData.

In cases where the GMesh finalizer runs, but the GraphData hasn’t been disposed (and because you cannot dispose native collections in the finalizer ) I’ll Debug.LogError a message that tries its best that it doesn’t go unnoticed. It’s in here at the very bottom.

Here’s an example how I use it in tests:

        using (var gMesh = GMesh.Cube(new GMeshCube(new int3(3))))
        {
            Validate.MeshElementCount(gMesh, 24, 96, 48, 26);
           
            using (var gMeshCopy1 = new GMesh(gMesh))
            using (var gMeshCopy2 = gMeshCopy1.Clone() as GMesh)
            {
                Validate.MeshElementCount(gMeshCopy1, 24, 96, 48, 26);
                Validate.MeshElementCount(gMeshCopy2, 24, 96, 48, 26);
                Assert.IsFalse(gMeshCopy1.Equals(gMesh));
                Assert.IsFalse(gMeshCopy2.Equals(gMesh));
                Assert.IsFalse(gMeshCopy1.Equals(gMeshCopy2));
            }
        }
7

I see. So, as you’ve figured out, allocating unmanaged memory in constructor is pretty much recipe for disaster.

Couple of things to consider:

  1. Remove constructors that do “cloning”. Use factory static method to generate new instance of the GMesh. Make it explicitly use Allocator as parameter to make sure user understands what allocations done. Hiding it internally is kinda nasty. Finalizer is nice, but in reality:
  2. GMesh should be struct. It looks like a native container that contains specific memory blocks. It acts like one, except, it isn’t one. And it can be rewritten into one by using unsafe collections. This will also avoid managed allocs upon cloning, since example is wasteful. Also, writing proper safety mechanisms would remove the need of finalizer.
  3. Remove GraphData and allocate memory blocks with GMesh as a native container instead.
  4. This will also allow passing whole GMesh to the jobs directly and making it Burst compilable as well.
  5. Less checks would be required for IsDisposed property, since it will be handled by GMesh instead of per data array’s.
8

Thanks for the input, I really appreciate it! Even though I’ll keep the design as is for now. :wink:

I’m happy with GMesh being a class providing a friendly interface to its internal, jobified data. After all, it will be used heavily in GUI tools, it is not meant to be a 100% Burst-only DOTS hardcore data cruncher.

The GMesh class wrapper allows me to write regular managed code as well as Jobs in the same class, which I often do because the process is to get it working without jobs first, and then port it to jobs and enjoy the tremendous speed increase, while not having to constantly make changes to all the tests, since they use the class interface.

But more importantly, it enables me to provide interfaces that I couldn’t otherwise, such as accepting input from IEnumerable or managed arrays which may eventually be useful since GMesh will be heavily used in GUI tooling. It may be less efficient than providing native data directly (such as NativeArray of vertex positions) but compatibility and ease of initial development (by just using managed array as input instead, for instance) allows me to move on to the next thing faster while enabling me to rather easily convert it to Jobs/Burst friendly code with tremendous speed gains at a later point. I’ve already gone through that cycle a couple times, for instance I made a plane with managed vertices and figured out how to make it work, then I moved it internally, converted into parallel jobs which gave me a 100x speed boost.

Btw, the cloning I really only added because I thought I might need it later, it’s not critical, but I wanted it in place so I can have a test that ensures cloning doesn’t fail (may help catch potentially related memory disposal issues, like this one).

I only added ICloneable because I thought “why not adhere to standards?”. But yes, the Clone(object xx) is bad, I’ll just remove ICloneable. Copy constructor suffices. And the two constructors are just this:

        public GMesh() => _data = new GraphData(Allocator.Persistent);

        public GMesh(GMesh other)
        {
            _pivot = other._pivot;
            _data = new GraphData(other._data);
        }

The fact that I’m passing Allocator.Persistent to GraphData is simply because it is a struct. Allocator will always be persistent.

Because of struct I had to have a constructor with some parameter, and I did not want to add some kind of Init() method which leaves the struct uninitialized between instantiating and (potentially not) calling Init(). This is what it looks like, and I can rely on ctor chaining for the copy ctor:

            public GraphData(AllocatorManager.AllocatorHandle allocator)
            {
                _elementCounts = new NativeArray<int>((int)Element.Count, allocator.ToAllocator);
                _vertices = new NativeList<Vertex>(allocator);
                _edges = new NativeList<Edge>(allocator);
                _loops = new NativeList<Loop>(allocator);
                _faces = new NativeList<Face>(allocator);
            }

            internal GraphData(GraphData other)
                : this(Allocator.Persistent)
            {
                _elementCounts.CopyFrom(other._elementCounts);
                _vertices.CopyFrom(other._vertices);
                _edges.CopyFrom(other._edges);
                _loops.CopyFrom(other._loops);
                _faces.CopyFrom(other._faces);
            }

Perhaps this could be misleading. I may change back to “bool bogusParameterBecauseOfStruct”. :sunglasses:

9

Forgot to mention, for Jobs I just pass in GraphData most of the time:

private struct CreatePlaneQuadsJob : IJob
{
    public GraphData Data;

    // ...
}

For some Jobs I just pass in one or more of the native GraphData collections via the NativeArray.ReadOnly properties that GraphData exposes, thus ensuring the list isn’t modified and Jobs know it’s read-only access (on top of the [ReadOnly] attribute I add).

Since GraphData only contains NativeXXX fields there is no copying of GraphData fields involved and neither do I have to copy collection data in and out of jobs.