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

Fixed leaks that affected shutting down and recreating GraphicsDevice under DirectX #5728

Merged
merged 3 commits into from Jun 2, 2017

Conversation

KonajuGames
Copy link
Contributor

Running tests has dropped from a continuous climb to 5GB memory to a more consistent use that peaks at 195MB.

  • The swapchain was not being disposed in GraphicsDevice.DirectX for Windows.
  • Added PlatformDispose() to BlendState.
  • SpriteBatchTest was not calling base.TearDown().
  • GraphicsDeviceTestFixtureBase calls game.DoInitialize() rather than CreateDevice() directly on the IGraphicsDeviceManager. This allows the call to game.Dispose() to properly clean up the GraphicsDeviceManager.
  • VertexBuffer ctor was assigning the GraphicsDevice to the VertexDeclaration, but this was not used anywhere. Most common VertexDeclarations are a static global resource, thus it was holding a reference to the first GraphicsDevice.

for (int i = 0; i < _targetBlendState.Length; ++i)
_targetBlendState[i] = null;
#if DIRECTX
PlatformDispose();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can declare this as a partial function... so that if it doesn't exist then it is ok? Better than an #if I think.

@@ -26,10 +26,6 @@ protected VertexBuffer(GraphicsDevice graphicsDevice, VertexDeclaration vertexDe
this.VertexCount = vertexCount;
this.BufferUsage = bufferUsage;

// Make sure the graphics device is assigned in the vertex declaration.
if (vertexDeclaration.GraphicsDevice != graphicsDevice)
vertexDeclaration.GraphicsDevice = graphicsDevice;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could live with this maybe... although I expect XNA does set it.

I wonder how many people have as a hack assumed they could fetch the graphics device via VertexDeclaration.GraphicsDevice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XNA even uses these as a static...

https://msdn.microsoft.com/en-us/library/microsoft.xna.framework.graphics.vertexpositioncolor.vertexdeclaration.aspx

So not sure hope they ensure it is cleaned up and reused when a GraphicsDevice is disposed and recreated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some tests with XNA. The VertexDeclaration.GraphicsDevice property remains null until a VertexBuffer is created using the vertex declaration. After the GraphicsDevice is disposed, the vertex declaration retains the reference to the disposed GraphicsDevice object. The disposed GraphicsDevice object is replaced with the new GraphicsDevice object when subsequently used again with a new VertexBuffer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KonajuGames - So that sounds exactly how we had things working here right?

Really the worst thing this could do would be keeping the old disposed GraphicsDevice class in memory. Since it was disposed it should have already released all native resources so this reference shouldn't keep anything big in memory.

I think these two lines can be restored without any leaks appearing as long as we're disposing everything else correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those lines were restored in the last commit. Now that GraphicsDevice properly disposes of everything, it is not an issue.

Steve Williams added 2 commits May 31, 2017 23:06
- The swapchain was not being disposed in GraphicsDevice.DirectX for Windows.
- Added PlatformDispose() to BlendState.
- SpriteBatchTest was not calling base.TearDown().
- GraphicsDeviceTestFixtureBase calls game.DoInitialize() rather than CreateDevice() directly on the IGraphicsDeviceManager. This allows the call to game.Dispose() to properly clean up the GraphicsDeviceManager.
- VertexBuffer ctor was assigning the GraphicsDevice to the VertexDeclaration, but this was not used anywhere. Most common VertexDeclarations are a static global resource, thus it was holding a reference to the first GraphicsDevice.
- Add setter for Game.graphicsDeviceManager property so it can get set when the GraphicsDeviceManager is created.
- Revert change to GraphicsDeviceTestFixtureBase so the XNA tests compile again.
- Add new assets to XNA test project.
@nkast
Copy link
Contributor

nkast commented Jun 1, 2017

Lately I had problem to run the XNA tests, probably because it run out of memory.
I tested this PR with the XNA tests but unfortunately nothing has changed.
Memory keeps growing and after a while the tests starts failing, at the end the app crash.

xnatests

Just to make it clear, This PR didn't make XNA tests worst, the bug/leak is already in Develop, but since it fixes a couple of leaks in SpriteBatchTest & GraphicsDeviceTestFixtureBase it's the reason I tests it against the XNA tests project.

@KonajuGames
Copy link
Contributor Author

The major fix here is in our GraphicsDevice.DirectX where it now disposes the swapchain. This will have no effect on the XNA tests.

I have had the XNA tests fails because it has run out of resources as well. Haven't tracked that one down yet.

@@ -861,7 +927,9 @@
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
</ItemGroup>
<ItemGroup />
<ItemGroup>
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
Copy link
Member

@tomspilman tomspilman Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you happen to know what this is? @KonajuGames ?

<None Include="Assets\Fonts\QuartzMS.xnb">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Include="Assets\Fonts\SegoeKeycaps.spritefont">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
Copy link
Member

@tomspilman tomspilman Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KonajuGames

I double checked.... this file isn't used at runtime in the tests which is why it wasn't included in the project before. Do we want to start including everything now? This will mean the XNA project and the MonoGame Test project won't match.

While some of these are omissions in the XNA project... I suspect some are like this and not used at runtime.

@tomspilman tomspilman changed the title Windows DirectX leak fix Fixed Windows DirectX leaks that affected shutting down and recreating GraphicsDevice Jun 2, 2017
@tomspilman tomspilman changed the title Fixed Windows DirectX leaks that affected shutting down and recreating GraphicsDevice Fixed leaks that affected shutting down and recreating GraphicsDevice under DirectX Jun 2, 2017
@tomspilman tomspilman added this to the 3.7 Release milestone Jun 2, 2017
@tomspilman
Copy link
Member

Thanks @KonajuGames ... merging!

@tomspilman tomspilman merged commit 6c2e48f into MonoGame:develop Jun 2, 2017
}
base.Dispose(disposing);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KonajuGames - Could you apply this same PlatformDispose technique to all the other render states? Just a little weird that the others just override Dispose(bool) now. Ran into this on console.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

