Quantcast

JavaFinalize and .NET Finalizers

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

JavaFinalize and .NET Finalizers

Jeremy A. Kolb - ARA/NED
I'm investigating https://github.com/MvvmCross/MvvmCross/issues/1343 for a possible memory leak related to our version of the RecyclerView widget MvxRecyclerView and have run against an interesting issue.

When swapping out the itemsource in the adapter the RecyclerView instantiates ViewHolders but never seems to kill them no matter how many times I force a GC.  If I add a "~MvxRecyclerViewHolder()"  method to MvxRecyclerViewHolder I never get in there.  However I do hit the JavaFinalize method and now I am completely confused.

Why would I hit the JavaFinalizer and not the .NET one?  There is never an explicit Dispose on the MvxRecyclerView so I can't see why the Java -> .NET connection would be severed.

The only instance I've found where Xamarin uses JavaFinalize is at https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/Renderers/GenericAnimatorListener.cs#L35 which doesn't explain much.

Am I seeing a bug in Xamarin?  In MvvmCross's RecyclerView?  Or more likely: am I just confused about the interaction of two garbage collectors.

Jeremy

_______________________________________________
Monodroid mailing list
[hidden email]

UNSUBSCRIBE INFORMATION:
http://lists.ximian.com/mailman/listinfo/monodroid
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JavaFinalize and .NET Finalizers

Jonathan Pryor-3
On Jun 17, 2016, at 9:50 AM, Jeremy A. Kolb - ARA/NED <[hidden email]> wrote:
> I'm investigating https://github.com/MvvmCross/MvvmCross/issues/1343 for a possible memory leak related to our version of the RecyclerView widget MvxRecyclerView and have run against an interesting issue.
>
> When swapping out the itemsource in the adapter the RecyclerView instantiates ViewHolders but never seems to kill them no matter how many times I force a GC.  If I add a "~MvxRecyclerViewHolder()"  method to MvxRecyclerViewHolder I never get in there.  However I do hit the JavaFinalize method and now I am completely confused.
>
> Why would I hit the JavaFinalizer and not the .NET one?  There is never an explicit Dispose on the MvxRecyclerView so I can't see why the Java -> .NET connection would be severed.
>
> The only instance I've found where Xamarin uses JavaFinalize is at https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/Renderers/GenericAnimatorListener.cs#L35 which doesn't explain much.
>
> Am I seeing a bug in Xamarin?  In MvvmCross's RecyclerView?  Or more likely: am I just confused about the interaction of two garbage collectors.

The interaction is deep voodoo. :-)

As usual with these things, enable GREF logging:

        https://developer.xamarin.com/guides/android/troubleshooting/troubleshooting/#Global_Reference_Messages
        https://developer.xamarin.com/releases/android/xamarin.android_5/xamarin.android_5.1/#GREF_Logging_Changes

        $ adb shell setprop debug.mono.log gref,gc

The relevant GC bridge code is `take_global_ref_jni()`, `take_weak_global_ref_jni()`, and `gc_cross_references()`:

        https://github.com/xamarin/xamarin-android/blob/c3811e8/src/monodroid/jni/monodroid-glue.c#L1140-L1202
        https://github.com/xamarin/xamarin-android/blob/c3811e8/src/monodroid/jni/monodroid-glue.c#L1483-L1522

This is run as part of Mono’s GC bridge code…which I’m kinda flaky on. The *general* idea is as follows:

1. Mono’s GC looks for finalizable objects.
2. When a finalizable instance has no more managed references, it’s placed into some finalizer queue
3. *Before* the finalizer queue is emptied, the GC bridge is queried to determine if the instance is *actually* finalizable.
3.a. If the instance can be finalized, it will be during the next GC.
3.b. If the instance can’t be finalized, it will be kept alive.

The GC bridge, in this context, is `gc_cross_references()`, which is given a set of Java.Lang.Object instances, and:

