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);
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.
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.
2) LzDataAttrBind/unregisterAll: Does anyone actually call this any more?
LzEventable/destroy should just be calling LzDataAttrBind/destroy
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".
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?
5) LaszloView#1795: Why did you un-deprecate releaseLayouts? I thought we
were trying to get rid of this?
On 2010-03-23, at 18:29, 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 4: 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:
> 1) In LzEventable, __removeDelegates does not need to be a separate function
> any more, does it?
>
> I asked laszlo-user about the need for layout.releaseLayout(). Until that's
> resolved, __removeDelegates is required :(.
>
> 2) LaszloEvents.lzs#102: deletes -> deleted
>
> Done.
>
> 3) Can we poll laszlo-user to see if anyone is using `releaseLayout*`?
>
> Done.
>
> 4) Perhaps rename __LZdelegates to make it more obvious that it is now _only_
> for tracking delegates that implement contraints?
>
> Done.
>
> Issues that still need to be addressed:
>
> 1) Compiler error in swf10 due to return before super call in LzDelegate
> constructor. (You can't conditionally execute a super call in a constructor.
> In ES Harmony I think the only thing you will be able to do is specify which,
> if any, of your args are passed to your super.)
>
> Fixed.
>
> 2) LzEventable should not be peeking inside an LzDelegate and smashing it's
> `c` and `m` properties (see issue #3 for why). You should create a new API
> for LzEventable (it might be called `destroy`) that does the work of
> unregistering and neutering it, and call that from LzEventable.
>
> Done. I added a destroy() API to LzDataAttrBind for symmetry - see
> LzNode.destroy().
>
> 3) We still don't quite have this tracking right. We don't need to create an
> entry for _every_ delegate, and I think we have the potential of adding a lot
> of useless overhead if we don't minimize this tracking to the minimum
> required. We only have to track delegates that are registered on events from
> another node. So I think that the decision whether or not to add a new
> delegate to __delegates in LaszloEvents should be in `register`, and only
> when the event sender is not the delegate context.
>
> Done.
>
> 4) LaszloeEvents#233
>
> `if (this.c && this.c['__LZdeleted']) {`
>
> This seems wrong. When you "destroy" a delegate in LzEventable, you set `c`
> to null, so this test will fail and you could end up adding a destroyed
> delegate to an event. I think the logic should be:
>
> `if ((this.c == null) || this.c['__LZdeleted']) {`
>
> Done.
>
> 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.
>
> Files:
> M test/components/lz/leak-components.lzx
> 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/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
>
