CompositionTarget.Rendering memory leak

ddklo wrote at 2012-01-12 18:56:

Hi.

I notice that this event is subscribed to in multiple places in the Helix 3D Toolkit creating a memory leak if it is not unsubscribed. In some of the places there is a method or property to unsubscribed from this event which is nice, but it can be hard to know when these objects should be disposed. In our app we leak HelixViewport3D's because there is no way to unsubscribed from this event which is attached in the constructor for the fps. Perhaps weak event subscription as discussed in this blog post: http://blog.catenalogic.com/post/2011/11/23/A-weak-event-listener-for-WPF-Silverlight-and-Windows-Phone-7.aspx could be used to allow for garbage collection. Or an public Unsubscribe/Dispose method on the HelixViewport3D.

Regards

Dagfinn

objo wrote at 2012-01-12 21:09:

thanks for the link, that's an interesting implementation!

Another approach: Can overriding OnVisualParentChanged be used to unsubcribe the Rendering events? Like I did in the ScreenSpaceVisual3D class, it seems to work there. Subscribe if the parent is not null, and unsubscribe if the parent is null.

ddklo wrote at 2012-01-13 00:03:

I think the OnVisualParentChanged method only works when the parent becomes "null" which might not always be the case unless it is explicit set to be null be implementing some dispose logic. We have some scenarios where we generate at lot of screenshots of the HelixViewport3D (hv) off screen where the hv is assigned a temporary RootVisual and in another circumstances the hv is contained within a another control so even though the container control parent might become "null" I don't think the hv parent will be null.

Since knowing exactly when to unsubscribe from the CompositionTarget.Rendering can be hard I think the weak event solution looks good. Also it might make sense to only subscribe to the event if needed (ShowTriangleCountInfo or ShowFrameRate is enabled) since it is fired a lot.

ddklo wrote at 2012-01-15 00:10:

Hi.

I profiled the source code with the ANTS MemoryProfiler by having a window open another window containing the HelixVewport3D and other visuals. By subscribing to the CompositionTarget.Rendering event a strong reference is created from the object subscribing to System.Windows.Media.MediaContext which prevents the object from being garbage collected.  Even worse if the scene contains one of the visuals subscribing to this event the entire scene and viewport is kept in memory. Closing the window did not trigger the OnVisualParentChanged event so it was never removed for the visuals where this method is overrided.

By copying over the WeakEventListner for the blog post and replacing all the events subscriptions to CompositionTarget.Rendering with

var weakEvent = WeakEventListener.SubscribeToWeakEvent(this, null, "Rendering", CompositionTargetRendering);

 all the memory leaks went away. And it did not seem to have any impact on the performance for the samples.

Currently this is quite a big issue for us, especially the event on the HelixViewport3D where there is no way to unsubscribe.

Do you think it makes sense changing the event subscription? If so can you provide a timeframe? We can work around it by compiling the source code ourself, but it would be nice to still get updates through nuget.

Regards Dagfinn

objo wrote at 2012-01-15 12:35:

hi Dagfinn, I am looking into this now! Thanks for good change suggestions!

I prefer to explicitly unsubscribe the events, but if there is not a reliable way to do this, I'll go for the weak event listener.

Can subscribing/unsubscribing to the Rendering event be done in the Loaded/Unloaded events? This seems to work in my test cases.

Using WeakEventListener I see the HelixViewport3Ds are freed, but it is leaking instances of WeakEventListener. Do you know what I am doing wrong? I used:

WeakEventListener<HelixViewport3D,CompositionTarget,EventArgs>.SubscribeToWeakEvent(this, null, "Rendering", this.CompositionTargetRendering); 

ddklo wrote at 2012-01-15 13:07:

I agree that explictly unsubscribing usually is the prefered approch. But I think its quiet common to use weak events when designing WPF controls (see the Weak Event Pattern - http://msdn.microsoft.com/en-us/library/aa970850.aspx). I'm not sure why the framework designeres didn't make the CompositionTarget.Rendering a weak event (probably some performance reason I think thats why the signature is using EventArgs instead of RenderingEventArgs).

I think what you are seeing is the expected behaviour it can take a little while before the  WeakEventListener is GC. Try waiting a while and then calling:

GC.Collect();
GC.WaitForPendingFinalizers();

The unloaded worked for my simple example for the HelixViewport3D, but then you have to be careful and reconnected when its loaded again. But since the Unloaded event is only for framework elements it can't be used for the Visual3D's. Not sure what would be the best approch overall, but I think since managing lifetime can be hard the weak event options is the safest. Otherwise it can be hard to know when the HelixToolkit could cause memory leaks without looking at the source code.

objo wrote at 2012-01-16 05:47:

Thanks! I was using dotTrace Memory, and it didn't do the garbage collection before taking the snapshot.

I have the Catel solution working, but I would like to see if it can be done with a standard WeakEventManager. I posted a question on stack overflow why a static event does not seem to work. Do you know?

http://stackoverflow.com/questions/8876112/using-weakeventmanager-with-a-static-event

ddklo wrote at 2012-01-16 11:08:

No, I'm not sure . Remember trying it myself without luck. It will be interesting to see if anyone knows.

There is an interesting article discribing different approches on codeproject:

http://www.codeproject.com/KB/cs/WeakEvents.aspx

I think the Catel solution is an improvment on Solution 4: Reusable Wrapper.

objo wrote at 2012-01-18 12:14:

Found the error (should set sender=null when calling DeliverEvent), will submit the solution using WeakEventManager soon.

Grunwald's article is great. I will also keep the Catel solution in mind!

objo wrote at 2012-01-18 21:02:

See change set 74235. I hope I got it right now!

ddklo wrote at 2012-01-19 10:21:

Hi. 

I profiled some more and can't see any memory leaks now. Thank you for your efforts. I also think its worth removing the destructors/finalizers since they are not needed or only include them in the debug build because they are expensive for the gc and causes the objects to live longer.

http://msdn.microsoft.com/en-us/library/ms973837.aspx

objo wrote at 2012-01-19 11:07:

good, I'll remove the finalizers (in the latest changeset they are wrapped in #if DEBUGs) and Debug.WriteLine completely if the problem is solved! Using the HelixViewport3D in TabControls seems to hold the instances a little longer than other containers, but from what I can see, these HelixViewport3D instances are also freed later...