On 2010-03-17, at 15:33, André Bargull wrote: > 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. > - 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. > - 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. > - minor change: rename _delegates so it won't show up by default in the > debugger, e.g. __delegates +1 > 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. > - 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. > 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 >>> >>> >> >> >>
