Approved. Let's check this in so we can get some QA on it (and before someone thinks up another amendment to delay the vote).
Nice work on a thorny problem! On 2010-03-24, at 17:45, Max Carlson wrote: > 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 >
