luca-piccioni / OpenGL.Net

Modern OpenGL bindings for C#.
MIT License
568 stars 108 forks source link

Question about Finalizer in OpenGL.Net.Objects #68

Closed antoinecla closed 6 years ago

antoinecla commented 6 years ago

The Dispose(bool) method of GraphicsResource delete the resource only when disposing == true. This means that when the the GraphicsResource is finalized ( disposing == false) the resource is not deleted.

Example:

Shader shaderObject = new Shader(ShaderType.VertexShader);
shaderObject.Create(_Context);
shaderObject.Dispose();

On dispose, the shader GL object is deleted (Gl.DeleteShader(name);) as expected.

If instead the GC calls the finalizer, Shader.DeleteName() is not called and the shader GL object is not deleted.

Is this the expected behavior or am I missing something? Can you explain your approach?

luca-piccioni commented 6 years ago

No, you haven't missed anything: it's just like you described. IDisposableinstances must be disposed, otherwise un-managed resources (GL objects) will be leaked.

The Resourceclass has the responsibility to track the lifetime of each instance (tracking is performed only in Debug builds), link resources to automate dispositions, and more importantly tracks the reference count. In this way a Resource can be shared along multiple instances, and is actually disposed when no other resource is referencing it (actually Dispose() can be called by Unref).

GraphicsResourcederives IDisposableimplementation from Resource, adding Dispose(GraphicsContext) for disposing GL object using a specific context.

The Dispose()method check whether the context has created the GL object is current: if it is, delete the GL object immediately, otherwise it defers the deletion till the execution of GraphicsContext.DisposeResources().

antoinecla commented 6 years ago

That makes sense.

It is a bit difficult to fully follow the Resource, GraphicsResource, GraphicContextdispose workflow but it seems that the Finalizer is not needed in your approach (and has an impact on the GC performance).

The Finalizer pattern is explicitly used to release native resources when a developer fails to call Dispose. So, it would still make sense to delete the native resource in the Finalizer.

I don't have a full understanding of your code yet but I feel that there are two options:

I think that option 2 is preferable but requires a full analysis of the code and serious testing!

luca-piccioni commented 6 years ago

I strongly disagree. The main reason is that finalizers does not ensure that resources are freed by a thread on which the right GL context is current: this will lead to real leaks and exceptions. The finalizers are written to avoid the Delete execution (managed parameter is false).

In some GL configuration, it would be possible to make the GL context current on finalizer thread, but since a GL context can be current on one thread at time, it will conflict with any rendering loop running. Additionally, making a context current on a thread is very expensive, and should be performed only once.