afrodynamics pushed a commit to PocketwatchGames/MonoGame that referenced this pull request Aug 12, 2017
… under DirectX (MonoGame#5728)

* Windows DirectX leak fix
- The swapchain was not being disposed in GraphicsDevice.DirectX for Windows.
- Added PlatformDispose() to BlendState.
- SpriteBatchTest was not calling base.TearDown().
- GraphicsDeviceTestFixtureBase calls game.DoInitialize() rather than CreateDevice() directly on the IGraphicsDeviceManager. This allows the call to game.Dispose() to properly clean up the GraphicsDeviceManager.
- VertexBuffer ctor was assigning the GraphicsDevice to the VertexDeclaration, but this was not used anywhere. Most common VertexDeclarations are a static global resource, thus it was holding a reference to the first GraphicsDevice.

* - Change BlendState.PlatformDispose() to a partial method.
- Add setter for Game.graphicsDeviceManager property so it can get set when the GraphicsDeviceManager is created.
- Revert change to GraphicsDeviceTestFixtureBase so the XNA tests compile again.
- Add new assets to XNA test project.

* Revert changes to MonoGame.Tests.XNA.csproj.
nkast pushed a commit to nkast/MonoGame that referenced this pull request Jun 26, 2018
… under DirectX (MonoGame#5728)

* Windows DirectX leak fix
- The swapchain was not being disposed in GraphicsDevice.DirectX for Windows.
- Added PlatformDispose() to BlendState.
- SpriteBatchTest was not calling base.TearDown().
- GraphicsDeviceTestFixtureBase calls game.DoInitialize() rather than CreateDevice() directly on the IGraphicsDeviceManager. This allows the call to game.Dispose() to properly clean up the GraphicsDeviceManager.
- VertexBuffer ctor was assigning the GraphicsDevice to the VertexDeclaration, but this was not used anywhere. Most common VertexDeclarations are a static global resource, thus it was holding a reference to the first GraphicsDevice.

* - Change BlendState.PlatformDispose() to a partial method.
- Add setter for Game.graphicsDeviceManager property so it can get set when the GraphicsDeviceManager is created.
- Revert change to GraphicsDeviceTestFixtureBase so the XNA tests compile again.
- Add new assets to XNA test project.

* Revert changes to MonoGame.Tests.XNA.csproj.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants