-
Notifications
You must be signed in to change notification settings - Fork 64
[Java.Interop] Better linker-friendly JavaObjectArray<T>.ValueMarshaler #546
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.
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
Merged
jpobst
merged 1 commit into
dotnet:master
from
jonpryor:jonp-no-activator-for-object-array
Dec 19, 2019
Merged
[Java.Interop] Better linker-friendly JavaObjectArray<T>.ValueMarshaler #546
jpobst
merged 1 commit into
dotnet:master
from
jonpryor:jonp-no-activator-for-object-array
Dec 19, 2019
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: dotnet/android#3393 When attempting to execute the `Java.Interop-Tests.dll` unit tests within Xamarin.Android, some of the `JavaObjectArray<T>` unit tests would fail because the linker was removing the default constructor for the `JavaObjectArray<T>.ValueMarshaler` type, and that type was only created via `Activator.CreateInstance()` (aka "Reflection"): Test 'Java.InteropTests.JavaObjectArray_Int32ArrayArray_ContractTest.CollectionContract`1.Clear' failed: System.MissingMethodException : Default constructor not found for type Java.Interop.JavaObjectArray`1+ValueMarshaler[[System.Int32[], mscorlib, Version=2.0.5.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]] at System.RuntimeType.CreateInstanceMono (System.Boolean nonPublic, System.Boolean wrapExceptions) at System.RuntimeType.CreateInstanceSlow (System.Boolean publicOnly, System.Boolean wrapExceptions, System.Boolean skipCheckThis, System.Boolean fillCache) at System.RuntimeType.CreateInstanceDefaultCtor (System.Boolean publicOnly, System.Boolean skipCheckThis, System.Boolean fillCache, System.Boolean wrapExceptions, System.Threading.StackCrawlMark& stackMark) [0x00027] at System.Activator.CreateInstance (System.Type type, System.Boolean nonPublic, System.Boolean wrapExceptions) at System.Activator.CreateInstance (System.Type type, System.Boolean nonPublic) at System.Activator.CreateInstance (System.Type type) at Java.Interop.JniRuntime+JniValueManager.GetValueMarshaler (System.Type type) at Java.Interop.JniRuntime+JniValueManager.GetValueMarshaler[T] () at Java.Interop.JavaObjectArray`1[T].SetElementAt (System.Int32 index, T value) at Java.Interop.JavaObjectArray`1[T]..ctor (System.Collections.Generic.IList`1[T] value) at Java.Interop.JavaObjectArray`1[T]..ctor (System.Collections.Generic.IEnumerable`1[T] value) at Java.InteropTests.JavaObjectArrayContractTest`1[T].CreateCollection (System.Collections.Generic.IEnumerable`1[T] values) at Cadenza.Collections.Tests.CollectionContract`1[T].Clear () In order for `runtime.ValueManager.GetValueMarshaler<T>()` to work in all cases, we need to ensure that the `JavaObjectArray<T>.ValueMarshaler` default constructor is preserved. How do we do that? There are two plausible ways to do that: 1. Introduce and use a `PreserveAttribute` type, or 2. Use constructs which are "IL friendly". [`PreserveAttribute`][0] is a custom attribute which the linker looks for "by name", not including the namespace, and allows a degree of control over linker behavior. While useful, it also has a tendency to be copied in numerous places -- Xamarin.Android and Xamarin.iOS both have copies of this type, in different namespaces! -- which in itself is not entirely desirable. Do we really want *another* copy of this type running around? `PreserveAttribute` also requires manual maintenance: you have to "know" it exists, and "know" how to use it, and "know" how it interacts with the linker, and if anything changes to invalidate that knowledge...there's ~nothing to verify that things are now wrong. This leaves the second solution -- use "IL friendly" constructs -- but in order to do so we need to know *how* to remove a Reflection-based `Activator.CreateInstance()` call with "something else" which does the "same" thing or better. Thus: Why are we using `Activator.CreateInstance()`? See also commit 77a6bf8, but: 1. We're within non-generic `JniRuntime.JniValueManager.GetValueMarshaler(System.Type)`, and 2. We know we need to return something that can marshal non-primitive arrays, and 3. We don't know, in terms of type parameters, what that type *is*. We need a `JavaObjectArray<T>.ValueMarshaler` instance, but `T` is provided by a `System.Type` instance. This is why we used `Activator.CreateInstance()`, so we could use: Activator.CreateInstance ( typeof (JavaObjectArray<>.ValueMarshaler) .MakeGenericType (elementType)); However, `Activator.CreateInstance()` is linker hostile, so how do we obtain a `JavaObjectArray<T>.ValueMarshaler` instance *without* reflection, while also allowing `T` to be specified at runtime? (This is where I'm thankful we're not a FullAOT environment...) In order to get the linker to preserve the constructor, the linker needs to "see" that the constructor is actually used. Do so: partial class JavaObjectArray<T> { internal static readonly ValueMarshaler Instance = new ValueMarshaler (); } That merely punts the problem. How do we get the linker to preserve the `JavaObjectArray<T>.Instance` field? We need an actual field ref: static JniValueMarshaler GetObjectArrayMarshalerHelper<T> () { return JavaObjectArray<T>.Instance; } That's not a complete solution, though: *something* needs to *statically* reference `GetObjectArrayMarshalerHelper<T>()` within IL so that the linker will preserve it. Otherwise, it'll be collected, which will cause the `JavaObjectArray<T>.Instance` field to be removed, as well as the `JavaObjectArray<T>.ValueMarshaler` constructor. How do we statically reference a generic method from a non-generic context? We do so by referencing it with type parameters specified, then use that reference to obtain the generic type definition of the method, then create a new method definition with our desired types. ...and we can do so (reasonably) sanely and (reasonably) efficiently by using delegates, which gives us a method via `ldftn`! Func<JniValueMarshaler> indirect = GetObjectArrayMarshalerHelper<object>; MethodInfo helperForObject = indirect.Method; MethodInfo helperForType = helperForObject.GetGenericMethodDefinition().MakeGenericMethod (elementType); Now that we have a `MethodInfo` for `GetObjectArrayMarshalerHelper<{elementType}>()`, we need only invoke it! Alas, `MethodInfo.Invoke()` is slow, so use more delegates! Func<JniValueMarshaler> direct = (Func<JniValueMarshaler>) Delegate.CreateDelegate (typeof (Func<JniValueMarshaler>), helperForType); return direct (); This allows us to have a reference to the generic `JavaObjectArray<T>.Instance` field "rooted" by the non-generic `JniValueMarshaler.GetValueMarshaler(Type)` method, in a way which is linker friendly and more efficient than `Activator.CreateInstance()`. [0]: https://docs.microsoft.com/en-us/dotnet/api/foundation.preserveattribute?view=xamarin-ios-sdk-12
jonpryor
added a commit
that referenced
this pull request
Dec 19, 2019
Try to make a single-change PR. This PR is only for `JavaExceptionTests` fixes. The JavaObjectArray<T> fixes are now at: #546
jpobst
approved these changes
Dec 19, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: dotnet/android#3393
When attempting to execute the
Java.Interop-Tests.dllunit testswithin Xamarin.Android, some of the
JavaObjectArray<T>unit testswould fail because the linker was removing the default constructor for
the
JavaObjectArray<T>.ValueMarshalertype, and that type was onlycreated via
Activator.CreateInstance()(aka "Reflection"):In order for
runtime.ValueManager.GetValueMarshaler<T>()to work inall cases, we need to ensure that the
JavaObjectArray<T>.ValueMarshalerdefault constructor is preserved.How do we do that?
There are two plausible ways to do that:
PreserveAttributetype, orPreserveAttributeis a custom attribute which the linker looksfor "by name", not including the namespace, and allows a degree of
control over linker behavior. While useful, it also has a tendency to
be copied in numerous places -- Xamarin.Android and Xamarin.iOS both
have copies of this type, in different namespaces! -- which in itself
is not entirely desirable. Do we really want another copy of this
type running around?
PreserveAttributealso requires manual maintenance: you have to"know" it exists, and "know" how to use it, and "know" how it
interacts with the linker, and if anything changes to invalidate that
knowledge...there's ~nothing to verify that things are now wrong.
This leaves the second solution -- use "IL friendly" constructs -- but
in order to do so we need to know how to remove a Reflection-based
Activator.CreateInstance()call with "something else" which does the"same" thing or better.
Thus: Why are we using
Activator.CreateInstance()? See alsocommit 77a6bf8, but:
We're within non-generic
JniRuntime.JniValueManager.GetValueMarshaler(System.Type), andWe know we need to return something that can marshal non-primitive
arrays, and
We don't know, in terms of type parameters, what that type is.
We need a
JavaObjectArray<T>.ValueMarshalerinstance, butTisprovided by a
System.Typeinstance.This is why we used
Activator.CreateInstance(), so we could use:However,
Activator.CreateInstance()is linker hostile, so how do weobtain a
JavaObjectArray<T>.ValueMarshalerinstance withoutreflection, while also allowing
Tto be specified at runtime?(This is where I'm thankful we're not a FullAOT environment...)
In order to get the linker to preserve the constructor, the linker
needs to "see" that the constructor is actually used. Do so:
That merely punts the problem. How do we get the linker to preserve
the
JavaObjectArray<T>.Instancefield? We need an actual field ref:That's not a complete solution, though: something needs to
statically reference
GetObjectArrayMarshalerHelper<T>()within ILso that the linker will preserve it. Otherwise, it'll be collected,
which will cause the
JavaObjectArray<T>.Instancefield to beremoved, as well as the
JavaObjectArray<T>.ValueMarshalerconstructor.How do we statically reference a generic method from a non-generic
context?
We do so by referencing it with type parameters specified, then use
that reference to obtain the generic type definition of the method,
then create a new method definition with our desired types.
...and we can do so (reasonably) sanely and (reasonably) efficiently
by using delegates, which gives us a method via
ldftn!Now that we have a
MethodInfoforGetObjectArrayMarshalerHelper<{elementType}>(), we need only invokeit! Alas,
MethodInfo.Invoke()is slow, so use more delegates!This allows us to have a reference to the generic
JavaObjectArray<T>.Instancefield "rooted" by the non-genericJniValueMarshaler.GetValueMarshaler(Type)method, in a way which islinker friendly and more efficient than
Activator.CreateInstance().