Automatic memory management v2

[johnspackman]

See issue https://github.com/qooxdoo/qooxdoo/issues/215

[coveralls]

Coverage increased (+0.04%) to 62.68% when pulling 6fa3d938784b591ffa4c878624750c632b92721d on johnspackman:automatic-memory-management-v2 into 641c95013682a329281b3ba5468768a802d2e0c5 on qooxdoo:master.

[oetiker]

@johnspackman I guess that some parts of the manual need to be changed accordingly too ?

[johnspackman]

Yes - how do we do that BTW?

Ill go through the code again this afternoon, looking for destructors/dispose which are required and update the API docs to match

[oetiker]

just patch the relevant parts of https://github.com/qooxdoo/qooxdoo/tree/master/documentation along with the code.

[level420]

@johnspackman beside taking care of calls to qx.core.ObjectRegistry.fromHashCode(): what else is needed to migrate existing code? Does it break compatibility somehow?

[johnspackman]

@level420 no its limited to fromHashCode, the entire trick is just about not storing references to objects, ie not calling qx.core.ObjectRegistry.register. The big impact internally in the Qooxdoo source is changing qx.ui.core.Widget and qx.html.Element so that they don't try and use weak references (ie don't require the .register call)

[coveralls]

Coverage increased (+0.02%) to 62.662% when pulling d1b8b9897ac7949c2e88f691fe636431f10e4c0e on johnspackman:automatic-memory-management-v2 into 641c95013682a329281b3ba5468768a802d2e0c5 on qooxdoo:master.

[coveralls]

Coverage increased (+0.04%) to 62.678% when pulling b1dc049804b64a9ea2dc4f25bff5e79843be2b80 on johnspackman:automatic-memory-management-v2 into 641c95013682a329281b3ba5468768a802d2e0c5 on qooxdoo:master.

[oetiker]

looked at the docs ... is it the case, that any class which has a 'dispose' method also must be disposed ?

[johnspackman]

not necessarily - any destructor or overridden dispose may do things which need to be called to clean up; but in a lot of cases all they do is set member variables to null, or call dispose on other objects.

So whether something has to be disposed becomes an API issue, rather than an qx.core.Object issue

The main things are event listeners for native events, and locale changes. This means that qx.ui.* is pretty much always having to be disposed.

BTW I have to go back through and check the overridden dispose, I realise I only checked destructors

[johnspackman]

Making it so that you have to "know" that this object requires dispose is not great though, nor is relying on API documentation especially when you might have to manually traverse the class hierarchy to check it.

How about if the class implements a marker interface that indicates that dispose is required; this would also mean that the object will be automatically stored in ObjectRegistry, which means that it is tracked and can be inspected, leaks can be checked for, and there is an explicit means of identifying classes at runtime and compile time which have this requirement.

Also, to mitigate the effects of breaking change for users of fromHashCode(), qx.core.Object.toHashCode() could issue a warning, once per class, if it is not automatically registered.

[johnspackman]

A principal of using garbage collection for managing objects instead of explicitly .dispose()ing them is that destructors and dispose() methods are not implemented. This is important for many reasons, the principal being that the Javascript garbage collector does not notify us when an object is about to be collected.

But this does not mean that objects can’t have startup and shutdown procedures - many do in fact, it’s just that cycle is (or should be) separate to garbage collection.

Historically, a common pattern in Qooxdoo classes is to use dispose/destructor as a general purpose shutdown, so if we want to move to automatic garbage collection we need to identify those classes which have a special shutdown procedure so that the calling code can respect that and free up resources. We could take the view that all destructor/dispose methods mean that the class has special shutdown procedures but this is unnecessarily harsh because there are a lot of classes which have trivial destructor implementations, or can be easily refactored to not have a destructor.

The difficulty here is in making it clear and obvious what needs to be disposed, and I’ve taken two approaches: first, each class which needs to be disposed implements the marker interface qx.core.IDisposable. Every Object which implements that interface will be registered with ObjectRegistry exactly as before, so that it can be identified for debugging purposes. This has the side effect that fromHashCode will also work for those objects, and that the classes can be identified reliably via reflection and (potentially) in the API viewer.

Secondly, many destructors simply set member variables to null and these have been removed. This is because they are unnecessary and by not providing one we make it clear that it is not necessary.

Thirdly, in some rare cases the code has been refactored to avoid the need for a destructor where previously there was one - for example, only adding a listener for the duration of the task and removing at the end of the task, instead of keeping the listener until dispose is called. Except in one or two places I have avoided this approach this time because the risk of introducing bugs with rash, unfocused changes is high.

In many cases, the destructors are for classes which are typically used as singletons and need not be tracked - for example the various qx.event.handler.* and qx.event.dispatch.* classes.

There is one remaining global list of objects which could benefit from refactoring - qx.data.SingleValueBinding keeps a global lookup of every bound object, so any listeners to or from an object property will prevent that object from being garbage collected. This could be modified so that the bindings are stored with the object, rather than in a global list indexed by hash code, but note that even when binding a single property, the target object also has links back to the source object; this means that unless both source and target are unbound, the binding will prevent garbage collection of both objects.

