This is a great improvement, but I still have three comments. 1) This version looks better, but I agree with André 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?
2) To be complete, you really should also look for and remove redundant delegate destroy handling in all the components. 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? Thanks. On 2010-03-16, at 15:13, 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: Automatically remove invalid delegates in > LzEventable.destroy() > > Bugs Fixed: LPP-8822 - Improve layout performance > > Technical Reviewer: ptw > QA Reviewer: hminsky > > Details: 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/LzEventable.lzs > M WEB-INF/lps/lfc/views/LzInputText.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 lps/components/utils/layouts/resizelayout.lzx > > Changeset: > http://svn.openlaszlo.org/openlaszlo/patches/20100314-maxcarlson-0.tar >