1. Toggles all the GREFs to Weak GREFs…for the *entire object graph* the instance refers to. (Plus some mirroring of the object graph in Java-land…)
2. Kicks off a Java-side GC.
3. Checks to see if anything was collected. If it was collected, Mono’s GC is told that the instance can be finalized. Otherwise, the instance is kept alive.
4. For all alive instances, toggle the Weak GREFs to GREFs.

Note: mid-term plan is to unify that code with the ~equivalent code here:

        https://github.com/xamarin/java.interop/blob/master/src/java-interop/java-interop-gc-bridge-mono.c

That project keeps getting punted as more important work pops up…

Very important question: what Android version do you see this on? There are two Weak reference implementations, one that uses “proper” JNI Weak Global References, and one which uses java.lang.WeakReference (because API-8 was the first to support JNI Weak Global references, and then Android 4.4 + ART broke them…). The  WeakReference implementation doesn’t get as much testing, so…it’s possibly suspect unless you can discount it.

Returning to the original description…
`Java.Lang.Object.Handle` is a JNI Global Reference, which *should* keep the instance alive. Period. Thus, it shouldn’t be possible for `Object.JavaFinalize()` to be invoked while `Object.Handle` is not IntPtr.Zero.

I can think of *one* way it would happen, but `GenericAnimatorListener` doesn’t trigger it: if the class had a `T(IntPtr, JniHandleOwnership)` constructor, *and* the instance were `Dispose()`d, then I can see `JavaFinalize()` being invoked:

        class Silly : Java.Lang.Object {
                public Silly () {}
                public Silly (IntPtr h, JniHandleOwnership t) : base (h, t) {}
                protected override void JavaFinalize () {
                }
        }
        // …
        using (var s = new Silly ()) {
        }

Once `s.Dispose()` is invoked, nothing will keep the Java-side peer alive, so later `Object.finalize()` may eventually be invoked by the Java GC, but since there is no longer a registered peer for the JNI instance, we’ll invoke the `Silly(IntPtr, JniHandleOwnership)` constructor to *create a new* peer, and invoke `Silly.JavaFinalize()` on the newly created peer, *not* the original instance:

        // above `using` block + subsequent behavior is as if…
        new Silly ().Dispose();
        // time passes...
        new Silly (value, JniHandleOwnership.DoNotTransfer).JavaFinalize();

That’s clearly not the case in GenericAnimatorListener — it doesn’t have the right constructor.

That *may* be the case in MvxRecyclerViewHolder, as it *does* have the activation constructor:

        https://github.com/MvvmCross/MvvmCross-AndroidSupport/blob/e88c556/MvvmCross.Droid.Support.V7.RecyclerView/MvxRecyclerViewHolder.cs#L107-L109


        public MvxRecyclerViewHolder(IntPtr handle, JniHandleOwnership transfer)
            : base(handle, transfer)
        {}

I would thus suggest you look into *why* `MvxRecyclerViewHolder ` has an Activation constructor. I strongly suggest *avoiding* activation constructors unless absolutely necessary:

        https://developer.xamarin.com/guides/android/under_the_hood/architecture/#Java_Activation

The Activation constructor has a tendency to “hide” or “change” the behavior of actual buggy behavior, so the fact that `MvxRecyclerViewHolder` provides an activation constructor without overriding any virtual methods (other than Dispose()) does not fill me with confidence.

If this is happening, you’d see the `MvxRecyclerViewHolder._bindingContext` is null — along with all over members! You could check for that within your `JavaFinalize()` override, and see if the instance has “meaningful” data compared to what the instance had after construction.

Additionally, to track *managed* instance lifetimes, I suggest copious use of RuntimeHelpers.GetHashCode(object):

        https://msdn.microsoft.com/en-us/library/11tbk3h9(v=vs.110).aspx

 - Jon

_______________________________________________
Monodroid mailing list
[hidden email]

UNSUBSCRIBE INFORMATION:
http://lists.ximian.com/mailman/listinfo/monodroid
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JavaFinalize and .NET Finalizers

Jeremy A. Kolb - ARA/NED
Thanks Jon.  Reply is inline.

Jeremy

