sharpdx / SharpDX

SharpDX GitHub Repository
http://sharpdx.org
MIT License
1.69k stars 641 forks source link

Disposing Device seems to leak some memory #982

Open mellinoe opened 6 years ago

mellinoe commented 6 years ago

Consider code like this:

for (int i = 0; i < 1_000_000; i++)
{
    Device d = new Device(SharpDX.Direct3D.DriverType.Hardware);
    d.Dispose();
}

This seems to cause a very small leak each iteration. After about 5,000 iterations memory usage has jumped about 10 MB, which admittedly is not very much. This isn't a major problem, but I'd like to understand if I'm doing something wrong or if my measurements/debugging approach are wrong in some way.

This repros on the latest "CI" nuget package, but I haven't tried against the latest code in this repo yet (assuming there are changes after that CI build).

mellinoe commented 6 years ago

I wonder if this is this fix: https://github.com/sharpdx/SharpDX/pull/945

I had seen that change and assumed it was in the 4.1.0-ci184 build. But it turns out that build was actually from the day before the change, so it's not in there.

@amerkoleci Do you think that change would fix this?

mellinoe commented 6 years ago

I built master and there still seems to be a leak.

ziriax commented 6 years ago

Can you try inserting a GC.Collect and GC.WaitForPendingFinalizers after the loop? Maybe the leak you are seeing it just the garbage collector not yet kicking in.

I will look into this tomorrow, I am also debugging and fixing memory leaks.

mellinoe commented 6 years ago

@Ziriax I actually tried that, and forced a GC every 1000 iterations of the loop. It didn't seem to make a difference to me. I would really suspect that it isn't GC'd memory anyways -- when I take managed heap snapshots in Visual Studio, there is never a difference in size, even when the process's usage is climbing (according to Task Manager). Visual Studio's native heap snapshot tool shows the usage climbing.

xoofx commented 6 years ago

The same code in C++ leaks a bit of memory, so not sure this is an issue with SharpDX itself:

#include "stdafx.h"
#include "d3d11.h"

int main()
{
    ID3D11Device* pDevice;
    ID3D11DeviceContext* pDeviceContext;
    D3D_FEATURE_LEVEL level;

    while (true)
    {
        auto result = D3D11CreateDevice(NULL, D3D_DRIVER_TYPE_HARDWARE, NULL, 0, NULL, 0, D3D11_SDK_VERSION, &pDevice, &level, &pDeviceContext);
        if (FAILED(result))
        {
            return 1;
        }
        pDeviceContext->Release();
        pDevice->Release();
    }

    return 0;
}
mellinoe commented 6 years ago

@xoofx In that case, it looks like there's no problem with SharpDX here. Out of curiosity, what kind of GPU do you have? Perhaps it's vendor-specific or something. I've only tested this on my Nvidia GTX 770.

xoofx commented 6 years ago

I have a Nvidia GTX 1060... Looking at the load/unload events on dll, I can see that on Direct3D11 device creation, it loads dynamically 2 NVidia DLLs drivers nvldumd.dll and nvwgf2um.dll and they are unloaded when you dispose the device, so yeah, that can be exacerbated by a gdx driver that could leak some memory here. In fact just modifying the example above by keeping the first device alive, but disposing the next devices, and the memory is a lot more stable. So it confirms that it is the gfx drivers that may leak. Though, in practice, this is a pathological case that we barely see in applications, as we never create multiple device in a loop like that. In which case do you need this? (looping on create/dispose on a device without at least keeping one device around?)

mellinoe commented 6 years ago

keeping the first device alive, but disposing the next devices, and the memory is a lot more stable.

That's a good find. That matches the scenario I was originally concerned about, so hopefully it indicates there won't be any leaks at all. I agree that this is an obscure, pathological case, in general.

In which case do you need this?

A user of Veldrid needed to draw into multiple frames in a window, and I don't have support for multiple swapchains at the moment. So the current code involves creating one device per view, and then disposing the device when the frame is closed. It's good to know that this issue probably won't actually result in a visible leak (unless every device is disposed at once), but the real solution is to just let you create a new swapchain for the extra application frame.

ziriax commented 6 years ago

I think DirectX doesn't free resources immediately when the refcount becomes zero, I recall having to call Flush on the context to really delete the resources. Of course this doesn't apply here, but maybe is related somehow.

xoofx commented 6 years ago

I think DirectX doesn't free resources immediately when the refcount becomes zero, I recall having to call Flush on the context to really delete the resources. Of course this doesn't apply here, but maybe is related somehow.

Yes. It is part of the documentation and was part of DeviceContext dispose until recently (#929). Thought I always assumed that it would have required a minimal set of operation to occur on the device context to make this relevant.... but checking this with the C++ code above by adding ClearState and Flush:

            pDeviceContext->ClearState();
            pDeviceContext->Flush();

And the memory is stable. Doing the same with the Device.ImmediateContext before disposing the device should give the similar behavior (except the GC behind solicited more for the Device/ImmediateContext object to release from the nursery/gem0)

amerkoleci commented 6 years ago

So better revert the fix I've submitted about #929

chrs84 commented 6 years ago

Any news on this? I am getting memory leaks because of this when freeing different AppDomains.