Just a heads up: I'm getting an error trying to compile your change: > [java] ERRORS: > [java] > [/var/folders/4C/4C5C5+lq2RWP+U+8ZLnNq++++TQ/-Tmp-/lzswf9/lzgen1844514813459409919/LzDelegate.as: > 7] line unknown: Error: A super statement cannot occur after a this, super, > return, or throw statement.
[I'll continue to review in the mean time.] On 2010-03-22, at 14:10, 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 3: 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: From Andre and Tucker: > >> I haven't yet tested the change, but there are some issues: >>> - LzDataAttrBind leaks now because it's not stored in _delegates > That's a show-stopper. > > I opted to keep __LZdelegates around to ensure states and > releaseContraintMethod() work as expected, so this shouldn't be an issue. > >>> - swf8-kernel: heavy use of delegates but many objects aren't LzEventables >>> - leads to leaks through _delegates array > Good point. LzDelegate needs to be careful to _only_ add LzEventable's to > __delegates array. > > Done. > >>> - you can create delegates even after the context is already destroyed, >>> _delegates array won't be cleared in that case > Good point. So, in LzDelegate, if you are trying to create a delegate on a > deleted context: issue a warning in debug mode, and return null from the > constructor. > > Done. > >>> - minor change: rename _delegates so it won't show up by default in the >>> debugger, e.g. __delegates > +1 > > Done. > >>> 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 > I don't think this will be a problem, because it only removes delegates whose > method is the constraint method. This was already a kludge and I don't think > we are making it any worse here. The correct fix would be to have a > constraint be an object that maintains all the bookkeeping necessary to apply > and remove it. That's a separate bug, IMO. > > This was causing a regression in smokecheck, so I went back to useing > __LZdelegates to track constraint delegates. > >>> - LzLayout#releaseLayout(): similar problem like releaseContraintMethod >>> because releaseLayout() will also remove all constraints on the layout > If there are no callers of releaseLayout, we should just remove it from the > API immediately to solve this problem. > > I think the overall benefit of having automatic delegate management outweighs > these incompatibility costs. > > Done - removed releaseLayout() other than a deprecation warning. I also > removed view.releaseLayouts(). > > Round 1 from Tucker: > > 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 > > This was due to me merging __LZdelegates and _delegates. I opted to keep > __LZdelegates around to ensure releaseConstraintMethod() and states continue > to work as expected. > > 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). > > I removed it, except for a warning. > > 3) You need to update to ToT, at least one file (LzAnimatorGroup) was stale. > > Done. > > 4) In LzDelegate, you should move the `if (this.c == null)` inside the `if > ($debug)`. > > Done. > > 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. > > I decided against this, as it would remove all constraints on the state, e.g. > applied="${...}". > > 6) In delegates.dbk you could remove `this.myDel =`, since it is unused. > > Done. > > 7) You'll need to update the copyrights on most of the files you are touching > to be able to check this in. > > Done. > > > 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. > > LaszloView - Remove releaseLayouts(). > > 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/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 >