> -----Original Message-----
> From: [hidden email] [mailto:monodroid-
> [hidden email]] On Behalf Of Jonathan Pryor
> Sent: Monday, June 20, 2016 10:52 PM
> To: Monodroid for Android
> Subject: Re: [mono-android] JavaFinalize and .NET Finalizers
>
> On Jun 17, 2016, at 9:50 AM, Jeremy A. Kolb - ARA/NED <[hidden email]>
> wrote:
> > I'm investigating https://github.com/MvvmCross/MvvmCross/issues/1343
> for a possible memory leak related to our version of the RecyclerView widget
> MvxRecyclerView and have run against an interesting issue.
> >
> > When swapping out the itemsource in the adapter the RecyclerView
> instantiates ViewHolders but never seems to kill them no matter how many
> times I force a GC.  If I add a "~MvxRecyclerViewHolder()"  method to
> MvxRecyclerViewHolder I never get in there.  However I do hit the
> JavaFinalize method and now I am completely confused.
> >
> > Why would I hit the JavaFinalizer and not the .NET one?  There is never an
> explicit Dispose on the MvxRecyclerView so I can't see why the Java -> .NET
> connection would be severed.
> >
> > The only instance I've found where Xamarin uses JavaFinalize is at
> https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Pl
> atform.Android/Renderers/GenericAnimatorListener.cs#L35 which doesn't
> explain much.
> >
> > Am I seeing a bug in Xamarin?  In MvvmCross's RecyclerView?  Or more
> likely: am I just confused about the interaction of two garbage collectors.
>
> The interaction is deep voodoo. :-)
>
> As usual with these things, enable GREF logging:
>
> https://developer.xamarin.com/guides/android/troubleshooting/tro
> ubleshooting/#Global_Reference_Messages
> https://developer.xamarin.com/releases/android/xamarin.android_
> 5/xamarin.android_5.1/#GREF_Logging_Changes
>
> $ adb shell setprop debug.mono.log gref,gc
>
> The relevant GC bridge code is `take_global_ref_jni()`,
> `take_weak_global_ref_jni()`, and `gc_cross_references()`:
>
> https://github.com/xamarin/xamarin-
> android/blob/c3811e8/src/monodroid/jni/monodroid-glue.c#L1140-L1202
> https://github.com/xamarin/xamarin-
> android/blob/c3811e8/src/monodroid/jni/monodroid-glue.c#L1483-L1522
>
> This is run as part of Mono’s GC bridge code…which I’m kinda flaky on. The
> *general* idea is as follows:
>
> 1. Mono’s GC looks for finalizable objects.
> 2. When a finalizable instance has no more managed references, it’s placed
> into some finalizer queue 3. *Before* the finalizer queue is emptied, the GC
> bridge is queried to determine if the instance is *actually* finalizable.
> 3.a. If the instance can be finalized, it will be during the next GC.
> 3.b. If the instance can’t be finalized, it will be kept alive.
>
> The GC bridge, in this context, is `gc_cross_references()`, which is given a set
> of Java.Lang.Object instances, and:
>
> 1. Toggles all the GREFs to Weak GREFs…for the *entire object graph* the
> instance refers to. (Plus some mirroring of the object graph in Java-land…) 2.
> Kicks off a Java-side GC.
> 3. Checks to see if anything was collected. If it was collected, Mono’s GC is
> told that the instance can be finalized. Otherwise, the instance is kept alive.
> 4. For all alive instances, toggle the Weak GREFs to GREFs.
>
> Note: mid-term plan is to unify that code with the ~equivalent code here:
>
> https://github.com/xamarin/java.interop/blob/master/src/java-
> interop/java-interop-gc-bridge-mono.c
>
> That project keeps getting punted as more important work pops up…
>
> Very important question: what Android version do you see this on? There
> are two Weak reference implementations, one that uses “proper” JNI Weak
> Global References, and one which uses java.lang.WeakReference (because
> API-8 was the first to support JNI Weak Global references, and then Android
> 4.4 + ART broke them…). The  WeakReference implementation doesn’t get
> as much testing, so…it’s possibly suspect unless you can discount it.
>

