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
qooxdoo-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel
------------------------------------------------------------------------------
_______________________________________________
qooxdoo-devel mailing list
qooxdoo-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/qooxdoo-devel
