java-gnome-hackers

[java-gnome-hackers] WeakReferences going away

[Tom B. <Tom...@Su...>]

It appears that org.gnu.gtk is misusing the WeakReference class to store
its event listeners.  On my system, widgets that are initially
responsive to events quickly stop being responsive, even with simple
Java-GNOME example apps.  The problem is that the app's event listeners
are being GC'd even though the widgets they are listening to are still
live.

A WeakReference is supposed to be used for cases where it's okay for a
reference to go away, usually because it can be recreated.  The classic
example is with a cache; if a lookup is expensive, you cache the result
in a hashtable using a WeakReference (or just use WeakHashMap) -- if the
cache gets emptied by the garbage collector, it just means more
lookups.  The cache may have an entry disappear, but the caller never
knows this because the cache is a hidden implementation detail.

Event listeners need to remain permanently attached to their respective
widgets, however; they are not transitory in any way.  When I remove all
of the WeakReference use in GTK, the problem goes away as it should.  It
also simplies the event handling code quite a bit, and most likely
speeds it up as well.  I suspect there's a lot more potential for shared
code now, too.

Unless anyone objects soon, I want to check in this "WeakReference-free"
delta.

Tom

Re: [java-gnome-hackers] WeakReferences going away

[Philip A. C. <pch...@pc...>]

On Thu, 2003-02-27 at 10:38, Tom Ball wrote:
> It appears that org.gnu.gtk is misusing the WeakReference class to store
> its event listeners.  On my system, widgets that are initially
> responsive to events quickly stop being responsive, even with simple
> Java-GNOME example apps.  The problem is that the app's event listeners
> are being GC'd even though the widgets they are listening to are still
> live.
>=20
> Unless anyone objects soon, I want to check in this "WeakReference-free"
> delta.

Tom,

The original thought was to protect from memory leaks.

The problem is that sometimes developers do not call the remove method
to remove their listener before releasing all other references in their
code to the listener object.  This leaves the reference in the listener
list as the only reference to the listener and the listener is never
garbage collected.

Granted, this is a developer issue in that they *should* always call the
remove method first.  The hope was to give some protection against such
sloppy programming from causing big problems.

If it's causing too much trouble, can it be reworked?  If not, I'd say
remove it.  Maybe the idea was too altruistic.

BTW, Off topic:  I've been away for quite a while because of insane work
schedules.  I'm hoping to get involved again in the next few weeks.=20
What is still outstanding to be done?

--=20
Philip A. Chapman

Application Development:
Java, Visual Basic (MCP), VB for Applications, PostgreSQL, MySQL, MSSQL
Linux, Windows 9x, Windows NT, Windows 2000, Windows XP

[java-gnome-hackers] Welcome back, Philip!

[Tom B. <Tom...@Su...>]

On Thu, 2003-02-27 at 09:20, Philip A. Chapman wrote:
> BTW, Off topic:  I've been away for quite a while because of insane work
> schedules.  I'm hoping to get involved again in the next few weeks. 
> What is still outstanding to be done?

Work insanity seems rampant these days.  My own project was almost
cancelled at SunLabs, and now I need to focus on it more in spite of my
management's former support of my Java-GNOME help.  

In general, the biggest area that needs work are the examples and
demos.  That may seem like junior-level work, but they uncover lots of
problems in the toolkit that a senior-level engineer needs to
investigate.  Since these are also our best documentation, they deserve
extra scrutiny to make sure they are using the toolkit in the manner we
want them to.

If you are looking for a hard problem, Mark and I are seeing lots of
assertion failures.  The ones on my system are caused when
org_gnu_glib_GObject.c tries to copy a widget parameter (Containers fire
signals with child parameters) in jg_signal_cb in the "default" case
block.  If you can shed any light here, that would be great.

Tom

Re: [java-gnome-hackers] WeakReferences going away

[Tom B. <Tom...@Su...>]

On Thu, 2003-02-27 at 09:20, Philip A. Chapman wrote:
> The original thought was to protect from memory leaks.

Yes, dangling listeners can be a major source of object leaks.

> The problem is that sometimes developers do not call the remove method
> to remove their listener before releasing all other references in their
> code to the listener object.  This leaves the reference in the listener
> list as the only reference to the listener and the listener is never
> garbage collected.
> 
> Granted, this is a developer issue in that they *should* always call the
> remove method first.  The hope was to give some protection against such
> sloppy programming from causing big problems.

