Memory leaking b/c of handlers

[branflake2267]

It appears using this library is creating memory leaks b/c the handlers don't have a back reference.

I'd suggest integrating the widget framework into your elements so the handlers references can be destroyed.

https://developers.google.com/web-toolkit/articles/dom_events_memory_leaks_and_you

[laaglu]

On 10/03/2012 06:16 PM, Brandon Donnelson wrote:

It appears using this library is creating memory leaks b/c the handlers
don't have a back reference.

I'd suggest integrating the widget framework into your elements so the
handlers references can be destroyed.

https://developers.google.com/web-toolkit/articles/dom_events_memory_leaks_and_you

—
Reply to this email directly or view it on GitHub
#1.

Hi Brandon,

I am aware of this article. lib-gwt-svg used to follow the pattern
described in it, but I changed it about a year ago (see
https://groups.google.com/forum/?fromgroups=#!topic/lib-gwt-svg/Et0APoe9phw
for details).

My understanding is that leaks were created in early browser
implementations because they managed memory with a COM-like reference
counting scheme and could not deal with reference cycles. However
present implementations have a cycle collector and this is no longer an
issue.

Can you provide more details on your claim ? Is it based on just reading
the code and the article, or have you noticed a specific problem and
used memory analysis tools tracing the problem back to lib-gwt-svg and
the way it is implemented now ? If so have you tried to use
OMNode.cleanup() ?

Lukas

[branflake2267]

I haven't tried OMNode.cleanup() yet. Thanks for the info.

I've been profiling the svg rendering over and over I see the heap stack growing in the apps I've tried and SVG elements although getting cleared are lingering around in the heap profiling which is alluding to memory leaking. The other thing I see is the attaching the handlers directly to the dom and not to the gwt widget handler manager. I'm curious why you chose to use to register the handlers to the dom and not extend widget or use the widget mechanisms? If you look into that Widget class you'll notice mechanisms to deal with tear down of the element(s) and handlers, which I imagine you're already aware of.

I'm seeing signs of leaking all though I can't say its conclusively yet, although when looking at the profiling I'm seeing a long detached list grow. I guess my expectation is that the widgets that get cleared to get garbage collected. Although maybe its a matter of hows its written either higher or lower... I like to use GWT as it manages the attaching, detaching and such and makes this convient. But when working with an underlying jsni library, its nice to build this into the widget, so the widget manages the references.
http://www.vectomatic.org/gwt/svgreal-latest/svgreal.html?demo=013

[Michael-Allan]

Hi gents,

My own first guess is that a handler is being registered on something long-lived like the document, or the static OMNode.eventBus, and simply not unregistered.

The garbage collector fault mentioned in that article is probably a red herring, at least in this context. It seems doubtful that a browser capable of rendering SVG would have a garbage collector that broken.

Are other DOM elements freed from the heap on each collection cycle? Is it only the detached SVGs that linger around? Do they eventually cause a memory fault?

What happens if OMNode.eventBus is forcefully cleared after each SVG is thrown away?

[branflake2267]

From what I can tell so far the registering of the events are creating a references to the object groups and the OMNnode.cleanup() has to be explicitly called which I expect it to break the objects reference, but I'm not sure this cleanup is doing its job yet b/c I still have so many lingering SVG elements in the detached heap profile and event handlers. Then again there are many children in my svg, I have to consider all the possibilities of cleanup.

I did find some context alluding to object group cleanup in chrome documentation, although you have to break all the references to the wrapper before it will get freed. I don't see this happening with event registering at the moment.
https://developers.google.com/chrome-developer-tools/docs/memory-analysis-101

[branflake2267]

I'm pretty sure cleanup isn't going to work. I see all this hooking up to the dom stuff and no removing going on.

I'm not sure why a static eventbus is doing, but what happens to an handler gets added to that. I'm not sure this is going to get cleaned up?

And what about DOMHelper.bindEventListener((Element)ot.cast(), type.getName());? This is hooking up all kinds of events to the DOM. I compared this to RichTextAreaImpl(Standard...) and such, and its similar, except one thing, your missing all the unhooking.

The way I see it and correct me if I'm wrong, but the only way to clean up the memory leaks is unhook the element wrapper completely. That is remove the handlers and anything that may get atatched to the window for the element. I can't see this falling under the cyclic cleaner your talking about.

I'd copy the RichTextAreaImpl approach to hooking and unhooking the listeners wanted. Or CanvasElement. Thing is your not using the widget class, so it'd be tough to know when something gets detatched in the composite scope, so I'd use widget too.

I have to say I'm impressed with whats put together! Good job.

[branflake2267]

This is what I'm looking at:

http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/src/com/google/gwt/user/client/ui/impl/RichTextAreaImplStandard.java#308 - How RTA hooks up...

https://github.com/laaglu/lib-gwt-svg/blob/master/src/main/java/org/vectomatic/dom/svg/impl/DOMHelperImpl.java#L46 - How yours is hooking up. I see remove type methods, but they don't have it all and/or they don't percolate up to the OMNode...

[laaglu]

Hi Brandon,

First thanks a lot for spending time on this problem. I have read your
analysis but so far I am not convinced. Let me give you my point of view.

lib-gwt-svg design is fairly simple. One needs to distiguish between three
kinds of elements: Overlay types, Wrapper types and Widget types.

• Overlay types extend com.google.gwt.dom.client.Node and are the real DOM
  objects. They get attached to the DOM tree. They receive events.
• Wrapper types are the Java facade adding Java features (mostly interface
  support and support for event handler model).
• Widget types provide integration with other GWT widgets. lib-gwt-svg has
  very few of them (SVGImage, SVGPushButton, ...) because of the granularity
  of SVG. SVG elements cannot exist outside of an SVGSVGElement container, so
  it would not make sense to create an individual widget for every one of
  them.

How are these objects hooked ? There are 4 kinds of relationships to
consider

1/ Overlay types / Wrapper types ; Overlay types and Wrapper types always
work in pair. An overlay type has a back pointer to its wrapper type
(__wrapper) and a wrapper type has a pointer to its overlay type (ot). This
forms the reference cycle I was talking about and which I have been told
modern browser GCs can handle.

2/ Events ; The event bus is a kind of dictionary which maintains pairs of
(wrapper type, handler). When an event occurs the browser passes it to the
Overlay type (as its hooked to the DOM), which retrieves its associated
wrapper and then invokes the handlers through the event bus.

3/ Overlay types hierarchy. This is basically the DOM hierarchy. Note that
wrapper types do not maintain a parallel hierarchy mimicking the DOM
hierarchy: one wrapper type has no relationship with any other wrapper
type, even if the two associated Overlay types have a parent / child
relationship.

4/ Widget / Wrapper types. The SVGImage has a reference to the root
OMSVGSVGElement wrapper type.

For each of these relationships, you have a dedicated cleanup method:

• OMNode.cleanup removes the backpointer from Overlay type to Wrapper Type.
  I added that as a safety measure to break reference cycles in case the
  browser GC cannot do it itself but normally it is not supposed to be used.
• HandlerRegistration.removeHandler removes a (wrapper type, handler) pair
  from the event bus.
• ONNode.removeChild can be used to detach an overlay type from the DOM
  tree (it acts on the DOM tree as wrapper types do not form a tree as
  explained before).
• Widget.removeFromParent removes the reference from the Widget to the
  SVGImage or equivalent root class.

Of course memory leaks are possible. For instance, if you have a handler
for a wrapper type registered in the event bus, and you detach the wrapper
type from the widget / DOM tree, the wrapper type still has a pointer to
overlay type so the overlay type sticks around; in this case the correct
pattern is to call HandlerRegistration.removeHandler and this should get
GC'ed.

Lukas

On Fri, Oct 5, 2012 at 9:14 PM, Brandon Donnelson
notifications@github.comwrote:

This is what I'm looking at:

http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/src/com/google/gwt/user/client/ui/impl/RichTextAreaImplStandard.java#308- How RTA hooks up...

https://github.com/laaglu/lib-gwt-svg/blob/master/src/main/java/org/vectomatic/dom/svg/impl/DOMHelperImpl.java#L46- How yours is hooking up. I see remove type methods, but they don't have
it all and/or they don't percolate up to the OMNode...

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1#issuecomment-9186575.

[branflake2267]

I can see what your saying but I have to admit I like leaning on the widget to do the work. This is probably why I'm not digging with working with elements. :)

When I use cleanup in our main project I don't see it working of course this tells me I'm missing something. But using elements instead of widgets I suppose is my crux because I enjoy the fact the widget takes care of many of the needed duties to prevent leaks. I'm not sure it would impact the svg elements and that said it seems that your doing most of the same functions as the widget anyway. I have a feeling it might simplify some of your logic if you used the widget instead of the sinking methods for events. I'd really like to say .clear() and let the widget deal with all the children...

I've also begun testing to see if you auto GC reference is working. So far when drawing one svg element over and over I see the heap profile increase over time wether I use cleanup or not. I've got to make sure all my ducks are in a row and I want to do more testing but I'm not convinced leaving the GC up to the browser is the best option and for me, I'd prefer to programmatically clean up.

I think I'm going to test getting rid of the OMNNode and extend widget, this seems like an easier proposition to fix our project. :)

[branflake2267]

So far my preliminary results of removing the document as a wrapper and extending the OMNode as a widget got rid of my memory leaks and I think its far easier to add and remove regarding the gwt widget framework. :)