Qooxdoo Automatic Memory Management - a proposal

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Qooxdoo Automatic Memory Management - a proposal

John Spackman-3

Hi all


Apologies for a rather long post, please bear with me :)


I’ve been thinking a lot lately about memory management in Qooxdoo apps, specifically how to reliably remove the need to call dispose() (without causing leaks) and reducing complexity to our applications — I have a proposal for how to achieve this but it requires some subtle changes to the Qooxdoo framework.  In theory this would be largely backward compatible (with a few specific exceptions) and would make it significantly easier to write long-running or memory sensitive applications.


Although the mantra is that “if you created it, you should dispose it”, this is increasingly hard to follow as an application becomes more complex.  There are specific examples of why this is virtually impossible baked into Qooxdoo, for example, some classes override setXxxx methods to *copy* the passed in object rather than take ownership of it - which means that the calling code has to understand the private implementation of the object in order to correctly handle cleanup.  qx.ui.form.SelectBox.setSelected is an example of this - the caller has to “know” that setSelected will copy the array it is given and not take it over and be responsible for disposing it.


Another common example with qx.data.Array is that methods such as splice and slice return an array of the removed items, but the returned object is an instance of qx.data.Array so you *must* dispose it.  To illustrate my point, take a look at these samples of code and try to determine which is the “correct” one:


Sample 1A:

var selection = // … snip: get an array (.. or is it a qx.data.Array ..?)

selection.slice(0,1); // delete the first element


Sample 1B:

var selection = // … snip: get an array (.. or is it a qx.data.Array ..?)

selection.slice(0,1).dispose(); // delete the first element


In order to figure out which piece of code is correct, you must understand the “…snip: get an array” and know if it is a native array or a qx.data.Array


How about these examples:


Sample 2A:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

widget.setSelection(selection);


Sample 2B:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

widget.setSelection(selection);

selection.dispose();


Sample 2C:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

var oldSelection = widget.getSelection();

widget.setSelection(selection);

if (oldSelection)

oldSelection.dispose();


In order to choose from those samples, you must understand what the widget object is, where it came from, _and_ how it implements setSelection to know whether and what to dispose.  This is not good - not only is the answer obscure, but if the implementation of widget.setSelection changes, your code may break.  Sample 2C is also much longer than 2A, so you’re more likely to create a bug when typing it, and of course more code === more testing work.


My final example is simple, predictable layouts, EG:


Sample 1:

var widget = // .. snip get a widget

widget.setLayout(new qx.ui.layout.VBox());


Sample 2:

var widget = // .. snip get a widget

var oldLayout = widget.getLayout();

if (oldLayout)

oldLayout.dispose();

widget.setLayout(new qx.ui.layout.VBox());


Here, Sample 2 is arguably safer (given what we know about the implementation of setLayout) but is those 3 extra lines something that we will reliably type (and not make typos in, and actually test) every time we want to set the “layout” property value?  What about properties less well known than “layout”?


In all of these examples, there is no clear answer when reading the code as to who should handle the memory management and in many cases the work required to really satisfy the safe disposal of objects is difficult to know.  To me, this is exactly why garbage collection features so prominently in modern languages.  Why then does it seem like we’ve gone back a step with Qooxdoo and its reliance on the ObjectRegistry?


Searching through the code reveals some possible explanations, but the comments for ObjectRegistry kind of says it all: 

/**

 * Registration for all instances of qooxdoo classes. Mainly

 * used to manage them for the final shutdown sequence and to

 * use weak references when connecting widgets to DOM nodes etc.

 *

 * @ignore(qx.dev, qx.dev.Debug.*)

 */


This says that the ObjectRegistry is there so that all un-disposed objects can be disposed, and so that there are weak references between DOM nodes and the Widgets they represent.  IMHO it’s possible to show that the weak links from DOM objects are the only reason that matters.


Searching the code, and ignoring any debug specific code (e.g. qx.core.Logger and qx.dev.*), the only things that depend on ObjectRegistry having a lookup of every object are qx.ui.core.Widget and qx.data.controller.Tree.  Code elsewhere depends on a widget having a hashCode, but crucially *not* depending on that hashCode being listed in the ObjectRegistry in a big lookup table of everything.


In my own code, I use an object’s toHashCode and use the result in a lookup table - but only as a key in my own map, and rarely (if at all) by passing to qx.core.ObjectRegistry.fromHashCode().


If true for others then this is an important point because it means that by only registering objects with ObjectRegistry that _need_ to be registered (i.e. Widgets), memory management issues can almost completely disappear.  To test this, I modified qx.ui.core.Widget.construct, qx.html.Element.__flush, and qx.data.controller.Tree.bindProperty so that all qx.core.Object’s have a hashCode, but only Widgets are registered with ObjectRegistry.


…and then re-ran the Qooxdoo framework test suite.  At the end of the run only 413 objects were not disposed.  Not much, right?  But without this patch there were 4,500 objects unfreed.  (OK it’s fair to say that unit tests are not required to free up resources that they consume, but it is striking how easy it is to have significant resource leaks that can be so easily countered by garbage collection)


A potential issue is that with garbage disposal, there are no destructors.  That’s not to say that Qooxdoo destructors will not work, but they are only called if the objects are explicitly disposed() - and if automatic memory management is the norm, its increasingly less likely that the destructors will ever get called.


The disadvantages of this proposal is that it includes a change to a core part of the library and that therefore brings risk of issues; that destructors are no longer possible and that fromHashCode cannot be used without extra code are small but potentially important details.  On the other hand, the potential benefits are significant in reduced development time and reduced coding errors - the work necessary to manually manage memory can be considerable, and very difficult to test especially when it’s dependent on user interaction.  


I’ve created a branch in my fork of Qooxdoo to experiment with this, which you can see here: https://github.com/johnspackman/qooxdoo/tree/automatic-memory-management The automatic-memory-management branch is from the current master so if you’re using Qooxdoo 5.x you should be able to switch to it and experiment without any other side effects.


Any thoughts are welcome :)


Regards

John


------------------------------------------------------------------------------

_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel
Reply | Threaded
Open this post in threaded view
|

Re: Qooxdoo Automatic Memory Management - a proposal

Varol Okan
John,

I think this analysis is a great.
I have not looked at your changes but from this e-mail they seem small.

I know that most JS developers will not actively manage the memory in their applications, only the good developers do, or if a memory leak is obviously causing harm.

If we could get this integrated in QooxDoo, maybe with a switch to enable this feature. E.g. It could be added as a switch during the compilation it would not affect existing projects unless they would chose to enable this feature.

The when version 6.0 comes around it could be enabled per default.

Cheers,

Varol :)


On 01/02/2016 06:12 AM, John Spackman wrote:

Hi all


Apologies for a rather long post, please bear with me :)


I’ve been thinking a lot lately about memory management in Qooxdoo apps, specifically how to reliably remove the need to call dispose() (without causing leaks) and reducing complexity to our applications — I have a proposal for how to achieve this but it requires some subtle changes to the Qooxdoo framework.  In theory this would be largely backward compatible (with a few specific exceptions) and would make it significantly easier to write long-running or memory sensitive applications.


Although the mantra is that “if you created it, you should dispose it”, this is increasingly hard to follow as an application becomes more complex.  There are specific examples of why this is virtually impossible baked into Qooxdoo, for example, some classes override setXxxx methods to *copy* the passed in object rather than take ownership of it - which means that the calling code has to understand the private implementation of the object in order to correctly handle cleanup.  qx.ui.form.SelectBox.setSelected is an example of this - the caller has to “know” that setSelected will copy the array it is given and not take it over and be responsible for disposing it.


Another common example with qx.data.Array is that methods such as splice and slice return an array of the removed items, but the returned object is an instance of qx.data.Array so you *must* dispose it.  To illustrate my point, take a look at these samples of code and try to determine which is the “correct” one:


Sample 1A:

var selection = // … snip: get an array (.. or is it a qx.data.Array ..?)

selection.slice(0,1); // delete the first element


Sample 1B:

var selection = // … snip: get an array (.. or is it a qx.data.Array ..?)

selection.slice(0,1).dispose(); // delete the first element


In order to figure out which piece of code is correct, you must understand the “…snip: get an array” and know if it is a native array or a qx.data.Array


How about these examples:


Sample 2A:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

widget.setSelection(selection);


Sample 2B:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

widget.setSelection(selection);

selection.dispose();


Sample 2C:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

var oldSelection = widget.getSelection();

widget.setSelection(selection);

if (oldSelection)

oldSelection.dispose();


In order to choose from those samples, you must understand what the widget object is, where it came from, _and_ how it implements setSelection to know whether and what to dispose.  This is not good - not only is the answer obscure, but if the implementation of widget.setSelection changes, your code may break.  Sample 2C is also much longer than 2A, so you’re more likely to create a bug when typing it, and of course more code === more testing work.