The net effect of this patch is that automatic garbage collection is now possible, with the proviso that (a) if you must release any bindings manually, and (b) you must observe any “shutdown” requirements of classes.

[coveralls]

Coverage decreased (-7.1%) to 55.528% when pulling b1ecffef0435bcfca430a21a24ddc60fa2f687d6 on johnspackman:automatic-memory-management-v2 into 641c95013682a329281b3ba5468768a802d2e0c5 on qooxdoo:master.

[johnspackman]

apologies, this PR is not ready - I pushed too early yesterday having forgotten to return to the unit tests first :(

[coveralls]

Coverage decreased (-7.2%) to 55.484% when pulling fe5357a5aa77400171305ef90f01e4de0b24ae0c on johnspackman:automatic-memory-management-v2 into 641c95013682a329281b3ba5468768a802d2e0c5 on qooxdoo:master.

[coveralls]

Coverage decreased (-7.1%) to 55.538% when pulling 6604697a659630fe897c87fb7f7325c16a89c3ba on johnspackman:automatic-memory-management-v2 into 641c95013682a329281b3ba5468768a802d2e0c5 on qooxdoo:master.

[coveralls]

Coverage decreased (-0.01%) to 55.699% when pulling 440f3ab468368c91a8a8ee3189d808c66c0ccdab on johnspackman:automatic-memory-management-v2 into 05d65f0ab0bf0eee93b68881306448235e8d265e on qooxdoo:master.

[level420]

@johnspackman do you use this already in production?

[johnspackman]

Yes; the new changes to do with what is or is not kept in the ObjectRegistry (ie IDisposable etc) is very new, but the base has been in use on live sites since January.

[cajus]

What is the state of the PR? I guess some more automatic memory management is a "must have" and would simplify a couple of tasks. As mentioned in the preceding PR, it would be cool to erase existing listeners targeting to objects to be disposed, too.

[level420]

I've did some short tests with the PR against current master with my app which has a real monster form. I opened/loaded with data/closed the form window about 20 times and had a look into the windows task manager and the reported memory usage there. This is what users do constantly in my app.

So far memory usage walked up and down but in sum stabilized at some value.

Additionally I opened nearly all forms and windows of the app and no problem showed up so far. The app worked as expected.

I've used current chrome on windows 7 for the tests.

[level420]

I added my vote whereby I can not say having really checked the changes. I'm trusting here in @johnspackman and travis and others. I'm still undecided when we should merge as it breaks compatibility regarding fromHashCode(), qx.core.Object.toHashCode()

[zaucker]

What is holding up a merge?

[level420]

@zaucker : your vote ;-) and the votes of other core members

[zaucker]

:ballot_box_with_check: CALL FOR VOTES

[level420]

I think the main risk of having this PR merged, is that we loose the object registry functionality. I've tested the PR with my largest app and had no problems so far. Therefore I gave the PR my vote.

[zaucker]

No risk, no fun ...

[oetiker]

OK waiting to give my vote ... I have not tested it at all only read

[johnspackman]

@level420 it's true that the ObjectRegistry.fromHashCode functionality is lost by default for non-widgets, it's easily switched on and imho there is an argument for localised, purpose-specific registries. Poor consolation for anyone who has a bug caused by this, but there is the warning logged on the console when fromHashCode() can't find an object.

[zaucker]

Just tried this branch with a larger application and found no problem so far.

[woprandi]

I'll try to test this PR. Should I check something special ?

[level420]

@woprandi You should simply test your app and as much of its functionality as possible and have a look into memory consumption if it grows over time (wait a bit to allow the automatic garbage collector to work).

[woprandi]

After few minutes, I don't see any strange behavior or memory behavior. I'll work with these branch all that day

[cajus]

Will test it here with a bigger block of code. Please hold the line ;-)

[gnikolaidis]

I am impatient to use this PR in production. Whatever the risks of breaking compatibility with existing code, the benefits of NOT registering all qx.core.Object's are too important to ignore.

[hkollmann]

I tested this patch with our vzaweb application without any problems

[cajus]

I can't build the project with the PR activated:

>>> Collapsing parts  
>>> Verifying parts \<type 'exceptions.RuntimeError'> :
Unfullfilled dependency of class 'qx.event.Manager'[31,44]: 'qx.core.IDisposable'

[cajus]

Hmm. Looks like it is not in the patch. Verifying...

[cajus]

Ok. The bigger application loads and I don't see any issues going thru the most common usecases. Testing it using selenium would last some hours, so I'm skipping this for now.

@johnspackman: One build issue with the currently external upload manager:

Uncaught Error: Overwriting private member "__widget" of Class "com.zenesis.qx.upload.InputElement" is not allowed!

That one has been added in the qx.html.Element.

[cajus]

At least two business days have passed since the call for votes, and two assenting votes have been cast with no dissenting votes. Going to merge this one.