Paranoid, perhaps, not altruistic. :-)  One way to solve this is to
provide good example code -- developers tend to slavishly copy examples,
and as we learned the hard way with Swing, bad examples foster
widespread bad toolkit use.

In this case, I think the problem is related to my style of app
development:  I never keep listener references in my app, but instead
just attach them to a widget and forget about them:

	Button cancelButton = fontsel.getCancelButton();
	cancelButton.addListener(new ButtonListener() {
		public void buttonEvent(ButtonEvent event) {
		    if (event.isOfType(ButtonEvent.Type.CLICK)) {
			fontsel.destroy();
		    }
		}
	    });

In this example, the only reference to the ButtonListener is in
cancelButton's listener list, which is why it gets GC'd if that list is
implemented with WeakReferences.  This style of listener usage is very
common among AWT and Swing developers; that doesn't make it necessarily
correct usage, but I can testify that it is bewildering for such a
developer to have one's listeners "disappear" after installing them this
way.

Now, if an application class either implements the listener interface
directly or stores the reference, then you have the potential leakage
you describe since the app instance won't be GC'd.  But there I think
good documentation is the best one can do here.

> If it's causing too much trouble, can it be reworked?  If not, I'd say
> remove it.  Maybe the idea was too altruistic.

If memory isn't too big an issue (or if we want to create a "debug"
version of our libraries), we can keep a WeakReference'd static list in
each listener class of the current listeners and what widget they are
listening to.  Some key sequence (such as Ctl-Shift-F10) can trigger the
dumping of this list out to the command line for debugging purposes. 
This beats poking around in a heap profiler, and is much more likely to
be used by app developers.  If you want, I can add this feature soon.

Tom

Re: [java-gnome-hackers] WeakReferences going away

[Philip A. C. <pch...@pc...>]

On Thu, 2003-02-27 at 12:10, Tom Ball wrote:
> In this case, I think the problem is related to my style of app
> development:  I never keep listener references in my app, but instead
> just attach them to a widget and forget about them:
>=20
> 	Button cancelButton =3D fontsel.getCancelButton();
> 	cancelButton.addListener(new ButtonListener() {
> 		public void buttonEvent(ButtonEvent event) {
> 		    if (event.isOfType(ButtonEvent.Type.CLICK)) {
> 			fontsel.destroy();
> 		    }
> 		}
> 	    });
>=20

Yes, this does seem to be an issue of coding style.  Many, many code
examples of Swing use inner classes as event handlers.  Not to start any
flame wars, but I *personally* dislike inner classes and very rarely (if
ever) use them.  However, my personal preference does not matter.  Java
allows developers to do this and many swing developers (and some IDEs w/
GUI designers) use this heavily.  Therefore, we should code to support
*both* methods.  (Disclaimer:  I realize that the use of inner classes
are not the only possible cause of this problem, but would probably be
the most likely.)

> But there I think good documentation is the best one can do here.

Then good documentation it shall be.  Even if this weren't a problem,
good documenatation is a must.  :-)

> If memory isn't too big an issue (or if we want to create a "debug"
> version of our libraries), we can keep a WeakReference'd static list in
> each listener class of the current listeners and what widget they are
> listening to.  Some key sequence (such as Ctl-Shift-F10) can trigger the
> dumping of this list out to the command line for debugging purposes.=20
> This beats poking around in a heap profiler, and is much more likely to
> be used by app developers.  If you want, I can add this feature soon.

I think that this would be a *very* nice feature in a "debug" version.

--=20
Philip A. Chapman

Application Development:
Java, Visual Basic (MCP), VB for Applications, PostgreSQL, MySQL, MSSQL
Linux, Windows 9x, Windows NT, Windows 2000, Windows XP

Re: [java-gnome-hackers] Welcome back, Philip!

[Tom B. <Tom...@Su...>]

On Thu, 2003-02-27 at 10:07, Tom Ball wrote:
> If you are looking for a hard problem, Mark and I are seeing lots of
> assertion failures.  The ones on my system are caused when
> org_gnu_glib_GObject.c tries to copy a widget parameter (Containers fire
> signals with child parameters) in jg_signal_cb in the "default" case
> block.  If you can shed any light here, that would be great.

Fixed:

diff -r1.7 org_gnu_glib_GObject.c
198c198
<       *(void **) &peer = g_value_get_pointer(&param_values[i]);
---
>       *(void **) &peer = g_value_get_object(&param_values[i]);

I guess GLib-2.0 has stronger type checking...

Tom