My final example is simple, predictable layouts, EG:


Sample 1:

var widget = // .. snip get a widget

widget.setLayout(new qx.ui.layout.VBox());


Sample 2:

var widget = // .. snip get a widget

var oldLayout = widget.getLayout();

if (oldLayout)

oldLayout.dispose();

widget.setLayout(new qx.ui.layout.VBox());


Here, Sample 2 is arguably safer (given what we know about the implementation of setLayout) but is those 3 extra lines something that we will reliably type (and not make typos in, and actually test) every time we want to set the “layout” property value?  What about properties less well known than “layout”?


In all of these examples, there is no clear answer when reading the code as to who should handle the memory management and in many cases the work required to really satisfy the safe disposal of objects is difficult to know.  To me, this is exactly why garbage collection features so prominently in modern languages.  Why then does it seem like we’ve gone back a step with Qooxdoo and its reliance on the ObjectRegistry?


Searching through the code reveals some possible explanations, but the comments for ObjectRegistry kind of says it all: 

/**

 * Registration for all instances of qooxdoo classes. Mainly

 * used to manage them for the final shutdown sequence and to

 * use weak references when connecting widgets to DOM nodes etc.

 *

 * @ignore(qx.dev, qx.dev.Debug.*)

 */


This says that the ObjectRegistry is there so that all un-disposed objects can be disposed, and so that there are weak references between DOM nodes and the Widgets they represent.  IMHO it’s possible to show that the weak links from DOM objects are the only reason that matters.


Searching the code, and ignoring any debug specific code (e.g. qx.core.Logger and qx.dev.*), the only things that depend on ObjectRegistry having a lookup of every object are qx.ui.core.Widget and qx.data.controller.Tree.  Code elsewhere depends on a widget having a hashCode, but crucially *not* depending on that hashCode being listed in the ObjectRegistry in a big lookup table of everything.


In my own code, I use an object’s toHashCode and use the result in a lookup table - but only as a key in my own map, and rarely (if at all) by passing to qx.core.ObjectRegistry.fromHashCode().


If true for others then this is an important point because it means that by only registering objects with ObjectRegistry that _need_ to be registered (i.e. Widgets), memory management issues can almost completely disappear.  To test this, I modified qx.ui.core.Widget.construct, qx.html.Element.__flush, and qx.data.controller.Tree.bindProperty so that all qx.core.Object’s have a hashCode, but only Widgets are registered with ObjectRegistry.


…and then re-ran the Qooxdoo framework test suite.  At the end of the run only 413 objects were not disposed.  Not much, right?  But without this patch there were 4,500 objects unfreed.  (OK it’s fair to say that unit tests are not required to free up resources that they consume, but it is striking how easy it is to have significant resource leaks that can be so easily countered by garbage collection)


A potential issue is that with garbage disposal, there are no destructors.  That’s not to say that Qooxdoo destructors will not work, but they are only called if the objects are explicitly disposed() - and if automatic memory management is the norm, its increasingly less likely that the destructors will ever get called.


The disadvantages of this proposal is that it includes a change to a core part of the library and that therefore brings risk of issues; that destructors are no longer possible and that fromHashCode cannot be used without extra code are small but potentially important details.  On the other hand, the potential benefits are significant in reduced development time and reduced coding errors - the work necessary to manually manage memory can be considerable, and very difficult to test especially when it’s dependent on user interaction.  


I’ve created a branch in my fork of Qooxdoo to experiment with this, which you can see here: https://github.com/johnspackman/qooxdoo/tree/automatic-memory-management The automatic-memory-management branch is from the current master so if you’re using Qooxdoo 5.x you should be able to switch to it and experiment without any other side effects.


Any thoughts are welcome :)


Regards

John



------------------------------------------------------------------------------


_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel


------------------------------------------------------------------------------

_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel
Reply | Threaded
Open this post in threaded view
|

Re: Qooxdoo Automatic Memory Management - a proposal

Petr Kobalíček
I think the whole concept of ObjectRegistry should be dropped. It creates nothing but overhead and it's really annoying to manually dispose objects. I remember that these weak references to connect DOM elements and widgets were created more than 10 years ago to support IE6, which is now dead. There is no modern browser that has problem with this anymore.

On Sat, Jan 2, 2016 at 1:16 PM, Varol Okan <[hidden email]> wrote:
John,

