Change 20100314-maxcarlson-0 by maxcarl...@bank on 2010-03-14 00:54:51 PST
in /Users/maxcarlson/openlaszlo/trunk-clean
for http://svn.openlaszlo.org/openlaszlo/trunk
Summary: UPDATE 6: Automatically remove invalid delegates in
LzEventable.destroy()
Bugs Fixed: LPP-8822 - Improve layout performance
Technical Reviewer: ptw
QA Reviewer: hminsky
Release Notes: It is no longer necessary to manually track and release
delegates whose target is 'this' to prevent memory leaks. This is handled
automatically by the event system.
Details: Updated to address Tucker's comments:
General comment:
It really sucks that LzDataAttrBind pretends to be an LzDelegate but isn't
really. We probably should use interface/implements to make that more
explicit. At least then the swf10 back-end could give us some help.
Nits:
1) LzEventable#105 Not sure what this means:
> > // uncomment to LFC delegates cleaned up, e.g. datapath and inputtext
> > //if ($debug) Debug.warn('removing %w delegates from %w.',
> > this.__delegates.length, this);
I removed this.
2) LzNode#400:
/** Tracks constraint delegates
@access private */
Might be better to say:
/** Tracks delegates and data bindings for states so they can be
applied/removed
because that is what we have reduced this to. Perhaps someday we can come up
with a less kludgey mechanism than state reaching out and grabbing these things
from the node constructor.
Done.
Issues:
1) LZNode#2080
> > //remove __LZconstraintdelegates
> > // We have to do this to remove LzDataAttrBinds
I don't follow. It seems to me that in LzNode/dataBindAttribute, the
LzDataAttrBind object needs to go on __LZconstraintDelegates, so it can be
applied/unapplied by LzState. But LzDataAttrBind's constructor should register
the binding on __delegates so that it can be auto-cleaned when the node it is
binding is destroyed. We really want to separate these two tasks so future
generations don't have to work this out again. Then you can truly just let
__LZconstraintdelegates drop on the floor in LzNode/destroy.
Yeah, you're right! Done.
2) LzDataAttrBind/unregisterAll: Does anyone actually call this any more?
LzEventable/destroy should just be calling LzDataAttrBind/destroy
Removed unregisterAll().
3) LaszloEvents#227: There needs to be a comment here. Something like
"Delegates between LzEventables can cause memory leaks. We record such
delegates so they can be cleaned up in destroy".
Done.
4) LaszloEvents#227: Do you not have to test that the context _is_ actually an
LzEventable here? At least in the runtimes that don't enforce types?
Ooh good point! I improved the testing and ensured LzDelegate.__tracked is
correctly maintained.
5) LaszloView#1795: Why did you un-deprecate releaseLayouts? I thought we
were trying to get rid of this?
I removed it since it's deprecated in 4.7.1 - but there doesn't seem to be a
replacement. It could be useful when you want to move things around without
layouts interfering - but you could lock/unlock the layouts instead. Maybe we
should make the layouts array public so folks could do this? The replication
manager does this during updates...
Otherwise, the same as r5 (Updated to address Andre's comments):
LzDelegate:
> + if (eventSender !== this.c) {
> + var _dels:Array = this.c['__delegates'];
> + if (_dels == null) {
> + this.c.__delegates = [ this ];
> + } else {
> + _dels.push( this );
> + }
> + }
1) This will create multiple entries for each delegate, you should add a
boolean flag to track whether the delegate was already added to the __delegates
array to prevent more than one entry.
Done.
2) What about the issue with the swf8 kernel using LzDelegates?
I updated the swf8 kernel to be very careful about unregistering all delegates
used on instances.
LzDelegate:
> +public function destroy ():void {
> + this.unregisterAll();
> + this.c = null;
> + this.m = null;
> +}
Please add "@access private" to hide the function from the docs - can you
remind me why functions are added to the docs by default?! There is a JIRA
issue to switch this default behaviour, isn't it?
Done. I think it's that way to force developers to notice that they need to
write documentation...
LzNode:
> + for ( var i = dels.length - 1; i >= 0; i-- ){
> + if (dels[i].c) {
> + dels[ i ].destroy();
> + }
> }
Won't work for LzDataAttrBind - there is no "c" attribute, so the if-test will
fail.
Done. I added a __LZdeleted flag to LzDelegate that can be used instead.
LaszloEvents - Add __events array to track events registered on the delegate
and make LzDelegate static. Add hasevents boolean to track whether calls to
unregisterAll() are needed. Add __tracked flag to prevent adding to
__delegates more than once. Add __LZdeleted flag to track when to destroy(),
mirroring LzEventable.
LzHTTPDataProvider - Track LzHTTPLoaders and call destroy() on each one when
destroyed.
LzDataset - Call destroy() on the dataprovider when destroyed.
kernel/*/LzHTTPLoader - Add destroy() method which is called when a loader
should be destroyed. Add stub methods for swf9 and dhtml.
LzState, LzNode - Use LzDelegate/LzDataAttrBind.__LZdeleted to decide whether
to call destroy().
kernel/swf/* - Ensure all delegates not used by global services are tracked and
destroyed. Use new hasevents flag to avoid unneeded calls to unregisterAll().
Otherwise, the same:
LzNode,LzState - Rename __LZdelegates -> __LZconstraintdelegates, shorten array
lookups where possible, use LzDelegate.destroy() instead of unregisterAll().
LaszloEvents - Add check/warning when a delegate is created for an invalid
context. Move __delegates tracking to register() and only add LzEventables
that are registering for a different context. Add warnings for unneeded
unregisterAll() calls, but comment them out due to too many warnings. Add
destroy() API that neuters and unregisters the delegate.
LzDataAttrBind - Add destroy() API for symmetry with LzDelegate.
LzDatapath - Use super.destroy() for efficiency.
LaszloView - releaseLayouts() now calls layout.destroy().
LzState - Rename __LZstatedelegates -> __LZstateconstraintdelegates.
Otherwise, the same as before:
LzInputText,LzAnimatorGroup,LzDatapointer,LzDataset - Remove extra delegate
unregistration as it's now handled by the event system.
LzEventable - Add _delegates array to hold delegates that have a context
pointing to this object. Clean out delegate contexts in destroy().
LaszloEvents - LzDelegate registers on its contexts' _delegates array.
LaszloLayout,resizelayout - don't track delegates anymore.
Tests: Run modified leak-components test in swf10 - the memory monitor in the
upper left shows much less leakage with this patch: you can click the monitor
to force garbage collection. Also run smokecheck across runtimes.
Files:
M test/components/lz/leak-components.lzx
M WEB-INF/lps/lfc/kernel/swf/LzTextSprite.as
M WEB-INF/lps/lfc/kernel/swf/LzMediaLoader.lzs
M WEB-INF/lps/lfc/kernel/swf/LzInputTextSprite.as
M WEB-INF/lps/lfc/kernel/swf/LzXMLLoader.lzs
M WEB-INF/lps/lfc/kernel/swf/LzMakeLoadSprite.as
M WEB-INF/lps/lfc/kernel/swf/LzHTTPLoader.as
M WEB-INF/lps/lfc/kernel/swf/LzSprite.as
M WEB-INF/lps/lfc/kernel/dhtml/LzHTTPLoader.js
M WEB-INF/lps/lfc/kernel/swf9/LzHTTPLoader.as
M WEB-INF/lps/lfc/core/LzNode.lzs
M WEB-INF/lps/lfc/core/LzEventable.lzs
M WEB-INF/lps/lfc/views/LzInputText.lzs
M WEB-INF/lps/lfc/views/LaszloView.lzs
M WEB-INF/lps/lfc/helpers/LzState.lzs
M WEB-INF/lps/lfc/events/LaszloEvents.lzs
M WEB-INF/lps/lfc/controllers/LzAnimatorGroup.lzs
M WEB-INF/lps/lfc/controllers/LaszloLayout.lzs
M WEB-INF/lps/lfc/data/LzDatapointer.lzs
M WEB-INF/lps/lfc/data/LzHTTPDataProvider.lzs
M WEB-INF/lps/lfc/data/LzDataset.lzs
M WEB-INF/lps/lfc/data/LzDatapath.lzs
M WEB-INF/lps/lfc/data/LzDataAttrBind.lzs
M docs/src/developers/delegates.dbk
M lps/components/rpc/ajax.lzx
M lps/components/extensions/html.lzx
M lps/components/utils/layouts/resizelayout.lzx
M lps/components/utils/replicator/replicator.lzx
M lps/components/av/rtmpstatus.lzx
M lps/components/base/basetabelement.lzx
M lps/components/base/basescrollbar.lzx
M lps/components/base/basecomponent.lzx
M lps/components/base/basewindow.lzx
M lps/components/base/submit.lzx
M lps/components/base/basetrackgroup.lzx
M lps/components/base/basedatacombobox.lzx
Changeset:
http://svn.openlaszlo.org/openlaszlo/patches/20100314-maxcarlson-0.tar
