[Java.Interop] Avoid Type.GetType() in ManagedPeer

[jonpryor]

Fixes: #1165

Context: #1153
Context: #1157

When building for NativeAOT (#1153) or when building .NET Android apps with -p:IsAotcompatible=true (#1157), we get IL2057 warnings from ManagedPeer.cs:

    ManagedPeer.cs(93,19,93,112): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type.
    ManagedPeer.cs(156,18,156,65): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type.
    ManagedPeer.cs(198,35,198,92): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type.

These warnings are because ManagedPeer.Construct() and ManagedPeer.RegisterNativeMembers() use Type.GetType() on string values provided from Java code, and thus the IL trimmer does not have visibility into those strings, and thus cannot reliably determine which types need to be preserved:

// Java Callable Wrapper
/* partial */ class ManagedType
{
  public static final String __md_methods;
  static {
    __md_methods =
      "n_GetString:()Ljava/lang/String;:__export__\n" +
      "";
    net.dot.jni.ManagedPeer.registerNativeMembers (
        /* nativeClass */             ManagedType.class,
        /* assemblyQualifiedName */   "Example.ManagedType, Hello-NativeAOTFromJNI",
        /* methods */                 __md_methods);
  }

  public ManagedType (int p0)
  {
    super ();
    if (getClass () == ManagedType.class) {
      net.dot.jni.ManagedPeer.construct (
          /* self */                  this,
          /* assemblyQualifiedName */ "Example.ManagedType, Hello-NativeAOTFromJNI",
          /* constructorSignature */  "System.Int32, System.Runtime",
          /* arguments */             new java.lang.Object[] { p0 });
    }
  }
}

ManagedPeer.construct() passes two sets of assembly-qualified type names: assemblyQualifiedName contains the type to construct, while constructorSignature contains a :-separated list of assembly-qualified type names for the constructor parameters. Each of these are passed to Type.GetType().

ManagedPeer.registerNativeMembers() passes an assembly-qualified type name to ManagedPeer.RegisterNativeMembers(), which passes the assembly-qualified type name to Type.GetType() to find the type to register native methods for.

If we more strongly rely on JNI signatures, we can remove the need for Java Callable Wrappers to contain assembly-qualified type names entirely, thus removing the need for ManagedPeer to use Type.GetType(), removing the IL2057 warnings.

For ManagedPeer.construct(), assemblyQualifiedName can be replaced with getting the JNI type signature from self.getClass(), and constructorSignature can be replaced with a
JNI method signature of the calling constructor.

For ManagedPeer.registerNativeMembers(), assemblyQualifiedName can be replaced with getting the JNI type signature from nativeClass.

// Java Callable Wrapper
/* partial */ class ManagedType
{
  public static final String __md_methods;
  static {
    __md_methods =
      "n_GetString:()Ljava/lang/String;:__export__\n" +
      "";
    net.dot.jni.ManagedPeer.registerNativeMembers (
        /* nativeClass */             ManagedType.class,
        /* methods */                 __md_methods);
  }

  public ManagedType (int p0)
  {
    super ();
    if (getClass () == ManagedType.class) {
      net.dot.jni.ManagedPeer.construct (
          /* self */                  this,
          /* constructorSignature */  "(I)V",
          /* arguments */             new java.lang.Object[] { p0 });
    }
  }
}

Furthermore, if we add [DynamicallyAccessedMembers] to JniRuntime.JniTypeManager.GetType(), we can fix some IL2075 warnings which appeared after fixing the IL2057 warnings.

Aside: Excising assembly-qualified type names from Java Callable Wrappers had some "interesting" knock-on effects in the unit tests, requiring that more typemap information be explicitly provided. (This same information was implicitly provided before, via the provision of assembly-qualified type names everywhere…)

[jonpryor]

There is a remaining problem with this approach: there is no requirement of a 1:1 mapping between Java types and managed types. There are already scenarios in which the mapping is ambiguous, and the pre-existing solution of using assembly-qualified type names resolved the ambiguity: arrays.

Consider Java int[]: this has a JNI signature of [I, and can be bound as either int[] or Java.Interop.JavaInt32Array. The approach suggested in this PR does not handle that ambiguity, and will arbitrarily pick one (not sure which at the moment), preventing the other from being usable in constructor signatures.

This will be addressed in a future commit to this PR.

[jonathanpeppers]

My general questions about performance depend on how often these would be called at runtime by a MAUI app?

If it's at build time -- or rare cases, we probably won't even notice these.

[jonathanpeppers]

Does this happen on device? This looks like it is O(N^2). Is there a way to rework the data to be a Dictionary<Type, JniPrimitiveArrayInfo> to make this lookup faster? I guess you'd also want the keys to map to duplicate entries as the values.

[jonpryor]

Probably not, and we're talking O(N**2) on 8 entries, each containing 4 elements within e.ArrayTypes, or 32 entries total. O(N**2) on 32 entries should be negligible.

Is there a way to rework the data to be a Dictionary<Type, JniPrimitiveArrayInfo> to make this lookup faster?

Yes, but as this is static data, it'll be constructed during app startup, even for .NET Android (unless we make it conditional, and please no?), so I'm trying to figure out how to minimize startup overhead.

My assumption (un-timed) is that creating an array of structs will be faster than populating a Dictionary, particularly when we're dealing with so few elements (8).

[jonpryor]

So, about that init vs. lookup time…

Repro App

```csharp

using System.Diagnostics;

const int CreationCount = 1000000;
const int LookupCount = 1000000;

var a = Stopwatch.StartNew ();
for (int i = 0; i < CreationCount; ++i) {
CreateArray ();
}
a.Stop ();

var d = Stopwatch.StartNew ();
for (int i = 0; i < CreationCount; ++i) {
CreateDict ();
}
d.Stop ();

Console.WriteLine ($"Array Creation: {a.ElapsedMilliseconds}ms");
Console.WriteLine ($" Dict Creation: {d.ElapsedMilliseconds}ms");

var av = CreateArray ();
a = Stopwatch.StartNew ();
for (int i = 0; i < LookupCount; ++i) {
TryArrayLookup (av, typeof (int));
}
a.Stop ();

var bv = CreateDict ();
for (int i = 0; i < LookupCount; ++i) {
TryDictLookup (bv, typeof (int));
}
d.Stop ();
Console.WriteLine ($"Array Lookup: {a.ElapsedMilliseconds}ms");
Console.WriteLine ($" Dict Lookup: {d.ElapsedMilliseconds}ms");

static JniPrimitiveArrayInfo[] CreateArray ()
{
return new JniPrimitiveArrayInfo[]{
new ("Z", typeof (Boolean), typeof (Boolean[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaBooleanArray)),
new ("B", typeof (SByte), typeof (SByte[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaSByteArray)),
new ("C", typeof (Char), typeof (Char[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaCharArray)),
new ("S", typeof (Int16), typeof (Int16[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaInt16Array)),
new ("I", typeof (Int32), typeof (Int32[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaInt32Array)),
new ("J", typeof (Int64), typeof (Int64[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaInt64Array)),
new ("F", typeof (Single), typeof (Single[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaSingleArray)),
new ("D", typeof (Double), typeof (Double[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaDoubleArray)),
};
}

static bool TryArrayLookup (JniPrimitiveArrayInfo[] array, Type type)
{
foreach (var e in array) {
if (Array.IndexOf (e.ArrayTypes, type) < 0)
continue;
return true;
}
return false;
}

static Dictionary<Type, JniPrimitiveArrayInfo> CreateDict ()
{
return new Dictionary<Type, JniPrimitiveArrayInfo> () {
[typeof (Boolean)] = new ("Z", typeof (Boolean), typeof (Boolean[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaBooleanArray)),
[typeof (SByte) ] = new ("B", typeof (SByte), typeof (SByte[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaSByteArray)),
[typeof (Char) ] = new ("C", typeof (Char), typeof (Char[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaCharArray)),
[typeof (Int16) ] = new ("S", typeof (Int16), typeof (Int16[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaInt16Array)),
[typeof (Int32) ] = new ("I", typeof (Int32), typeof (Int32[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaInt32Array)),
[typeof (Int64) ] = new ("J", typeof (Int64), typeof (Int64[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaInt64Array)),
[typeof (Single) ] = new ("F", typeof (Single), typeof (Single[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaSingleArray)),
[typeof (Double) ] = new ("D", typeof (Double), typeof (Double[]), typeof (JavaArray), typeof (JavaPrimitiveArray), typeof (JavaDoubleArray)),
};
}

static bool TryDictLookup (Dictionary<Type, JniPrimitiveArrayInfo> dict, Type type)
{
foreach (var v in dict.Values) {
if (Array.IndexOf (v.ArrayTypes, type) < 0)
return true;
}
return false;
}

class JavaArray {}
class JavaPrimitiveArray : JavaArray {}
class JavaBooleanArray : JavaPrimitiveArray {}
class JavaSByteArray : JavaPrimitiveArray {}
class JavaCharArray : JavaPrimitiveArray {}
class JavaInt16Array : JavaPrimitiveArray {}
class JavaInt32Array : JavaPrimitiveArray {}
class JavaInt64Array : JavaPrimitiveArray {}
class JavaSingleArray : JavaPrimitiveArray {}
class JavaDoubleArray : JavaPrimitiveArray {}

readonly struct JniPrimitiveArrayInfo {
public readonly JniTypeSignature JniTypeSignature;
public readonly Type PrimitiveType;
public readonly Type[] ArrayTypes;
public JniPrimitiveArrayInfo (string jniSimpleReference, Type primitiveType, params Type[] arrayTypes)
{
JniTypeSignature = new JniTypeSignature (jniSimpleReference, arrayRank: 1, keyword: true);
PrimitiveType = primitiveType;
ArrayTypes = arrayTypes;
}
}

struct JniTypeSignature {
public string SimpleName;
public int ArrayRank;
public bool Keyword;
public JniTypeSignature (string simpleName, int arrayRank, bool keyword)
{
SimpleName = simpleName;
ArrayRank = arrayRank;
Keyword = keyword;
}
}

</details>

[jonpryor]

Apparently <details/> doesn't work like i thought, so here's the execution + summary:

Results:

% dotnet run
Array Creation: 646ms
 Dict Creation: 1131ms
Array Lookup: 29ms
 Dict Lookup: 1131ms

% dotnet run -c Release
Array Creation: 389ms
 Dict Creation: 770ms
Array Lookup: 21ms
 Dict Lookup: 770ms

As expected, array lookup is faster -- half the time! -- and lookup is also much faster with arrays.

That said, this investigation did present a bug in GetBuiltInTypeArraySignature(): it shouldn't have been looking at .ArrayTypes, but instead .PrimitiveType, because the callsite in GetTypeSignature(Type) "unwraps" arrays, which kinda makes that particular codepath unusable, as in GetBuiltInTypeArraySignature() should just be removed entirely. 🤔

[jonpryor]

…and GetBuiltInTypeArraySignature() is required; trying to remove it breaks unit tests. Which means my "repro app" isn't accurate on the lookup side. After updating it, arrays are still faster:

% dotnet run -c Release
Array Creation: 392ms
 Dict Creation: 753ms
Array Lookup: 156ms
 Dict Lookup: 753ms

[jonathanpeppers]

Seeing the manual work involved to test, should we have a BenchmarkDotNet project in this repo?

It seems like it would be useful to commit your benchmark above, and we'd have the option to run it later. BDN also does appropriate warmup, runs out of process, does some math, that might generally make it better than a manual benchmark.

[jonpryor]

Yes, we should probably have one, and it should probably be in tests/Java.Interop-PerformanceTests (and/or I could "just" add the above app to Java.Interop-PerformanceTests…).

[jonathanpeppers]

Should candidateParameterTypes be a HashSet? So Contains() can be faster?

[jonpryor]

As mentioned above, I'm concerned about app startup time overheads, and I thus believe/assert that HashSet/Dictionary/etc. creation and population will take much more time than an array, and because this array is (1) small, and (2) never changed, init time should be minimal. (But I should verify this.)

[jonpryor]

I think I was confused above; this is from ManagedPeer.Construct(), which isn't part of app startup, and no static data is in the picture.

Despite being confused, I'm think I'm more or less right, though: each element of candidateParameterTypes will be a small list, likely no more than 5 elements. I believe that the overhead of a HashSet vs. a List at such small sizes is not warranted. Arrays are fast!

[jpobst]

Should we run an XA test PR on this before merging?

[jonpryor]

My general questions about performance depend on how often these would be called at runtime by a MAUI app?

Right now, nothing; this codepath is only hit by "JavaInterop1" Java Callable Wrappers, not Xamarin.Android "XAJavaInterop1" Java Callable Wrappers. (The codepath is present in .NET Android; it's just that only our unit tests will actually exercise it in practice.) Rephrased: the only way to hit it is for Java code to call ManagedPeer.construct(), and .NET Android apps use TypeManager.Activate(). No "real" .NET Android apps currently use ManagedPeer.construct().

This is thus a more "exploratory quest" to answer the question: so what about Type.GetType() (IL2057), and what can we do instead? (TL;DR: yes, it's possible. It's also fugly. I have no idea if it's actually worth it.) That question needs to be "appropriately answered" in order to support NativeAOT, if that is ever to happen.

[jonpryor]

@jpobst asked

Should we run an XA test PR on this before merging?

Yes, but given how we have a unit test failing here, I'll need to fix that first. We might be able to consider merging by end-of-year? November is out.

[jonathanpeppers]

Seeing that we tested out the performance, I don't really have any other concerns here. 👍

[jonathanpeppers]

Is the C# attribute available where we could use nameof()? The resulting IL would be the same, but might help typos.

[jonpryor]

No, this assembly doesn't have a reference to Java.Interop.dll.

[jonpryor]

@jpobst, @jonathanpeppers: @Suchiman on https://libera.chat / #csharp mentioned an alternate way to make Type.GetType() work in a NativeAOT context:

1. Create a new method that contains all the Type.GetType() invocations that are needed.
2. The method in (1) needs to be called, but doesn't need to actually "do" anything; it can early return (but it the early return needs to be conditioned on a parameter):

   void TellNativeAotAboutTypes(bool loadTypes)
   {
       if (!loadTypes)
           return;
       Type.GetType ("System.Int32, System.Runtime");
       // …
   }

3. You can then Type.GetType() a non-const value and the type will be found and loaded.

For example:

using System.Text;

TellNativeAotAboutTypes ();

var t = Type.GetType (GetTypeName (), throwOnError: true);

Console.WriteLine (t);

string GetTypeName() =>
    new StringBuilder()
    .Append ("System.")
    .Append ("Int32")
    .Append (", System.Runtime")
    .ToString ();

void TellNativeAotAboutTypes (bool load = false)
{
    if (!load)
        return;
    Type.GetType ("System.Int32, System.Runtime");
}

The above app works at runtime. If you remove the TellNativeAotAboutTypes() invocation, or remove the bool load=false parameter while keeping the early return, then execution fails with:

Unhandled Exception: System.IO.FileNotFoundException: Could not resolve assembly 'System.Runtime'.
   at System.Reflection.TypeNameParser.ResolveAssembly(String) + 0x97
   at System.Reflection.TypeNameParser.GetType(String, ReadOnlySpan`1, String) + 0x32
   at System.Reflection.TypeNameParser.NamespaceTypeName.ResolveType(TypeNameParser&, String) + 0x17
   at System.Reflection.TypeNameParser.GetType(String, Func`2, Func`4, Boolean, Boolean, Boolean, String) + 0x99
   at Program.<Main>$(String[] args) + 0x3d
   at native-aot-with-Suchiman-workaround!<BaseAddress>+0x120dbc

However, compilation still generates an IL2057 warning:

% dotnet publish -c Release -r osx-x64
…
…/Program.cs(8,9): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type. 

So the good news is that we have a more plausible workaround in Java.Interop that doesn't require changing TypeManager.Activate() to do what was done in this PR, and further suggests that we could revert #1168 (this PR).

The bad news is that this described workaround means we have IL2057 trim warnings, but we could [UnconditionalSuppressMessage]/etc. the IL2057 warning in that context.

@jonathanpeppers, @jpobst: what do you think we should do? Revert #1168 so that ManagedPeer is semantically closer to TypeManager and require this "pre-register types" workaround? Or keep #1168?

[MichalStrehovsky]

Instead of TellNativeAotAboutTypes, could you replace the uses of Type.GetType with an enum? For example:

Type GetTheType(TypeId id) =>
    id switch
    {
        Id.Int32 => typeof(int),
        // or Id.Int32 => Type.GetType("System.Int32, System.Runtime"),
    }

This way you don't have to do any suppressions, it's trim safe by construction.

Alternatively:

var dict = new Dictionary<string, Type>
{
    { "System.Int32, System.Runtime", typeof(int) },
    ...
}

I assume you're going to do something with the type (like Activator.CreateInstance it), so you'll probably actually need something along the lines of this:

struct DictionaryOfConstructableTypes
{
    Dictionary<string, Type> _inner;

    public void Add(string s, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t) => _inner.Add(s, t);

    // We need to do a suppression here because static analysis cannot guarantee
    // whatever comes out of the dictionary has the constructors kept, but it's easy to audit
    // that it is in fact the case manually because the method that adds them is annotated to keep them.
    [UnconditionalSuppressMessage(...)]
    [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]
    public Type Get(string s) => _inner[s];
}

This way you can pass anything that comes out of this dictionary to Activator.CreateInstance because the static analysis can guarantee both the type and the constructor are available.

[jonpryor]

(The perils of comments after closing the PR. I didn't see @MichalStrehovsky 's comment until just now…)

@MichalStrehovsky: I lack the imagination to see how replacing Type.GetType() with an enum would work, in practice.

Firstly, there's architecture: "core" assemblies like Java.Interop.dll need this "alternate mechanism", but your hypothetical enum cannot be constructed until app build time, when all assemblies in the app are known. By definition, Java.Interop.dll cannot reference such an enum, unless the enum is empty, at which point it's basically an int. "Trim-safe by construction" it's not.

Secondly, generated Java code needs to be able to refer to C# types, and this mapping is done at packaging time. With enough hand-waving one can imagine a world where at app packaging time we create an int::type mapping, hard-coding these int values into the Java code.

Thirdly, this isn't incremental: change an assembly, add a new type, and all your previous int::type mappings are potentially wrong. Rebuilding the world is slow, and something we need to avoid in Debug builds. This in turn might be solvable, but it doesn't look easy in the 5s of mental energy I gave it.

It also doesn't quite consider the sheer number of entries we're looking at. The Mono.Android-Tests unit tests (debug builds, unlinked) contains 8274 typemap entries, and that isn't a MAUI app, nor does it reference AndroidX, either/both of which will explode the number of types involved. (A default MAUI app, debug build, contains 14309 typemap entries.)

The Dictionary<string, Type> approach appears viable, except for scale. Again, 8274 entries, and this dict needs to be initialized during app startup in order for anything to work properly. That's an actually significant amount of startup overhead, which is scary. (Related.)

The approach committed in 005c914 (which may or may not need to be reverted) builds atop the typemap infrastructure which already exists, and needs to exist, and has been optimized a fair bit. The alternate approach has the benefit of needing fewer changes overall, and avoids the need for me to construct a gigantic multi-thousand entry dict mapping strings to types. (Native AOT will presumably need to create such a mapping, somehow, but that's Not My Problem™ {until it is, due to perf hits}.)