I've tested this on a Tango device which is 4.4 ART (and I've definitely seen funny bugs with this device),
as well as the API23 emulator image.

> Returning to the original description…
> `Java.Lang.Object.Handle` is a JNI Global Reference, which *should* keep
> the instance alive. Period. Thus, it shouldn’t be possible for
> `Object.JavaFinalize()` to be invoked while `Object.Handle` is not IntPtr.Zero.
>
> I can think of *one* way it would happen, but `GenericAnimatorListener`
> doesn’t trigger it: if the class had a `T(IntPtr, JniHandleOwnership)`
> constructor, *and* the instance were `Dispose()`d, then I can see
> `JavaFinalize()` being invoked:
>
> class Silly : Java.Lang.Object {
> public Silly () {}
> public Silly (IntPtr h, JniHandleOwnership t) : base (h, t) {}
> protected override void JavaFinalize () {
> }
> }
> // …
> using (var s = new Silly ()) {
> }
>
> Once `s.Dispose()` is invoked, nothing will keep the Java-side peer alive, so
> later `Object.finalize()` may eventually be invoked by the Java GC, but since
> there is no longer a registered peer for the JNI instance, we’ll invoke the
> `Silly(IntPtr, JniHandleOwnership)` constructor to *create a new* peer, and
> invoke `Silly.JavaFinalize()` on the newly created peer, *not* the original
> instance:
>
> // above `using` block + subsequent behavior is as if…
> new Silly ().Dispose();
> // time passes...
> new Silly (value, JniHandleOwnership.DoNotTransfer).JavaFinalize();
>
> That’s clearly not the case in GenericAnimatorListener — it doesn’t have the
> right constructor.
>
> That *may* be the case in MvxRecyclerViewHolder, as it *does* have the
> activation constructor:
>
> https://github.com/MvvmCross/MvvmCross-
> AndroidSupport/blob/e88c556/MvvmCross.Droid.Support.V7.RecyclerView/
> MvxRecyclerViewHolder.cs#L107-L109
>
>
>         public MvxRecyclerViewHolder(IntPtr handle, JniHandleOwnership
> transfer)
>             : base(handle, transfer)
>         {}
>
> I would thus suggest you look into *why* `MvxRecyclerViewHolder ` has an
> Activation constructor. I strongly suggest *avoiding* activation constructors
> unless absolutely necessary:
>
> https://developer.xamarin.com/guides/android/under_the_hood/a
> rchitecture/#Java_Activation

We put Activation constructors almost everywhere because they tend to fix crashes when coming back to an activity.
Plus we allow almost all of our classes to be overridden by the user if they need to do so.  Maybe in this case MvxRecyclerViewHolder
doesn't need to use the constructor.  It's hard to tell when it is needed.

>
> The Activation constructor has a tendency to “hide” or “change” the behavior
> of actual buggy behavior, so the fact that `MvxRecyclerViewHolder` provides
> an activation constructor without overriding any virtual methods (other than
> Dispose()) does not fill me with confidence.

As far as I can tell Dispose() is not being called on MvxRecyclerViewHolder.

>
> If this is happening, you’d see the
> `MvxRecyclerViewHolder._bindingContext` is null — along with all over
> members! You could check for that within your `JavaFinalize()` override, and
> see if the instance has “meaningful” data compared to what the instance had
> after construction.

All the values appear to be meaningful data.

>
> Additionally, to track *managed* instance lifetimes, I suggest copious use of
> RuntimeHelpers.GetHashCode(object):
>
> https://msdn.microsoft.com/en-us/library/11tbk3h9(v=vs.110).aspx
>
>  - Jon

Thanks for the tips.  I'll enable debugging/logging  and see what I can find.

>
> _______________________________________________
> Monodroid mailing list
> [hidden email]
>
> UNSUBSCRIBE INFORMATION:
> http://lists.ximian.com/mailman/listinfo/monodroid
_______________________________________________
Monodroid mailing list
[hidden email]

UNSUBSCRIBE INFORMATION:
http://lists.ximian.com/mailman/listinfo/monodroid
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JavaFinalize and .NET Finalizers

