500548 - add WeakFunctionObject and WeakMethodClosure to Tamarin

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Steven Johnson]

FlashPlayer has "weak" versions of Function and MethodClosure, used to implement weak EventListeners (ie, the listener can be collected even if it has not explicitly removed itself). The code that does this is very intimate with internals of the VM and probably needs to be moved into the VM itself.

Comment 1

[Steven Johnson]

Attachment: Patch

Initial stab at accomplishing this; this patch isn't ready for review (much less landing):
-- more virtual method use than I'd like
-- the Weak variants should probably do isValid() checking internally to prevent misuse
-- ugly ugly usage of "GCObject*" slots in the base class to avoid unnecessary duplucate slots; it works but ewwwwww

OTOH this has a distinct advantage over the variants currently in FP: the weak versions have proper "is-a" relationship with parents, while the Flash versions don't (and have massive special casing as a result)

Saving the patch here since it's not on my critical path right now but I don't want to lose it.

Comment 2

[Steven Johnson]

Attachment: WeakMethodClosure

Upon further reflection, I don't think we need (or want) WeakFunctionObject; the one current use case can be replaced with a simple WeakRef-to-normal-FunctionObject. I've added WeakMethodClosure along with a bit of cleanup to FunctionObject and MethodObject.

Note that this differs from the one in Flash in a significant way: it only holds "savedThis" via WeakRef, but not the MethodEnv. I contend that this is an improvement, as the intent of WeakMethodClosure is to avoid holding the receiver in memory, and that the MethodEnv is irrelevant to our purposes. Since WeakRefs are cheap, but not free, we shouldn't use them when we don't need to, and I think we don't need to here. (Werner, I'm asking you for feedback as someone who knows the details of EventDispatcher better than I.)

Comment 3

[Jeff Mott]

Makes sense to me.

Comment 4

[Steven Johnson]

Gentle review ping for Edwin/Werner...

Comment 5

[Steven Johnson]

Comment on attachment 508020 [details] [diff] [review]
WeakMethodClosure

Peevishly changing reviewers in hopes of getting someone to look at this...

Comment 6

[Lars T Hansen]

Comment on attachment 508020 [details] [diff] [review]
WeakMethodClosure

Some general refactoring and renaming could have been a separate patch, but OK.

Copious use of tab characters here, are you back to Visual Studio? :-)

Grammo, "it return non-null ..."

Grammo, "this is never currently never even exposed..."

"static WeakMethodClosure* create()" needs to be REALLY_INLINE.  Most create functions are called from very few places, typically one (but sometimes more, which is why the pattern is worthwhile).  By that fact alone it is almost never desirable not to have them be REALLY_INLINE.  But there's more: the new operator is also REALLY_INLINE and when the size of the object can be precomputed the operator usually boils way into a direct call to a precomputed GC allocator.

Otherwise this all looks credible, I'm glad you stuck with using the standard weakrefs.

Comment 7

[Steven Johnson]

Re: tabs, doh, probably a side-effect from copy-pasting some source from the Flash version of these.

Re: grammar, clearly I need to redouble my efforts there.

re: inline, willfix.

Comment 8

[Tamarin Bot]

changeset: 5892:b20592f4833c
user:      Steven Johnson <[e-mail]>
summary:   Bug 500548 - add WeakMethodClosure to Tamarin (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/b20592f4833c

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Steven Johnson from comment #2)
> Note that this differs from the one in Flash in a significant way: it only
> holds "savedThis" via WeakRef, but not the MethodEnv. I contend that this is
> an improvement, as the intent of WeakMethodClosure is to avoid holding the
> receiver in memory, and that the MethodEnv is irrelevant to our purposes.
> Since WeakRefs are cheap, but not free, we shouldn't use them when we don't
> need to, and I think we don't need to here. (Werner, I'm asking you for
> feedback as someone who knows the details of EventDispatcher better than I.)

I think this change has introduced a memory leak.

Steven's statement above makes it sound like he has removed the reference to the MethodEnv entirely from instances of WeakMethodClosure ("the MethodEnv is irrelevant to our purposes").

But there *is* still a reference to the MethodEnv: The one that is being passed up the super-class chain, and retained in the parent class of MethodEnv.

I believe the old WeakMethodClosure (the one that was not part of the tamarin) held the env weakly, while the new one holds it strongly.  Getting rid of the weak-ref in the name of blind removal of "cheap but not free" WeakRef support is wrong-headed: You need to verify that the referenced object will be retained in the usual-case in order to justify changing the weak-ref to a strong one.

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #9)

Spawned off Bug 767410 to follow up on making the MethodEnv reference weak (again).