Make JniTypeManager faster by optimizing some code #582

grendello · 2020-02-24T08:17:15Z

Optionally do not look for the JniAddNativeMethodRegistrationAttribute.
It turns out that this attribute is not commonly present in
Xamarin.Android applications and so all of the calls to
GetCustomAttribute failed to find it, incurring unnecessary
performance penalty on the application.

Make the code consult a global flag (in case of Xamarin.Android it is
set on the build time) which tells it whether or not to skip querying
for the above custom attribute. The flag is true by default to
maintain backward compatibility.

Optimize the JniTypeManager.GetTypeSignature method to not require an
allocation of Lazy<> + a number of instances of KeyValuePair<> as
well as JniTypeSignature. This affects Xamarin.Android startup a
little:

Before:
  344        9        199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type)

After:
  82        9        199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type)

jonpryor · 2020-02-24T16:30:48Z

src/Java.Interop/Java.Interop/JniTypeManagerOptions.cs

+	{
+		// If `false`, reflection checks for the presence of the `JniAddNativeMethodRegistrationAttribute` attribute
+		// throughout the **entire** application will be skipped.
+		public static bool JniAddNativeMethodRegistrationAttributePresent { get; set; } = true;


This need not exist here. Instead, it should be in JniRuntime.CreationOptions and in JniRuntime.

jonpryor · 2020-02-24T16:31:24Z

src/Java.Interop/Java.Interop/JniRuntime.JniTypeManager.cs


 			static bool FindAndCallRegisterMethod (Type marshalType, JniNativeMethodRegistrationArguments arguments)
 			{
+				if (!JniTypeManagerOptions.JniAddNativeMethodRegistrationAttributePresent)


This would then need to become an instance method, and check Runtime.JniAddNativeMethodRegistrationAttributePresent.

jonpryor · 2020-02-24T16:33:12Z

Current failed build should be ignorable; it's a SIGSEGV from mono.

jonpryor · 2020-02-25T16:50:23Z

src/Java.Interop/Java.Interop/JniRuntime.cs


 		internal    bool                                TrackIDs            {get; private set;}
 		internal    bool                                NewObjectRequired   {get; private set;}
+		internal    CreationOptions                     Options             { get; }


This shouldn't be needed; just add a JniAddNativeMethodRegistrationAttributePresent property, assign it, as is done with JniVersion, InvocationPointer, etc. The approach here will just keep the CreationOptions instance alive ~forever, "wasting" some amount of memory that would otherwise be collectible.

jonpryor · 2020-02-25T17:29:49Z

src/Java.Interop/Java.Interop/JniTypeSignature.cs

 		}

-		public JniTypeSignature (string simpleReference, int arrayRank = 0, bool keyword = false)
+		public JniTypeSignature (string? simpleReference, int arrayRank = 0, bool keyword = false)


Why do we allow a null value here? We don't even test what happens when null is provided!

Does it even make sense to allow null here?

Oh, it's a struct; there's no way to prevent null from here.

That said, that doesn't in turn mean that this constructor needs to support a null parameter, just that it's not possible in general to prevent SimpleReference from being null.

Thoughts?

I can see just instance when SimpleReference should be allowed to be null - creation of JniTypeSignature.Empty field to use as the default value for variables. As you mentioned on Slack, though, this could be done using a default constructor (possibly private?), though. Could SimpleReference be String.Empty by default?

Could SimpleReference be String.Empty by default

No. It's a struct; structs are special, and one way in which they're special is that they always have a default constructor which "zero-initializes" all members, which means that after invoking the default constructor SimpleReference can never be string.Empty; it can only be null.

Which is in part why JniTypeSignature.SimpleReference is of type string? (after enabling nullable-reference-types); there's always a chance it could be null, so all callers need to be careful.

That does not in any way mean or imply that the constructor needs to allow simpleReference to be null. It may be "nice" in that it allows "equivalent construction" between the default constructor and the non-default constructor -- which in itself may be a good thing! -- but it isn't required.

I can't find anything in the Framework Design Guidelines which address this point.

jonpryor · 2020-02-26T01:27:12Z

src/Java.Interop/Java.Interop/JavaPrimitiveArrays.cs

+
+		static bool GetBuiltInTypeArraySignature (Type type, ref JniTypeSignature signature)
+		{
+			if (type == typeof (JavaPrimitiveArray<Boolean>)) {


Should this also be checking against JavaBooleanArray?

I didn't change anything in the .tt generation logic here - the types checked against are the same types we had previously.

jonpryor · 2020-02-26T01:28:03Z

src/Java.Interop/Java.Interop/JavaPrimitiveArrays.cs

+		static bool GetBuiltInTypeArraySignature (Type type, ref JniTypeSignature signature)
+		{
+			if (type == typeof (JavaPrimitiveArray<Boolean>)) {
+				signature = GetCachedTypeSignature (ref __BooleanTypePrimitiveArraySignature, "Z", arrayRank: 1, keyword: true);


Why both __BooleanTypePrimitiveArraySignature and __BooleanTypeArraySignature? They both have the same signature, [Z, and would be Equal to each other. Why not just have the single field?

I didn't want to change the old code semantics, and it kept the two types distinct (maybe because it was all stored in an array) and I always aim to make the minimal set of changes necessary.

Optionally do not look for the `JniAddNativeMethodRegistrationAttribute`. It turns out that this attribute is not commonly present in Xamarin.Android applications and so all of the calls to `GetCustomAttribute` failed to find it, incurring unnecessary performance penalty on the application. Make the code consult a global flag (in case of Xamarin.Android it is set on the build time) which tells it whether or not to skip querying for the above custom attribute. The flag is `true` by default to maintain backward compatibility. Optimize the `JniTypeManager.GetTypeSignature` method to not require an allocation of `Lazy<>` + a number of instances of `KeyValuePair<>` as well as `JniTypeSignature`. This affects Xamarin.Android startup a little: Before: 344 9 199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type) After: 82 9 199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type)

...otherwise we get `LV;`, which is wrong, and corresponding unit test failures. See: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3507789&view=ms.vss-test-web.build-test-results-tab&runId=11520990&resultId=100015&paneView=debug

Should fix: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3507789&view=ms.vss-test-web.build-test-results-tab&runId=11520992&resultId=100290&paneView=debug

Replace the `JniRuntime.JniBuiltinArrayMappings` field and its associated `Lazy<T>` initializer with new methods `JniRuntime.GetBuiltInTypeArraySignature()` and `JniRuntime.GetBuiltInTypeSignature()`, which removes an array initialization from the `JniRuntime` static constructor and an array iteration from `JniRuntime.JniTypeManager.GetTypeSignature()`. Additionally, use `Type.GetTypeCode(Type)` to more quickly map builtin types such as `string` to `java/lang/String`. Additionally, add a new `JniRuntime.CreationOptions.JniAddNativeMethodRegistrationAttributePresent` property which controls whether or not `JniRuntime.JniTypeManager.TryRegisterNativeMembers()` attempts to look for the `[JniAddNativeMethodRegistration]` custom attribute on methods. This is a seldom used attribute, and if no types in the app use the attribute, this can save precious startup time by removing the `method.GetCustomAttribute()` calls. These changes together improve Xamarin.Android startup time, saving ~262ms: * Before: Total(ms) Self(ms) Calls Method name 344 9 199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type) * After: Total(ms) Self(ms) Calls Method name 82 9 199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type)

Context: dotnet/android#4302 Context: dotnet#582 Java.Interop tests use their own runtime which doesn't know about the results of Xamarin.Android's build process scanning for the presence of the `JniAddNativeMethodRegistration` custom attribute. In addition, Java.Interop's Java type scanner (for typemaps) does not include in its results the types in `Java.Interop-Tests` which derive from `Java.Interop.JavaObject` (which is **functionally** equivalent to Xamarin.Android's `Java.Lang.Object`) and, thus, the scan doesn't notice that the custom attribute mentioned above is used by a few test classes. In effect, two tests fail with a similar exception: Java.Lang.LinkageError : No implementation found for void com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(int) (tried Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor and Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor__I) at Java.Interop.JniEnvironment+InstanceMethods.CallNonvirtualVoidMethod (Java.Interop.JniObjectReference instance, Java.Interop.JniObjectReference type, Java.Interop.JniMethodInfo method, Java.Interop.JniArgumentValue* args) [0x0008e] in <d30c11568ddc48d5aff8f2d4b2eaa867>:0 at Java.Interop.GenericMarshaler.JniPeerInstanceMethodsExtensions._InvokeConstructor[T] (Java.Interop.JniPeerMembers+JniInstanceMethods peer, System.String constructorSignature, Java.Interop.IJavaPeerable self, T value) [0x00059] in <f6f553d46eb149a29e6542964ef27aaa>:0 at Java.Interop.GenericMarshaler.JniPeerInstanceMethodsExtensions.FinishGenericCreateInstance[T] (Java.Interop.JniPeerMembers+JniInstanceMethods peer, System.String constructorSignature, Java.Interop.IJavaPeerable self, T value) [0x00029] in <f6f553d46eb149a29e6542964ef27aaa>:0 at Java.InteropTests.CallVirtualFromConstructorBase..ctor (System.Int32 value) [0x0004e] in <d39267c627f54df3bc31406b36effb6c>:0 at Java.InteropTests.CallVirtualFromConstructorDerived..ctor (System.Int32 value) [0x00000] in <d39267c627f54df3bc31406b36effb6c>:0 at Java.InteropTests.InvokeVirtualFromConstructorTests.InvokeVirtualFromConstructor () [0x00000] in <d39267c627f54df3bc31406b36effb6c>:0 at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&) at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <26b1d17e27e54492bd3e4105b16bfc7e>:0 --- End of managed Java.Lang.LinkageError stack trace --- java.lang.UnsatisfiedLinkError: No implementation found for void com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(int) (tried Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor and Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor__I) at com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(Native Method) at com.xamarin.interop.CallVirtualFromConstructorBase.<init>(Unknown Source:35) at com.xamarin.interop.CallVirtualFromConstructorDerived.<init>(Unknown Source:2) at crc64f295cabbf85394f5.TestSuiteInstrumentation.n_onStart(Native Method) at crc64f295cabbf85394f5.TestSuiteInstrumentation.onStart(Unknown Source:0) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189) Force the tests to always assume the custom attribute is present, thus restoring the default behavior from before Java.Interop PR582

Context: dotnet/android#4302 Context: #582 Java.Interop tests use their own runtime which doesn't know about the results of Xamarin.Android's build process scanning for the presence of the `[JniAddNativeMethodRegistration]` custom attribute. In addition, Java.Interop's Java type scanner (for typemaps) does not include in its results the types in `Java.Interop-Tests` which derive from `Java.Interop.JavaObject` (which is *functionally* equivalent to Xamarin.Android's `Java.Lang.Object`) and, thus, the scan doesn't notice that the custom attribute mentioned above is used by a few test classes. In effect, two tests fail with a similar exception: Java.Lang.LinkageError : No implementation found for void com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(int) (tried Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor and Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor__I) at Java.Interop.JniEnvironment+InstanceMethods.CallNonvirtualVoidMethod (Java.Interop.JniObjectReference instance, Java.Interop.JniObjectReference type, Java.Interop.JniMethodInfo method, Java.Interop.JniArgumentValue* args) [0x0008e] in <d30c11568ddc48d5aff8f2d4b2eaa867>:0 at Java.Interop.GenericMarshaler.JniPeerInstanceMethodsExtensions._InvokeConstructor[T] (Java.Interop.JniPeerMembers+JniInstanceMethods peer, System.String constructorSignature, Java.Interop.IJavaPeerable self, T value) [0x00059] in <f6f553d46eb149a29e6542964ef27aaa>:0 at Java.Interop.GenericMarshaler.JniPeerInstanceMethodsExtensions.FinishGenericCreateInstance[T] (Java.Interop.JniPeerMembers+JniInstanceMethods peer, System.String constructorSignature, Java.Interop.IJavaPeerable self, T value) [0x00029] in <f6f553d46eb149a29e6542964ef27aaa>:0 at Java.InteropTests.CallVirtualFromConstructorBase..ctor (System.Int32 value) [0x0004e] in <d39267c627f54df3bc31406b36effb6c>:0 at Java.InteropTests.CallVirtualFromConstructorDerived..ctor (System.Int32 value) [0x00000] in <d39267c627f54df3bc31406b36effb6c>:0 at Java.InteropTests.InvokeVirtualFromConstructorTests.InvokeVirtualFromConstructor () [0x00000] in <d39267c627f54df3bc31406b36effb6c>:0 at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&) at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <26b1d17e27e54492bd3e4105b16bfc7e>:0 --- End of managed Java.Lang.LinkageError stack trace --- java.lang.UnsatisfiedLinkError: No implementation found for void com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(int) (tried Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor and Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor__I) at com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(Native Method) at com.xamarin.interop.CallVirtualFromConstructorBase.<init>(Unknown Source:35) at com.xamarin.interop.CallVirtualFromConstructorDerived.<init>(Unknown Source:2) at crc64f295cabbf85394f5.TestSuiteInstrumentation.n_onStart(Native Method) at crc64f295cabbf85394f5.TestSuiteInstrumentation.onStart(Unknown Source:0) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189) Force the tests to always assume the custom attribute is present, thus restoring the default behavior from before b33d183.

Replace the `JniRuntime.JniBuiltinArrayMappings` field and its associated `Lazy<T>` initializer with new methods `JniRuntime.GetBuiltInTypeArraySignature()` and `JniRuntime.GetBuiltInTypeSignature()`, which removes an array initialization from the `JniRuntime` static constructor and an array iteration from `JniRuntime.JniTypeManager.GetTypeSignature()`. Additionally, use `Type.GetTypeCode(Type)` to more quickly map builtin types such as `string` to `java/lang/String`. Additionally, add a new `JniRuntime.CreationOptions.JniAddNativeMethodRegistrationAttributePresent` property which controls whether or not `JniRuntime.JniTypeManager.TryRegisterNativeMembers()` attempts to look for the `[JniAddNativeMethodRegistration]` custom attribute on methods. This is a seldom used attribute, and if no types in the app use the attribute, this can save precious startup time by removing the `method.GetCustomAttribute()` calls. These changes together improve Xamarin.Android startup time, saving ~262ms: * Before: Total(ms) Self(ms) Calls Method name 344 9 199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type) * After: Total(ms) Self(ms) Calls Method name 82 9 199 Java.Interop.JniRuntime/JniTypeManager:GetTypeSignature (System.Type)

Context: dotnet/android#4302 Context: #582 Java.Interop tests use their own runtime which doesn't know about the results of Xamarin.Android's build process scanning for the presence of the `[JniAddNativeMethodRegistration]` custom attribute. In addition, Java.Interop's Java type scanner (for typemaps) does not include in its results the types in `Java.Interop-Tests` which derive from `Java.Interop.JavaObject` (which is *functionally* equivalent to Xamarin.Android's `Java.Lang.Object`) and, thus, the scan doesn't notice that the custom attribute mentioned above is used by a few test classes. In effect, two tests fail with a similar exception: Java.Lang.LinkageError : No implementation found for void com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(int) (tried Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor and Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor__I) at Java.Interop.JniEnvironment+InstanceMethods.CallNonvirtualVoidMethod (Java.Interop.JniObjectReference instance, Java.Interop.JniObjectReference type, Java.Interop.JniMethodInfo method, Java.Interop.JniArgumentValue* args) [0x0008e] in <d30c11568ddc48d5aff8f2d4b2eaa867>:0 at Java.Interop.GenericMarshaler.JniPeerInstanceMethodsExtensions._InvokeConstructor[T] (Java.Interop.JniPeerMembers+JniInstanceMethods peer, System.String constructorSignature, Java.Interop.IJavaPeerable self, T value) [0x00059] in <f6f553d46eb149a29e6542964ef27aaa>:0 at Java.Interop.GenericMarshaler.JniPeerInstanceMethodsExtensions.FinishGenericCreateInstance[T] (Java.Interop.JniPeerMembers+JniInstanceMethods peer, System.String constructorSignature, Java.Interop.IJavaPeerable self, T value) [0x00029] in <f6f553d46eb149a29e6542964ef27aaa>:0 at Java.InteropTests.CallVirtualFromConstructorBase..ctor (System.Int32 value) [0x0004e] in <d39267c627f54df3bc31406b36effb6c>:0 at Java.InteropTests.CallVirtualFromConstructorDerived..ctor (System.Int32 value) [0x00000] in <d39267c627f54df3bc31406b36effb6c>:0 at Java.InteropTests.InvokeVirtualFromConstructorTests.InvokeVirtualFromConstructor () [0x00000] in <d39267c627f54df3bc31406b36effb6c>:0 at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&) at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <26b1d17e27e54492bd3e4105b16bfc7e>:0 --- End of managed Java.Lang.LinkageError stack trace --- java.lang.UnsatisfiedLinkError: No implementation found for void com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(int) (tried Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor and Java_com_xamarin_interop_CallVirtualFromConstructorDerived_calledFromConstructor__I) at com.xamarin.interop.CallVirtualFromConstructorDerived.calledFromConstructor(Native Method) at com.xamarin.interop.CallVirtualFromConstructorBase.<init>(Unknown Source:35) at com.xamarin.interop.CallVirtualFromConstructorDerived.<init>(Unknown Source:2) at crc64f295cabbf85394f5.TestSuiteInstrumentation.n_onStart(Native Method) at crc64f295cabbf85394f5.TestSuiteInstrumentation.onStart(Unknown Source:0) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189) Force the tests to always assume the custom attribute is present, thus restoring the default behavior from before b33d183.

jonpryor reviewed Feb 24, 2020

View reviewed changes

grendello force-pushed the no-reflection branch from d940181 to ad4885d Compare February 24, 2020 18:35

grendello changed the title ~~Make GetCustomAttribute calls optional in JniTypeManager~~ Make JniTypeManager faster by optimizing some code Feb 25, 2020

jonpryor reviewed Feb 25, 2020

View reviewed changes

grendello force-pushed the no-reflection branch 2 times, most recently from c34fef4 to 63c5c34 Compare February 25, 2020 17:03

grendello mentioned this pull request Feb 25, 2020

Startup performance improvements (less reflection) dotnet/android#4302

Merged

jonpryor reviewed Feb 25, 2020

View reviewed changes

jonpryor reviewed Feb 26, 2020

View reviewed changes

grendello force-pushed the no-reflection branch from 63c5c34 to 8c2f7e4 Compare February 26, 2020 08:31

grendello force-pushed the no-reflection branch from 8c2f7e4 to 18f9e78 Compare February 27, 2020 10:09

jonpryor added 3 commits February 28, 2020 17:07

V is a keyword.

ddc6cbb

...otherwise we get `LV;`, which is wrong, and corresponding unit test failures. See: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3507789&view=ms.vss-test-web.build-test-results-tab&runId=11520990&resultId=100015&paneView=debug

V is a keyword.

7e3dcf7

...otherwise we get `LV;`, which is wrong, and corresponding unit test failures. See: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3507789&view=ms.vss-test-web.build-test-results-tab&runId=11520990&resultId=100015&paneView=debug

Return the null check

b2351e2

Should fix: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3507789&view=ms.vss-test-web.build-test-results-tab&runId=11520992&resultId=100290&paneView=debug

jonpryor merged commit b33d183 into dotnet:master Mar 3, 2020

grendello deleted the no-reflection branch March 3, 2020 08:09

grendello mentioned this pull request Mar 6, 2020

Force looking for the [JniAddNativeMethodRegistration] attribute #595

Merged

brendanzagaeski added this to the In Preview: 10.3 (d16-6) milestone Mar 27, 2020

github-actions bot locked and limited conversation to collaborators Apr 13, 2024

Make JniTypeManager faster by optimizing some code #582

Make JniTypeManager faster by optimizing some code #582

Uh oh!

Conversation

grendello commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Feb 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grendello commented Feb 24, 2020 •

edited

Loading