Jonathan Pryor-3
On Jun 21, 2016, at 3:40 PM, Jeremy A. Kolb - ARA/NED <[hidden email]> wrote:
> We put Activation constructors almost everywhere because they tend to fix crashes when coming back to an activity.

There’s a fine line between a “fix” and “hides the symptoms of the actual problem.” :-)

> Plus we allow almost all of our classes to be overridden by the user if they need to do so.  Maybe in this case MvxRecyclerViewHolder
> doesn't need to use the constructor.  It's hard to tell when it is needed.

To me, there are only two circumstances in which you should provide the activation constructor:

1. You’re subclassing Android.App.Application or Android.App.Instrumentation.
2. You’re subclassing a Java type that contains virtual methods, and the constructor of the Java type can invoke those methods.

(2) is harder to narrow down, other than a blanket “Android.Views.View *might* require the activation constructor.”

This almost certainly does *not* describe `RecyclerView.ViewHolder`, as the only virtual (non-`final` instance) method it has is `Object.toString()`:

        https://developer.android.com/reference/android/support/v7/widget/RecyclerView.ViewHolder.html

> As far as I can tell Dispose() is not being called on MvxRecyclerViewHolder.

Normal GC finalizer behavior would also qualify. (I should have thought of that  before…) Once the Mono GC has finalized an instance, `Object.Handle` is null, and there’s no GREF to keep the Java instance alive. Consequently, `Object.finalize()`/`Object.JavaFinalize()` can be invoked on that instance…an instance which *can’t* be associated with anything meaningful (short of Reflection-fu) — because `Object.Handle` is null! — and now, 5 years on, I wonder why I ever bound that method in the first place…

Hm….

---

I think the docs describe what the Activation constructor is *for* reasonably well. A related topic would be, what’s the problem with providing the activation constructor?

Alternatively, how would providing the activation constructor “hide the symptoms of the actual problem”?

Let’s take an IRunnable implementation:

        https://github.com/xamarin/xamarin-android/blob/5777337/src/Mono.Android/Java.Lang/Thread.cs#L11-L54

Notice that it *doesn’t* have an activation constructor. This isn’t for lack of people asking for one (on many types!), because an exception was thrown because the activation constructor doesn’t exist:

        https://bugzilla.xamarin.com/show_bug.cgi?id=27408

Yes, not having an activation constructor can cause a NotSupportedException/MissingMethodException to be thrown. The fix *isn’t* to add the activation constructor.

The fix is to figure out why we were trying to use the activation constructor in the first place. In the case of Bug #27408 it was due to a bug in our handle management (multithreaded code is hard!).

Adding the activation constructor would have “fixed” that bug. It also means we’d have had ~no way to even know that something was wrong, and no way to reason about a fix.

I don’t want a platform that cargo-cults around bug fixes with no understanding of *why* things are happening. I’m trying to keep the cruft low, the “it works if I do *this* but I don’t know why” crap to a minimum.

Not that I’m always successful, but I *really* don’t want to hide bugs. I want them found, a bright light shone on them, and the bug exterminated with extreme prejudice. Adding activation constructors “everywhere” is anathema to this.

 - Jon

_______________________________________________
Monodroid mailing list
[hidden email]

UNSUBSCRIBE INFORMATION:
http://lists.ximian.com/mailman/listinfo/monodroid
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JavaFinalize and .NET Finalizers

Jeremy A. Kolb - ARA/NED
Jon,

I'm an idiot.   The GC wasn't actually happening but I thought that it was.  Once I cleared out my obj/ and bin/ directories and rebuilt I noticed that I had an error.  Once I got the GC going again and added in the RuntimeHelpers.GetHashCode calls I could see that the ViewHolders were being destroyed properly.  After I cleared the list and invoked the GC the JavaFinalizer would run.  One or two garbage collections after that would hit the .NET finalizer.

Thanks for the help and clearing up some of the magic that happens in a system with two GCs.

Jeremy

> -----Original Message-----
> From: [hidden email] [mailto:monodroid-
> [hidden email]] On Behalf Of Jonathan Pryor
> Sent: Tuesday, June 21, 2016 10:07 PM
> To: Monodroid for Android
> Subject: Re: [mono-android] JavaFinalize and .NET Finalizers
>
> On Jun 21, 2016, at 3:40 PM, Jeremy A. Kolb - ARA/NED <[hidden email]>
> wrote:
> > We put Activation constructors almost everywhere because they tend to
> fix crashes when coming back to an activity.
>
> There’s a fine line between a “fix” and “hides the symptoms of the actual
> problem.” :-)
>
> > Plus we allow almost all of our classes to be overridden by the user
> > if they need to do so.  Maybe in this case MvxRecyclerViewHolder doesn't
> need to use the constructor.  It's hard to tell when it is needed.
>
> To me, there are only two circumstances in which you should provide the
> activation constructor:
>
> 1. You’re subclassing Android.App.Application or
> Android.App.Instrumentation.
> 2. You’re subclassing a Java type that contains virtual methods, and the
> constructor of the Java type can invoke those methods.
>
> (2) is harder to narrow down, other than a blanket “Android.Views.View
> *might* require the activation constructor.”
>
> This almost certainly does *not* describe `RecyclerView.ViewHolder`, as the
> only virtual (non-`final` instance) method it has is `Object.toString()`:
>
> https://developer.android.com/reference/android/support/v7/widg
> et/RecyclerView.ViewHolder.html
>
> > As far as I can tell Dispose() is not being called on MvxRecyclerViewHolder.
>
> Normal GC finalizer behavior would also qualify. (I should have thought of
> that  before…) Once the Mono GC has finalized an instance, `Object.Handle`
> is null, and there’s no GREF to keep the Java instance alive. Consequently,
> `Object.finalize()`/`Object.JavaFinalize()` can be invoked on that instance…an
> instance which *can’t* be associated with anything meaningful (short of
> Reflection-fu) — because `Object.Handle` is null! — and now, 5 years on, I
> wonder why I ever bound that method in the first place…
>
> Hm….
>
> ---
>
> I think the docs describe what the Activation constructor is *for* reasonably
> well. A related topic would be, what’s the problem with providing the
> activation constructor?
>
> Alternatively, how would providing the activation constructor “hide the
> symptoms of the actual problem”?
>
> Let’s take an IRunnable implementation:
>
> https://github.com/xamarin/xamarin-
> android/blob/5777337/src/Mono.Android/Java.Lang/Thread.cs#L11-L54
>
> Notice that it *doesn’t* have an activation constructor. This isn’t for lack of
> people asking for one (on many types!), because an exception was thrown
> because the activation constructor doesn’t exist:
>
> https://bugzilla.xamarin.com/show_bug.cgi?id=27408
>
> Yes, not having an activation constructor can cause a
> NotSupportedException/MissingMethodException to be thrown. The fix
> *isn’t* to add the activation constructor.
>
> The fix is to figure out why we were trying to use the activation constructor in
> the first place. In the case of Bug #27408 it was due to a bug in our handle
> management (multithreaded code is hard!).
>
> Adding the activation constructor would have “fixed” that bug. It also means
> we’d have had ~no way to even know that something was wrong, and no
> way to reason about a fix.
>
> I don’t want a platform that cargo-cults around bug fixes with no
> understanding of *why* things are happening. I’m trying to keep the cruft
> low, the “it works if I do *this* but I don’t know why” crap to a minimum.
>
> Not that I’m always successful, but I *really* don’t want to hide bugs. I want
> them found, a bright light shone on them, and the bug exterminated with
> extreme prejudice. Adding activation constructors “everywhere” is
> anathema to this.
>
>  - Jon
>
> _______________________________________________
> Monodroid mailing list
> [hidden email]
>
> UNSUBSCRIBE INFORMATION:
> http://lists.ximian.com/mailman/listinfo/monodroid
_______________________________________________
Monodroid mailing list
[hidden email]

UNSUBSCRIBE INFORMATION:
http://lists.ximian.com/mailman/listinfo/monodroid
Loading...