I haven't yet tested the change, but there are some issues:
- LzDataAttrBind leaks now because it's not stored in _delegates
- swf8-kernel: heavy use of delegates but many objects aren't
LzEventables - leads to leaks through _delegates array
- you can create delegates even after the context is already destroyed,
_delegates array won't be cleared in that case
- minor change: rename _delegates so it won't show up by default in the
debugger, e.g. __delegates
I've suggested to merge _delegates with __LZdelegates - but after
thinking about it I see some potential issues:
- maybe a problem with releaseContraintMethod - will now also remove
delegates which aren't constraints
- LzLayout#releaseLayout(): similar problem like releaseContraintMethod
because releaseLayout() will also remove all constraints on the layout
On 3/17/2010 8:02 PM, P T Withington wrote:
Jus a few tweaks,
1) I'm seeing some spurious debugger output in smokecheck and 2 failures with
your change.
Tests: 939 Failures: 2 Errors: 0
TestFailure: /TestSuite/bug_3285/test1 failed: Same: expected 9 got 11
TestFailure: /TestSuite/bug_3285/test1 failed: Same: expected 10 got 12
2) Please make releaseLayout issue a deprecated warning in debug mode and say
that it will be removed in 5.1 (so we know when it is safe to remove).
3) You need to update to ToT, at least one file (LzAnimatorGroup) was stale.
4) In LzDelegate, you should move the `if (this.c == null)` inside the `if
($debug)`.
5) In LzState, instead of a separate __LZstatedelegates property, you could
just use _delegates (for the state node) and call __removeDelegates from
remove. That way state obeys the new protocol.
6) In delegates.dbk you could remove `this.myDel =`, since it is unused.
7) You'll need to update the copyrights on most of the files you are touching
to be able to check this in.
With those final tweaks, approved! Great to see this in.
On 2010-03-17, at 01:02, 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: UPDATED AGAIN: 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 to prevent memory leaks. This is handled automatically by the event
system.
Details: Updated to address Tucker's comments:
1) This version looks better, but I agree with Andre that this mechanism is
redundant with __LZdelegates. We need to integrate them. The only thing that
is happening in LzState is a kludge that temporarily sets the parent's
__LZdelegates to null so we can capture the delegates that were created as a
result of applying the state. This same kludge would work perfectly well with
your new mechanism. Can you revise one more time to do that?
Done.
2) To be complete, you really should also look for and remove redundant
delegate destroy handling in all the components.
Done.
3) Finally, the documentation needs to be updated (and a release note added)
about the new self-destroying behavior of delegates. I'm sure there a many
lines of user code wasted on tracking delegates for destroy methods that are
now redundant. In fact, perhaps you could add some debug code that detects
when a delegate has been destroyed by your new mechanism and issue a warning if
someone makes an unregisterAll call after the delegte is destroyed?
This should be taken care of - let me know if I missed anything in the docs.
components/* - Remove manual delegate tracking.
LaszloEvents - Warn when unregisterAll() is called on a previously destroyed
delegate.
Otherwise, the same as before, when I updated to address Andre's comments:
Not approved, because
1) delegates will now leak memory until their context is destroyed
True, but delegates are tiny, compared to the risk of leaking a entire nodes
that currently exists.
2) you really should integrate this approach with LzNode#__LZdelegates
I though of that, but __LZdelegates is used especially to track delegates that
need to be applied - see LzState.apply()
3) just setting the delegate's context to null doesn't look right,
instead call unregisterAll() and prevent further calls to register(..)
(if you intended to remove the reference from the delegate to the
context, you also should set the method reference to null, because of
bound methods, cf. ActionScript3)
Agreed. this is done.
4) API change: releaseLayout()'s contract is changed, cf. last mail from
Tucker
Fixed.
5) missing integration with explicit delegate unregister code, cf.
LzInputText#destroy() (may be added in an additional change - but there
needs to be a note at least!)
I think I caught all these.
LzInputText,LzAnimatorGroup,LzDatapointer,LzDataset - Remove extra delegate
unregistration as it's now handled by the event system.
Otherwise the same as before:
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.
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/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 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