I think this analysis is a great.
I have not looked at your changes but from this e-mail they seem small.

I know that most JS developers will not actively manage the memory in their applications, only the good developers do, or if a memory leak is obviously causing harm.

If we could get this integrated in QooxDoo, maybe with a switch to enable this feature. E.g. It could be added as a switch during the compilation it would not affect existing projects unless they would chose to enable this feature.

The when version 6.0 comes around it could be enabled per default.

Cheers,

Varol :)



On 01/02/2016 06:12 AM, John Spackman wrote:

Hi all


Apologies for a rather long post, please bear with me :)


I’ve been thinking a lot lately about memory management in Qooxdoo apps, specifically how to reliably remove the need to call dispose() (without causing leaks) and reducing complexity to our applications — I have a proposal for how to achieve this but it requires some subtle changes to the Qooxdoo framework.  In theory this would be largely backward compatible (with a few specific exceptions) and would make it significantly easier to write long-running or memory sensitive applications.


Although the mantra is that “if you created it, you should dispose it”, this is increasingly hard to follow as an application becomes more complex.  There are specific examples of why this is virtually impossible baked into Qooxdoo, for example, some classes override setXxxx methods to *copy* the passed in object rather than take ownership of it - which means that the calling code has to understand the private implementation of the object in order to correctly handle cleanup.  qx.ui.form.SelectBox.setSelected is an example of this - the caller has to “know” that setSelected will copy the array it is given and not take it over and be responsible for disposing it.


Another common example with qx.data.Array is that methods such as splice and slice return an array of the removed items, but the returned object is an instance of qx.data.Array so you *must* dispose it.  To illustrate my point, take a look at these samples of code and try to determine which is the “correct” one:


Sample 1A:

var selection = // … snip: get an array (.. or is it a qx.data.Array ..?)

selection.slice(0,1); // delete the first element


Sample 1B:

var selection = // … snip: get an array (.. or is it a qx.data.Array ..?)

selection.slice(0,1).dispose(); // delete the first element


In order to figure out which piece of code is correct, you must understand the “…snip: get an array” and know if it is a native array or a qx.data.Array


How about these examples:


Sample 2A:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

widget.setSelection(selection);


Sample 2B:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

widget.setSelection(selection);

selection.dispose();


Sample 2C:

var widget = // … snip: get a widget

var selection = new qx.data.Array();

var oldSelection = widget.getSelection();

widget.setSelection(selection);

if (oldSelection)

oldSelection.dispose();


In order to choose from those samples, you must understand what the widget object is, where it came from, _and_ how it implements setSelection to know whether and what to dispose.  This is not good - not only is the answer obscure, but if the implementation of widget.setSelection changes, your code may break.  Sample 2C is also much longer than 2A, so you’re more likely to create a bug when typing it, and of course more code === more testing work.


My final example is simple, predictable layouts, EG:


Sample 1:

var widget = // .. snip get a widget

widget.setLayout(new qx.ui.layout.VBox());


Sample 2:

var widget = // .. snip get a widget

var oldLayout = widget.getLayout();

if (oldLayout)

oldLayout.dispose();

widget.setLayout(new qx.ui.layout.VBox());


Here, Sample 2 is arguably safer (given what we know about the implementation of setLayout) but is those 3 extra lines something that we will reliably type (and not make typos in, and actually test) every time we want to set the “layout” property value?  What about properties less well known than “layout”?


In all of these examples, there is no clear answer when reading the code as to who should handle the memory management and in many cases the work required to really satisfy the safe disposal of objects is difficult to know.  To me, this is exactly why garbage collection features so prominently in modern languages.  Why then does it seem like we’ve gone back a step with Qooxdoo and its reliance on the ObjectRegistry?


Searching through the code reveals some possible explanations, but the comments for ObjectRegistry kind of says it all: 

/**

 * Registration for all instances of qooxdoo classes. Mainly

 * used to manage them for the final shutdown sequence and to

 * use weak references when connecting widgets to DOM nodes etc.

 *

 * @ignore(qx.dev, qx.dev.Debug.*)

 */


This says that the ObjectRegistry is there so that all un-disposed objects can be disposed, and so that there are weak references between DOM nodes and the Widgets they represent.  IMHO it’s possible to show that the weak links from DOM objects are the only reason that matters.


