Still can't approve this, although we are getting close!
Nits:
1) In LzEventable, __removeDelegates does not need to be a separate function
any more, does it?
2) LaszloEvents.lzs#102: deletes -> deleted
3) Can we poll laszlo-user to see if anyone is using `releaseLayout*`?
4) Perhaps rename __LZdelegates to make it more obvious that it is now _only_
for tracking delegates that implement contraints?
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.)
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.
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.
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']) {`
Otherwise looks good.
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
>
