Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove unused code in the interop layers #1255

Open
2 tasks
filipnavara opened this issue Sep 22, 2024 · 3 comments
Open
2 tasks

Remove unused code in the interop layers #1255

filipnavara opened this issue Sep 22, 2024 · 3 comments
Labels
enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java

Comments

@filipnavara
Copy link
Member

filipnavara commented Sep 22, 2024 •
edited
Loading

Casual look at the Java.Interop/java-interop projects that there's a lot of code that can be removed since it's no longer used:

  • Any code that supports Java's WeakReference instead of the JNI weak handle. This option is not used anymore but the native code to support it still exists:

    java-interop/src/java-interop/java-interop-gc-bridge-mono.cc

    Lines 1288 to 1292 in 5bd93ae

    case JAVA_INTEROP_GC_BRIDGE_USE_WEAK_REFERENCE_KIND_JAVA:
    message = "Using java.lang.ref.WeakReference for JNI Weak References.";
    take_global_ref = take_global_ref_java;
    take_weak_global_ref = take_weak_global_ref_java;
    break;
  • java_interop_strdup / java_interop_free
@filipnavara filipnavara mentioned this issue Sep 23, 2024
Draft
@jpobst jpobst added enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java labels Sep 30, 2024
@jonpryor
Copy link
Member

jonpryor commented Oct 23, 2024 •
edited
Loading

I had written:

The WeakReference code is effectively dead. The contents of src/java-interop is a "desktop port" of GC bridge code from dotnet/android for use with MonoVM, which we never got around to unifying. The WeakReference backend is no longer supported; see dotnet/android@2f93f76

Additionally, note that the use of WeakReference was when JNI Weak Global References didn't work (Android < API-14).

I was wrong. (Or at least, not quite right.)

dotnet/android@2f93f763 removes support for JNI Global References being direct object pointers. Meaning instead of there being an indirection (GREF value > GREF table > object instance), the GREF value was the object instance. In a world in which objects can move around in memory due to GC, "direct" references cannot work, but Android worked this way for quite some time (through API-13).

Consequently, dotnet/android@2f93f763 is entirely unrelated to our code to use java.lang.ref.WeakReference instead of JNI Weak Global References.

The java.lang.ref.WeakReference code can be used by setting the debug.mono.wref system property.

That said, we have no current tests which set this property, and I see no obvious usage of debug.mono.wref on GitHub, so while this code may not be "dead", it's close to it

jonpryor added a commit to dotnet/android that referenced this issue Oct 23, 2024
@jonpryor
[native, Mono.Android] Remove WeakReference support
8aa4bd0 
Context: dotnet/java-interop#1255
Context: xamarin/monodroid@a5c5290
Context: https://issuetracker.google.com/issues/36986478
Context: https://github.com/dotnet/android/blob/b56d68fe2a2c2dffa47eb7e41ae08a00fda86367/Documentation/workflow/SystemProperties.md#debugmonowref

Android did not support [JNI Weak Global References][0] until API-8
(Android 2.2).  To allow Mono for Android applications to run on
API-7 and earlier devices, while using JNI Weak Global References
when possible (fewer GC allocations, better performance), the GC
bridge supported two modes of operation:

  * `debug.mono.wref=jni`: Use JNI Weak Global References.
  * `debug.mono.wref=java`: Use [`java.lang.ref.WeakReference`][1].

With the exception of a ["minor" (lol) issue][2] in KitKat v4.4.2
(API-19) -- in which `JNIEnv::NewGlobalRef()` around a collected
JNI Weak Global Reference would return a non-`NULL` value, which
would result in app crashes; xamarin/monodroid@a5c52905 added
a workaround to use the `WeakReference` backend on Android 4.4.2 --
the `debug.mono.wref=java` backend has not been used since *2014*.

Apps *could* use [`@(AndroidEnvironment)`][3] to set
`debug.mono.wref=java`, but there is no reason to do so (worse
performance!), [nor has anyone done so on GitHub.com][4]
(no matches for `debug.mono.wref` within files that would be used
with `@(AndroidEnvironment)`).

Cleanup src/native and src/Mono.Android, and *remove* support for the
`debug.mono.wref=java` backend.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#weak_global_references
[1]: https://developer.android.com/reference/java/lang/ref/WeakReference
[2]: https://issuetracker.google.com/issues/36986478
[3]: https://learn.microsoft.com/dotnet/android/building-apps/build-items#androidenvironment
[4]: https://github.com/search?type=code&q=debug.mono.wref
@jonpryor jonpryor mentioned this issue Oct 23, 2024
Merged
jonpryor added a commit to dotnet/android that referenced this issue Oct 23, 2024
@jonpryor
[native, Mono.Android] Remove WeakReference support
6b24700 
Context: dotnet/java-interop#1255
Context: xamarin/monodroid@a5c5290
Context: https://issuetracker.google.com/issues/36986478
Context: https://github.com/dotnet/android/blob/b56d68fe2a2c2dffa47eb7e41ae08a00fda86367/Documentation/workflow/SystemProperties.md#debugmonowref

Android did not support [JNI Weak Global References][0] until API-8
(Android 2.2).  To allow Mono for Android applications to run on
API-7 and earlier devices, while using JNI Weak Global References
when possible (fewer GC allocations, better performance), the GC
bridge supported two modes of operation:

  * `debug.mono.wref=jni`: Use JNI Weak Global References.
  * `debug.mono.wref=java`: Use [`java.lang.ref.WeakReference`][1].

With the exception of a ["minor" (lol) issue][2] in KitKat v4.4.2
(API-19) -- in which `JNIEnv::NewGlobalRef()` around a collected
JNI Weak Global Reference would return a non-`NULL` value, which
would result in app crashes; xamarin/monodroid@a5c52905 added
a workaround to use the `WeakReference` backend on Android 4.4.2 --
the `debug.mono.wref=java` backend has not been used since *2014*.

Apps *could* use [`@(AndroidEnvironment)`][3] to set
`debug.mono.wref=java`, but there is no reason to do so (worse
performance!), [nor has anyone done so on GitHub.com][4]
(no matches for `debug.mono.wref` within files that would be used
with `@(AndroidEnvironment)`).

Cleanup src/native and src/Mono.Android, and *remove* support for the
`debug.mono.wref=java` backend.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#weak_global_references
[1]: https://developer.android.com/reference/java/lang/ref/WeakReference
[2]: https://issuetracker.google.com/issues/36986478
[3]: https://learn.microsoft.com/dotnet/android/building-apps/build-items#androidenvironment
[4]: https://github.com/search?type=code&q=debug.mono.wref
jonpryor added a commit to dotnet/android that referenced this issue Oct 23, 2024
@jonpryor
[native, Mono.Android] Remove WeakReference support
55632c3 
Context: dotnet/java-interop#1255
Context: xamarin/monodroid@a5c5290
Context: https://issuetracker.google.com/issues/36986478
Context: https://github.com/dotnet/android/blob/b56d68fe2a2c2dffa47eb7e41ae08a00fda86367/Documentation/workflow/SystemProperties.md#debugmonowref

Android did not support [JNI Weak Global References][0] until API-8
(Android 2.2).  To allow Mono for Android applications to run on
API-7 and earlier devices, while using JNI Weak Global References
when possible (fewer GC allocations, better performance), the GC
bridge supported two modes of operation:

  * `debug.mono.wref=jni`: Use JNI Weak Global References.
  * `debug.mono.wref=java`: Use [`java.lang.ref.WeakReference`][1].

With the exception of a ["minor" (lol) issue][2] in KitKat v4.4.2
(API-19) -- in which `JNIEnv::NewGlobalRef()` around a collected
JNI Weak Global Reference would return a non-`NULL` value, which
would result in app crashes; xamarin/monodroid@a5c52905 added
a workaround to use the `WeakReference` backend on Android 4.4.2 --
the `debug.mono.wref=java` backend has not been used since *2014*.

Apps *could* use [`@(AndroidEnvironment)`][3] to set
`debug.mono.wref=java`, but there is no reason to do so (worse
performance!), [nor has anyone done so on GitHub.com][4]
(no matches for `debug.mono.wref` within files that would be used
with `@(AndroidEnvironment)`).

Cleanup src/native and src/Mono.Android, and *remove* support for the
`debug.mono.wref=java` backend.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#weak_global_references
[1]: https://developer.android.com/reference/java/lang/ref/WeakReference
[2]: https://issuetracker.google.com/issues/36986478
[3]: https://learn.microsoft.com/dotnet/android/building-apps/build-items#androidenvironment
[4]: https://github.com/search?type=code&q=debug.mono.wref
jonpryor added a commit to dotnet/android that referenced this issue Oct 25, 2024
@jonpryor
[native, Mono.Android] Remove WeakReference support (#9454)
e98ae9c 
Context: dotnet/java-interop#1255
Context: xamarin/monodroid@a5c5290
Context: https://issuetracker.google.com/issues/36986478
Context: https://github.com/dotnet/android/blob/b56d68fe2a2c2dffa47eb7e41ae08a00fda86367/Documentation/workflow/SystemProperties.md#debugmonowref

Android did not support [JNI Weak Global References][0] until API-8
(Android 2.2).  To allow Mono for Android applications to run on
API-7 and earlier devices, while using JNI Weak Global References
when possible (fewer GC allocations, better performance), the GC
bridge supported two modes of operation:

  * `debug.mono.wref=jni`: Use JNI Weak Global References.
  * `debug.mono.wref=java`: Use [`java.lang.ref.WeakReference`][1].

With the exception of a ["minor" (lol) issue][2] in KitKat v4.4.2
(API-19) -- in which `JNIEnv::NewGlobalRef()` around a collected
JNI Weak Global Reference would return a non-`NULL` value, which
would result in app crashes; xamarin/monodroid@a5c52905 added
a workaround to use the `WeakReference` backend on Android 4.4.2 --
the `debug.mono.wref=java` backend has not been used since *2014*.

Apps *could* use [`@(AndroidEnvironment)`][3] to set
`debug.mono.wref=java`, but there is no reason to do so (worse
performance!), [nor has anyone done so on GitHub.com][4]
(no matches for `debug.mono.wref` within files that would be used
with `@(AndroidEnvironment)`).

Cleanup src/native and src/Mono.Android, and *remove* support for the
`debug.mono.wref=java` backend.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#weak_global_references
[1]: https://developer.android.com/reference/java/lang/ref/WeakReference
[2]: https://issuetracker.google.com/issues/36986478
[3]: https://learn.microsoft.com/dotnet/android/building-apps/build-items#androidenvironment
[4]: https://github.com/search?type=code&q=debug.mono.wref
@jonpryor
Copy link
Member

dotnet/android@e98ae9c removes support for debug.mono.wref=java, java.lang.ref.WeakReference, and the weak_handle fields from Java.Lang.Object, Java.Lang.Exception, Java.Interop.JavaObject, and Java.Interop.JavaException.

Development of #1257 can resume, if desired.

@filipnavara
Copy link
Member Author

Development of #1257 can resume, if desired.

I'll get to it in the upcoming week[s]. Thanks for the Android PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jonpryor @jpobst @filipnavara