Searching the code, and ignoring any debug specific code (e.g. qx.core.Logger and qx.dev.*), the only things that depend on ObjectRegistry having a lookup of every object are qx.ui.core.Widget and qx.data.controller.Tree.  Code elsewhere depends on a widget having a hashCode, but crucially *not* depending on that hashCode being listed in the ObjectRegistry in a big lookup table of everything.


In my own code, I use an object’s toHashCode and use the result in a lookup table - but only as a key in my own map, and rarely (if at all) by passing to qx.core.ObjectRegistry.fromHashCode().


If true for others then this is an important point because it means that by only registering objects with ObjectRegistry that _need_ to be registered (i.e. Widgets), memory management issues can almost completely disappear.  To test this, I modified qx.ui.core.Widget.construct, qx.html.Element.__flush, and qx.data.controller.Tree.bindProperty so that all qx.core.Object’s have a hashCode, but only Widgets are registered with ObjectRegistry.


…and then re-ran the Qooxdoo framework test suite.  At the end of the run only 413 objects were not disposed.  Not much, right?  But without this patch there were 4,500 objects unfreed.  (OK it’s fair to say that unit tests are not required to free up resources that they consume, but it is striking how easy it is to have significant resource leaks that can be so easily countered by garbage collection)


A potential issue is that with garbage disposal, there are no destructors.  That’s not to say that Qooxdoo destructors will not work, but they are only called if the objects are explicitly disposed() - and if automatic memory management is the norm, its increasingly less likely that the destructors will ever get called.


The disadvantages of this proposal is that it includes a change to a core part of the library and that therefore brings risk of issues; that destructors are no longer possible and that fromHashCode cannot be used without extra code are small but potentially important details.  On the other hand, the potential benefits are significant in reduced development time and reduced coding errors - the work necessary to manually manage memory can be considerable, and very difficult to test especially when it’s dependent on user interaction.  


I’ve created a branch in my fork of Qooxdoo to experiment with this, which you can see here: https://github.com/johnspackman/qooxdoo/tree/automatic-memory-management The automatic-memory-management branch is from the current master so if you’re using Qooxdoo 5.x you should be able to switch to it and experiment without any other side effects.


Any thoughts are welcome :)


Regards

John



------------------------------------------------------------------------------


_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel


------------------------------------------------------------------------------

_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel



------------------------------------------------------------------------------

_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel
Reply | Threaded
Open this post in threaded view
|

Re: Qooxdoo Automatic Memory Management - a proposal

John Spackman-3
Hi Petr, Varol

Thanks for the feedback :)

Petr, can we show that is definitive, that DOM->JS leaks have been fixed?  Because if so then I agree that the registry can be eliminated completely.  My concern is that I still see leaks in IE11 that survive page refreshes and wouldn’t want to make the problem worse (although I can’t show that’s not a client problem either :( ).  

ObjectRegistry still needs to remain so that objects can have a unique hash code, but if we can show DOM->JS is not a problem then the central list-of-every-object-ever can go.

I find that with widgets, disposing is much less of a concern because either widgets are very rarely removed from the screen once created, or the default parent/child handling takes care of it.  The exception to this is things like ListItem etc which are routinely created and destroyed in response to user interactions, and this significantly complicates controller code so removing it from widgets also would definitely be a good thing.

John.

From: Petr Kobalíček <[hidden email]>
Reply-To: qooxdoo Development <[hidden email]>
Date: Saturday, 2 January 2016 at 16:30
To: qooxdoo Development <[hidden email]>
Subject: Re: [qooxdoo-devel] Qooxdoo Automatic Memory Management - a proposal

I think the whole concept of ObjectRegistry should be dropped. It creates nothing but overhead and it's really annoying to manually dispose objects. I remember that these weak references to connect DOM elements and widgets were created more than 10 years ago to support IE6, which is now dead. There is no modern browser that has problem with this anymore.

On Sat, Jan 2, 2016 at 1:16 PM, Varol Okan <[hidden email]> wrote:
John,

I think this analysis is a great. 
I have not looked at your changes but from this e-mail they seem small.

I know that most JS developers will not actively manage the memory in their applications, only the good developers do, or if a memory leak is obviously causing harm.

If we could get this integrated in QooxDoo, maybe with a switch to enable this feature. E.g. It could be added as a switch during the compilation it would not affect existing projects unless they would chose to enable this feature.

The when version 6.0 comes around it could be enabled per default.

Cheers,

Varol :)



------------------------------------------------------------------------------

_______________________________________________
qooxdoo-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel